FEAT: Linux Support #110
FEAT: Linux Support #110
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements Linux support by unifying build scripts and ODBC bindings across macOS and Linux, removes Windows-specific pipeline jobs, and consolidates platform-specific modules.
- Removes Windows-only pipeline job and Docker-based macOS steps, streamlining CI.
- Updates build scripts (
build.sh, CMakeLists) and Python module loader for Linux. - Consolidates and refactors ODBC binding code to be cross-platform and drops deprecated files.
Reviewed Changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_003_connection.py | Added pooling performance and basic tests |
| mssql_python/pybind/mac_utils.h | Expanded platform guard to include Linux |
| mssql_python/pybind/mac_utils.cpp | Unified macOS/Linux utility definitions |
| mssql_python/pybind/ddbc_bindings.h | Introduced cross-platform dynamic loader and converters |
| mssql_python/pybind/ddbc_bindings.cpp | Refactored driver loading, logging, and removed Windows includes |
| mssql_python/pybind/build.sh | Extended build script to detect Linux and macOS |
| mssql_python/pybind/CMakeLists.txt | Removed macOS-only source branching |
| mssql_python/ddbc_bindings.py | Added Linux architecture mapping |
| mssql_python/db_connection.py | Simplified imports to unified connection module |
| eng/pipelines/pr-validation-pipeline.yml | Dropped Windows job, simplified macOS job |
Comments suppressed due to low confidence (3)
tests/test_003_connection.py:221
- [nitpick] In test_connection_pooling_basic, catching Exception without asserting expected failure leaves the test unclear. Use pytest.raises to explicitly verify that acquiring a third connection exceeds the pool limit.
except Exception as e:
mssql_python/pybind/mac_utils.cpp:4
- [nitpick] The comment refers to a header, but this is a .cpp file. Update the comment to correctly describe the implementation file.
// This header defines utility functions for safely handling SQLWCHAR-based
mssql_python/pybind/ddbc_bindings.h:68
- The file mac_buffers.h is included but not present in the diff. Ensure this header exists or remove the include to prevent compilation errors.
#include "mac_buffers.h" // For macOS-specific buffer handling
| @@ -10,7 +18,7 @@ | |||
|
|
|||
| namespace py = pybind11; | |||
|
|
|||
| #if defined(__APPLE__) | |||
| #if defined(__APPLE__) || defined(__linux__) | |||
There was a problem hiding this comment.
[nitpick] The filename mac_utils.h suggests macOS-only utilities, but it now includes Linux logic. Consider renaming to os_utils.h or similar to reflect cross-platform support.
Copilot uses AI. Check for mistakes.
| std::cout << "Detected platform: " << platformName << std::endl; | ||
| std::cout << "Detected driver file name: " << driverFileName << std::endl; |
There was a problem hiding this comment.
[nitpick] Avoid using std::cout in library code; switch to the LOG() function or a dedicated logger for consistent, configurable output.
| std::cout << "Detected platform: " << platformName << std::endl; | |
| std::cout << "Detected driver file name: " << driverFileName << std::endl; | |
| LOG("Detected platform: {}", platformName); | |
| LOG("Detected driver file name: {}", driverFileName); |
Copilot uses AI. Check for mistakes.
mssql_python/pybind/CMakeLists.txt
Outdated
| message(STATUS "Using standard source file: ${DDBC_SOURCE}") | ||
| # Include connection module for Windows | ||
| add_library(ddbc_bindings MODULE ${DDBC_SOURCE} connection/connection.cpp connection/connection_pool.cpp) | ||
| # endif() |
There was a problem hiding this comment.
There is a commented '# endif()' without a matching 'if' block. Remove or correct this leftover comment to avoid confusion.
| # endif() |
Copilot uses AI. Check for mistakes.
…into bewithgaurav/linux
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Draft PR: NOT FOR REVIEW
ADO Work Item Reference
Summary
This pull request introduces significant updates to the
pr-validation-pipeline.ymlfile and themssql_pythonmodule, focusing on streamlining pipeline jobs, improving cross-platform compatibility, and removing deprecated code. The most important changes include removing Windows-specific pipeline jobs, enhancing support for Linux in build scripts, and simplifying themssql_pythonmodule by eliminating platform-specific code.Pipeline Updates:
PytestOnWindowsjob and its associated steps, including dependency installation, database setup, and artifact publishing. These tasks are no longer required. (eng/pipelines/pr-validation-pipeline.yml, eng/pipelines/pr-validation-pipeline.ymlL10-R10)TestMacOSjob by replacing Docker-based SQL Server setup with an Azure SQL Database connection string for testing purposes. (eng/pipelines/pr-validation-pipeline.yml, eng/pipelines/pr-validation-pipeline.ymlR36-R45)eng/pipelines/pr-validation-pipeline.yml, eng/pipelines/pr-validation-pipeline.ymlL100-L138)Cross-Platform Compatibility:
build.shscript to support both macOS and Linux, including dynamic OS detection and corresponding build configurations. (mssql_python/pybind/build.sh, [1] [2] [3]ddbc_bindings.pyto handle Linux architectures (x86_64andarm64) and raise errors for unsupported architectures. (mssql_python/ddbc_bindings.py, mssql_python/ddbc_bindings.pyR15-R21)Codebase Simplification:
connection_mac.pyfile entirely, consolidating platform-specific connection logic into a singleconnection.pyfile. (mssql_python/connection_mac.py, mssql_python/connection_mac.pyL1-L327)CMakeLists.txtby removing macOS-specific source file handling and using a unified source file for all platforms. (mssql_python/pybind/CMakeLists.txt, mssql_python/pybind/CMakeLists.txtL181-R185)