CHORE: validate macOS wheel is self-contained by removing system ODBC before tests#658
Merged
Merged
Conversation
📊 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: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection.cpp: 76.2%
mssql_python.pybind.ddbc_bindings.cpp: 76.2%
mssql_python.__init__.py: 77.3%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.6%
mssql_python.logging.py: 85.5%🔗 Quick Links
|
Revert the 'uninstall system unixODBC' step (it only ran on the x86_64 runner, whose dylib is already correct, and it broke the benchmark by removing unixODBC that pyodbc needs). Add test_macos_dylibs_have_no_absolute_dependency_paths in test_000_dependencies.py. It runs 'otool -L' on every bundled dylib in both libs/macos/arm64 and libs/macos/x86_64 and fails if a sibling bundled library is referenced via an absolute path instead of @loader_path/@rpath. Because otool reads Mach-O load commands of any architecture, the x86_64 macOS CI lane now catches the arm64-only packaging bug behind GitHub #656.
Verified against the shipped binaries that: - arm64/libmsodbcsql.18.dylib hardcodes /opt/homebrew/lib/libodbcinst.2.dylib (the GH #656 break); x86_64 gets rewritten to @loader_path at build time because configure_dylibs.sh only fixes ARCH=\ on the Intel runner. - ddbc_bindings dlopens libmsodbcsql.18.dylib DIRECTLY, so libodbc.2.dylib is off the runtime load path (and is never touched by configure_dylibs.sh). The previous test scanned every dylib, so it would have flagged the dead libodbc.2.dylib and could never go green even after the real fix. Replace it with test_macos_driver_load_chain_is_relocatable, which starts at the driver and follows only its bundled sibling deps -- matching what dlopen resolves at runtime, ignoring external openssl, and catching the arm64 bug from an x86_64 runner via otool.
…river newer Homebrew (6.x) refuses to load formulae from third-party taps unless trusted, so brew install msodbcsql18 in the macOS benchmark step failed with "Refusing to load formula ... from untrusted tap". the driver never installed, so every pyodbc connection failed with "[unixODBC][Driver Manager]Can't open lib 'ODBC Driver 18 for SQL Server' : file not found" and the benchmark's pyodbc column came out all N/A. the step has continueOnError so the job stayed green. add brew trust microsoft/mssql-release between the tap and install. verified locally: forcing HOMEBREW_REQUIRE_TAP_TRUST=1 reproduces the error, brew trust clears it, and msodbcsql18 resolves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
pr-size: medium
bewithgaurav
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens macOS packaging validation by adding a dependency-inspection test to ensure the bundled msodbcsql18 load chain doesn’t hardcode Homebrew absolute paths (addressing GitHub issue #656), and tweaks the macOS pipeline’s Homebrew tap handling for installing msodbcsql18 during benchmarks.
Changes:
- Added a macOS-only test that walks
libmsodbcsql.18.dylib’s bundled dependency graph usingotool -Land fails on absolute-path links to bundled siblings. - Added a
brew trustattempt before installingmsodbcsql18in the macOS benchmark step.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/test_000_dependencies.py |
Adds a macOS relocatability test that inspects Mach-O dependency paths for bundled dylibs. |
eng/pipelines/pr-validation-pipeline.yml |
Adds a Homebrew “trust tap” attempt before installing msodbcsql18 for macOS benchmarks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
black (line-length 100) collapses the multi-line subprocess.run into one 99-char line. fixes the Python Linting check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
pr-size: small
…river the macOS pytest job had no step to remove system/Homebrew unixODBC and msodbcsql18 before running tests, unlike every Linux job. without it, mssql-python could load a system driver manager from the default dyld path and pass, hiding a broken bundle. the otool load-chain test is a static check and does not guarantee the bundled driver manager is the one loaded at runtime. add an uninstall step to PytestOnMacOS mirroring the Linux jobs: remove Homebrew msodbcsql18/mssql-tools18/unixodbc, delete leftover libodbc/libodbcinst dylibs and odbcinst config from both Homebrew prefixes, then otool -L the bundled driver to confirm it is intact. every command is guarded so it is a no-op when nothing is installed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bewithgaurav
changed the title
github-actions
Bot
added
pr-size: medium
the relocatability test silently continued when a driver dylib was missing, so if the packaging layout ever changed the test would pass without validating anything. now it records which arches were checked and asserts at least one bundled driver was found. also corrects the walk comment: queue.pop() is LIFO, so it is depth-first, not breadth-first (order does not affect the result). addresses the two review comments on the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
gargsaumya
approved these changes
Jul 15, 2026
bewithgaurav
approved these changes
Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
on macOS the test job never removed system ODBC before running, so mssql-python could load a Homebrew unixODBC driver manager / msodbcsql18 from the default dyld path and pass even if the bundled copy was broken. every Linux job already uninstalls system ODBC before tests to force the bundled driver. this brings macOS in line and adds a static check of the bundle itself.