Skip to content

Commit 28d8fdc

Browse files
bewithgauravCopilot
bewithgaurav
and
Copilot
authored
REFACTOR: Shift get_driver_path from Python to DDBC bindings with tests (#168)
### Work Item / Issue Reference <!-- IMPORTANT: Please follow the PR template guidelines below. For mssql-python maintainers: Insert your ADO Work Item ID below (e.g. AB#37452) For external contributors: Insert Github Issue number below (e.g. #149) Only one reference is required - either GitHub issue OR ADO Work Item. --> <!-- mssql-python maintainers: ADO Work Item --> > [AB#38102](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/38102) <!-- External contributors: GitHub Issue --> > GitHub Issue: #<ISSUE_NUMBER> ------------------------------------------------------------------- ### Summary <!-- Insert your summary of changes below. Minimum 10 characters required. --> 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]](diffhunk://#diff-dde2297345718ec449a14e7dff91b7bb2342b008ecc071f562233646d71144a1L640-R713) [[2]](diffhunk://#diff-dde2297345718ec449a14e7dff91b7bb2342b008ecc071f562233646d71144a1L665-R729) * 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]](diffhunk://#diff-e489a2a29f29954dcd0b17a34b15e53ee2aa4fa1def0a5a466bcee81df59ddf0L318-L332) [[2]](diffhunk://#diff-e489a2a29f29954dcd0b17a34b15e53ee2aa4fa1def0a5a466bcee81df59ddf0R364-R380) * Added helper imports and a method to compute the expected driver path in the test suite for accurate cross-validation. [[1]](diffhunk://#diff-e489a2a29f29954dcd0b17a34b15e53ee2aa4fa1def0a5a466bcee81df59ddf0R12-R14) [[2]](diffhunk://#diff-e489a2a29f29954dcd0b17a34b15e53ee2aa4fa1def0a5a466bcee81df59ddf0R170-L168) <!-- ### PR Title Guide > For feature requests FEAT: (short-description) > For non-feature requests like test case updates, config updates , dependency updates etc CHORE: (short-description) > For Fix requests FIX: (short-description) > For doc update requests DOC: (short-description) > For Formatting, indentation, or styling update STYLE: (short-description) > For Refactor, without any feature changes REFACTOR: (short-description) > For release related changes, without any feature changes RELEASE: #<RELEASE_VERSION> (short-description) ### Contribution Guidelines External contributors: - Create a GitHub issue first: https://github.com/microsoft/mssql-python/issues/new - Link the GitHub issue in the "GitHub Issue" section above - Follow the PR title format and provide a meaningful summary mssql-python maintainers: - Create an ADO Work Item following internal processes - Link the ADO Work Item in the "ADO Work Item" section above - Follow the PR title format and provide a meaningful summary --> --------- Co-authored-by: Copilot <[email protected]>
1 parent 2f3ada6 commit 28d8fdc

File tree

3 files changed

+129
-121
lines changed

3 files changed

+129
-121
lines changed

‎mssql_python/helpers.py‎

Lines changed: 0 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -114,78 +114,6 @@ def add_driver_name_to_app_parameter(connection_string):
114114
return ";".join(modified_parameters) + ";"
115115

116116

117-
def detect_linux_distro():
118-
"""
119-
Detect Linux distribution for driver path selection.
120-
121-
Returns:
122-
str: Distribution name ('debian_ubuntu', 'rhel', 'alpine', etc.)
123-
"""
124-
import os
125-
126-
distro_name = "debian_ubuntu" # default
127-
128-
try:
129-
if os.path.exists("/etc/os-release"):
130-
with open("/etc/os-release", "r") as f:
131-
content = f.read()
132-
for line in content.split("\n"):
133-
if line.startswith("ID="):
134-
distro_id = line.split("=", 1)[1].strip('"\'')
135-
if distro_id in ["ubuntu", "debian"]:
136-
distro_name = "debian_ubuntu"
137-
elif distro_id in ["rhel", "centos", "fedora"]:
138-
distro_name = "rhel"
139-
elif distro_id == "alpine":
140-
distro_name = "alpine"
141-
else:
142-
distro_name = distro_id # use as-is
143-
break
144-
except Exception:
145-
pass # use default
146-
147-
return distro_name
148-
149-
def get_driver_path(module_dir, architecture):
150-
"""
151-
Get the platform-specific ODBC driver path.
152-
153-
Args:
154-
module_dir (str): Base module directory
155-
architecture (str): Target architecture (x64, arm64, x86, etc.)
156-
157-
Returns:
158-
str: Full path to the ODBC driver file
159-
160-
Raises:
161-
RuntimeError: If driver not found or unsupported platform
162-
"""
163-
164-
platform_name = platform.system().lower()
165-
normalized_arch = normalize_architecture(platform_name, architecture)
166-
167-
if platform_name == "windows":
168-
driver_path = Path(module_dir) / "libs" / "windows" / normalized_arch / "msodbcsql18.dll"
169-
170-
elif platform_name == "darwin":
171-
driver_path = Path(module_dir) / "libs" / "macos" / normalized_arch / "lib" / "libmsodbcsql.18.dylib"
172-
173-
elif platform_name == "linux":
174-
distro_name = detect_linux_distro()
175-
driver_path = Path(module_dir) / "libs" / "linux" / distro_name / normalized_arch / "lib" / "libmsodbcsql-18.5.so.1.1"
176-
177-
else:
178-
raise RuntimeError(f"Unsupported platform: {platform_name}")
179-
180-
driver_path_str = str(driver_path)
181-
182-
# Check if file exists
183-
if not driver_path.exists():
184-
raise RuntimeError(f"ODBC driver not found at: {driver_path_str}")
185-
186-
return driver_path_str
187-
188-
189117
def sanitize_connection_string(conn_str: str) -> str:
190118
"""
191119
Sanitize the connection string by removing sensitive information.

‎mssql_python/pybind/ddbc_bindings.cpp‎

Lines changed: 66 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -637,20 +637,66 @@ std::string GetLastErrorMessage() {
637637
#endif
638638
}
639639

640-
// Function to call Python get_driver_path function
641-
std::string GetDriverPathFromPython(const std::string& moduleDir, const std::string& architecture) {
642-
try {
643-
py::module_ helpers = py::module_::import("mssql_python.helpers");
644-
py::object get_driver_path = helpers.attr("get_driver_path");
645-
py::str result = get_driver_path(moduleDir, architecture);
646-
return std::string(result);
647-
} catch (const py::error_already_set& e) {
648-
LOG("Python error in get_driver_path: {}", e.what());
649-
ThrowStdException("Failed to get driver path from Python: " + std::string(e.what()));
650-
} catch (const std::exception& e) {
651-
LOG("Error calling get_driver_path: {}", e.what());
652-
ThrowStdException("Failed to get driver path: " + std::string(e.what()));
653-
}
640+
641+
/*
642+
* Resolve ODBC driver path in C++ to avoid circular import issues on Alpine.
643+
*
644+
* Background:
645+
* On Alpine Linux, calling into Python during module initialization (via pybind11)
646+
* causes a circular import due to musl's stricter dynamic loader behavior.
647+
*
648+
* Specifically, importing Python helpers from C++ triggered a re-import of the
649+
* partially-initialized native module, which works on glibc (Ubuntu/macOS) but
650+
* fails on musl-based systems like Alpine.
651+
*
652+
* By moving driver path resolution entirely into C++, we avoid any Python-layer
653+
* dependencies during critical initialization, ensuring compatibility across
654+
* all supported platforms.
655+
*/
656+
std::string GetDriverPathCpp(const std::string& moduleDir) {
657+
namespace fs = std::filesystem;
658+
fs::path basePath(moduleDir);
659+
660+
std::string platform;
661+
std::string arch;
662+
663+
// Detect architecture
664+
#if defined(__aarch64__) || defined(_M_ARM64)
665+
arch = "arm64";
666+
#elif defined(__x86_64__) || defined(_M_X64) || defined(_M_AMD64)
667+
arch = "x86_64"; // maps to "x64" on Windows
668+
#else
669+
throw std::runtime_error("Unsupported architecture");
670+
#endif
671+
672+
// Detect platform and set path
673+
#ifdef __linux__
674+
if (fs::exists("/etc/alpine-release")) {
675+
platform = "alpine";
676+
} else if (fs::exists("/etc/redhat-release") || fs::exists("/etc/centos-release")) {
677+
platform = "rhel";
678+
} else {
679+
platform = "debian_ubuntu";
680+
}
681+
682+
fs::path driverPath = basePath / "libs" / "linux" / platform / arch / "lib" / "libmsodbcsql-18.5.so.1.1";
683+
return driverPath.string();
684+
685+
#elif defined(__APPLE__)
686+
platform = "macos";
687+
fs::path driverPath = basePath / "libs" / platform / arch / "lib" / "libmsodbcsql.18.dylib";
688+
return driverPath.string();
689+
690+
#elif defined(_WIN32)
691+
platform = "windows";
692+
// Normalize x86_64 to x64 for Windows naming
693+
if (arch == "x86_64") arch = "x64";
694+
fs::path driverPath = basePath / "libs" / platform / arch / "msodbcsql18.dll";
695+
return driverPath.string();
696+
697+
#else
698+
throw std::runtime_error("Unsupported platform");
699+
#endif
654700
}
655701

656702
DriverHandle LoadDriverOrThrowException() {
@@ -662,8 +708,11 @@ DriverHandle LoadDriverOrThrowException() {
662708
std::string archStr = ARCHITECTURE;
663709
LOG("Architecture: {}", archStr);
664710

665-
// Use Python function to get the correct driver path for the platform
666-
std::string driverPathStr = GetDriverPathFromPython(moduleDir, archStr);
711+
// Use only C++ function for driver path resolution
712+
// Not using Python function since it causes circular import issues on Alpine Linux
713+
// and other platforms with strict module loading rules.
714+
std::string driverPathStr = GetDriverPathCpp(moduleDir);
715+
667716
fs::path driverPath(driverPathStr);
668717

669718
LOG("Driver path determined: {}", driverPath.string());
@@ -2448,7 +2497,7 @@ PYBIND11_MODULE(ddbc_bindings, m) {
24482497

24492498
// Expose the C++ functions to Python
24502499
m.def("ThrowStdException", &ThrowStdException);
2451-
m.def("get_driver_path", &GetDriverPathFromPython, "Get platform-specific ODBC driver path");
2500+
m.def("GetDriverPathCpp", &GetDriverPathCpp, "Get the path to the ODBC driver");
24522501

24532502
// Define parameter info class
24542503
py::class_<ParamInfo>(m, "ParamInfo")

‎tests/test_000_dependencies.py‎

Lines changed: 63 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
import sys
1010
from pathlib import Path
1111

12+
from mssql_python.ddbc_bindings import normalize_architecture
13+
1214

1315
class DependencyTester:
1416
"""Helper class to test platform-specific dependencies."""
@@ -57,23 +59,26 @@ def _normalize_architecture(self):
5759
def _detect_linux_distro(self):
5860
"""Detect Linux distribution for driver path selection."""
5961
distro_name = "debian_ubuntu" # default
60-
62+
'''
63+
#ifdef __linux__
64+
if (fs::exists("/etc/alpine-release")) {
65+
platform = "alpine";
66+
} else if (fs::exists("/etc/redhat-release") || fs::exists("/etc/centos-release")) {
67+
platform = "rhel";
68+
} else {
69+
platform = "ubuntu";
70+
}
71+
72+
fs::path driverPath = basePath / "libs" / "linux" / platform / arch / "lib" / "libmsodbcsql-18.5.so.1.1";
73+
return driverPath.string();
74+
'''
6175
try:
62-
if os.path.exists("/etc/os-release"):
63-
with open("/etc/os-release", "r") as f:
64-
content = f.read()
65-
for line in content.split("\n"):
66-
if line.startswith("ID="):
67-
distro_id = line.split("=", 1)[1].strip('"\'')
68-
if distro_id in ["ubuntu", "debian"]:
69-
distro_name = "debian_ubuntu"
70-
elif distro_id in ["rhel", "centos", "fedora"]:
71-
distro_name = "rhel"
72-
elif distro_id == "alpine":
73-
distro_name = "alpine"
74-
else:
75-
distro_name = distro_id
76-
break
76+
if (Path("/etc/alpine-release").exists()):
77+
distro_name = "alpine"
78+
elif (Path("/etc/redhat-release").exists() or Path("/etc/centos-release").exists()):
79+
distro_name = "rhel"
80+
else:
81+
distro_name = "debian_ubuntu"
7782
except Exception:
7883
pass # use default
7984

@@ -164,6 +169,30 @@ def get_expected_python_extension(self):
164169

165170
return self.module_dir / extension_name
166171

172+
def get_expected_driver_path(self):
173+
platform_name = platform.system().lower()
174+
normalized_arch = normalize_architecture(platform_name, self.normalized_arch)
175+
176+
if platform_name == "windows":
177+
driver_path = Path(self.module_dir) / "libs" / "windows" / normalized_arch / "msodbcsql18.dll"
178+
179+
elif platform_name == "darwin":
180+
driver_path = Path(self.module_dir) / "libs" / "macos" / normalized_arch / "lib" / "libmsodbcsql.18.dylib"
181+
182+
elif platform_name == "linux":
183+
distro_name = self._detect_linux_distro()
184+
driver_path = Path(self.module_dir) / "libs" / "linux" / distro_name / normalized_arch / "lib" / "libmsodbcsql-18.5.so.1.1"
185+
186+
else:
187+
raise RuntimeError(f"Unsupported platform: {platform_name}")
188+
189+
driver_path_str = str(driver_path)
190+
191+
# Check if file exists
192+
if not driver_path.exists():
193+
raise RuntimeError(f"ODBC driver not found at: {driver_path_str}")
194+
195+
return driver_path_str
167196

168197
# Create global instance for use in tests
169198
dependency_tester = DependencyTester()
@@ -314,21 +343,6 @@ def test_python_extension_imports(self):
314343

315344
except Exception as e:
316345
pytest.fail(f"Failed to import or use ddbc_bindings: {e}")
317-
318-
def test_helper_functions_work(self):
319-
"""Test that helper functions can detect platform correctly."""
320-
try:
321-
from mssql_python.helpers import get_driver_path
322-
323-
# Test that get_driver_path works for current platform
324-
driver_path = get_driver_path(str(dependency_tester.module_dir), dependency_tester.normalized_arch)
325-
326-
assert Path(driver_path).exists(), \
327-
f"Driver path returned by get_driver_path does not exist: {driver_path}"
328-
329-
except Exception as e:
330-
pytest.fail(f"Failed to use helper functions: {e}")
331-
332346

333347
# Print platform information when tests are collected
334348
def pytest_runtest_setup(item):
@@ -349,4 +363,21 @@ def test_ddbc_bindings_import():
349363
import mssql_python.ddbc_bindings
350364
assert True, "ddbc_bindings module imported successfully."
351365
except ImportError as e:
352-
pytest.fail(f"Failed to import ddbc_bindings: {e}")
366+
pytest.fail(f"Failed to import ddbc_bindings: {e}")
367+
368+
369+
370+
def test_get_driver_path_from_ddbc_bindings():
371+
"""Test the GetDriverPathCpp function from ddbc_bindings."""
372+
try:
373+
import mssql_python.ddbc_bindings as ddbc
374+
module_dir = dependency_tester.module_dir
375+
376+
driver_path = ddbc.GetDriverPathCpp(str(module_dir))
377+
378+
# The driver path should be same as one returned by the Python function
379+
expected_path = dependency_tester.get_expected_driver_path()
380+
assert driver_path == str(expected_path), \
381+
f"Driver path mismatch: expected {expected_path}, got {driver_path}"
382+
except Exception as e:
383+
pytest.fail(f"Failed to call GetDriverPathCpp: {e}")

0 commit comments

Comments
 (0)