Skip to content

REFACTOR: Shift get_driver_path from Python to DDBC bindings with tests#168

Merged
bewithgaurav merged 7 commits intomainfrom
bewithgaurav/refactor_get_driver_path
Aug 6, 2025
Merged

REFACTOR: Shift get_driver_path from Python to DDBC bindings with tests#168
bewithgaurav merged 7 commits intomainfrom
bewithgaurav/refactor_get_driver_path

Conversation

@bewithgaurav
Copy link
Collaborator

@bewithgaurav bewithgaurav commented Aug 5, 2025

Work Item / Issue Reference

AB#38102

GitHub Issue: #<ISSUE_NUMBER>


Summary

This pull request refactors the ODBC driver path resolution logic to be implemented entirely in C++ instead of Python. The main motivation is to eliminate a circular import issue that caused failures on Alpine Linux due to musl libc's stricter dynamic loading rules. The Python helper and its associated test have been removed, and equivalent logic is now handled in C++. The test suite has been updated to validate the new C++-based resolution.

Refactor: Move driver path resolution to C++

  • Removed the get_driver_path function from mssql_python/helpers.py, eliminating Python-based driver path resolution.
  • Implemented GetDriverPathCpp in mssql_python/pybind/ddbc_bindings.cpp to handle all platform and architecture-specific ODBC driver path resolution in C++. This avoids Python dependencies during module initialization and resolves the Alpine Linux circular import issue. [1] [2]
  • Updated the C++ Python bindings to expose GetDriverPathCpp instead of the removed Python function.

Testing updates

  • Removed tests for the old Python helper and added a new test to validate the C++ GetDriverPathCpp function against expected driver paths derived in Python. [1] [2]
  • Added helper imports and a method to compute the expected driver path in the test suite for accurate cross-validation. [1] [2]

Copilot AI review requested due to automatic review settings August 5, 2025 11:32
@bewithgaurav bewithgaurav changed the title REFACTOR: Shift get_driver_path from Python to DDBC bindings REFACTOR: Shift get_driver_path from Python to DDBC bindings with tests Aug 5, 2025
@github-actions github-actions bot added the pr-size: medium Moderate update size label Aug 5, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request refactors ODBC driver path resolution from Python to C++ to eliminate circular import issues on Alpine Linux caused by musl libc's stricter dynamic loading rules.

  • Removed the Python get_driver_path function from helpers.py and implemented equivalent logic in C++ (GetDriverPathCpp)
  • Updated C++ bindings to use the new C++ implementation instead of calling Python helpers during module initialization
  • Modified test suite to validate the new C++ driver path resolution against expected paths

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
mssql_python/helpers.py Removed the Python get_driver_path function
mssql_python/pybind/ddbc_bindings.cpp Added C++ GetDriverPathCpp implementation and updated module bindings
tests/test_000_dependencies.py Replaced Python helper test with C++ function validation test

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 5, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 5, 2025
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 5, 2025
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 5, 2025
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 6, 2025
Copy link
Contributor

@gargsaumya gargsaumya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bewithgaurav bewithgaurav merged commit 28d8fdc into main Aug 6, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: medium Moderate update size

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants