CHORE: Accept mssql-python-rs wheel package - #760
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new “fallback on any download failure” behavior can mask transient/network errors and unexpectedly select legacy artifacts, which is a reliability risk for build/install automation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Rust-core wheel installation scripts in eng/scripts/ to prefer the renamed NuGet package mssql-python-rs-wheels and the new wheel filename prefix mssql_python_rs-*, while preserving fallback support for the legacy package ID / wheel prefix so mssql_py_core can still be imported from a source checkout.
Changes:
- Switch wheel selection to try
mssql_python_rs-*first, then fall back tomssql_py_core-*. - Download NuGet artifacts by trying
mssql-python-rs-wheelsfirst, thenmssql-py-core-wheelsif needed. - Update script/docs text and default temp output directory naming to reflect the new package.
File summaries
| File | Description |
|---|---|
| eng/scripts/resolve_nuget_feed.py | Updates feed-resolution documentation to reference the new NuGet package name. |
| eng/scripts/install-mssql-py-core.sh | Prefers new NuGet package ID and wheel prefix, with legacy fallback for compatibility. |
| eng/scripts/install-mssql-py-core.ps1 | Mirrors the Bash installer changes for Windows/PowerShell environments. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 77.5%
mssql_python.row.py: 77.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.6%
mssql_python.pybind.connection.connection.cpp: 84.4%
mssql_python.logging.py: 85.5%
mssql_python.helpers.py: 89.3%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to installer fallback behavior and appear correct, with only a minor docstring clarity nit remaining.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
eng/scripts/resolve_nuget_feed.py:44
- The docstring says this helper is needed to download only the
mssql-python-rs-wheelsnupkg, but the install scripts use the resolved base URL for both the new and legacy package IDs. Updating the wording avoids misleading future readers when maintaining the fallback logic.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The fallback logic and wheel selection changes are consistent across Bash/PowerShell and do not introduce regressions in the reviewed script flows.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
lgtm
Work Item / Issue Reference
Summary
Update the Rust-core wheel installers to prefer the renamed
mssql-python-rs-wheelsNuGet package andmssql_python_rs-*wheel prefix while retaining fallback support for existingmssql-py-core-wheelspackages andmssql_py_core-*wheels.The pinned transport version remains
0.1.9until stablemssql-python-rs-wheels 0.1.10is published. The Python import remainsmssql_py_core.Validation
0.1.9wheel selection andmssql_py_coreimport verified