-
Notifications
You must be signed in to change notification settings - Fork 35
FEAT: Modernize build system with pyproject.toml and custom build backend #408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add build_ddbc package for native extension compilation - python -m build_ddbc: standalone compilation CLI - Custom PEP 517 backend: auto-compiles on python -m build - Consolidate project config into pyproject.toml - Moved pytest config from pytest.ini - Added optional dependencies [dev], [lint], [all] - Added coverage, mypy, black, autopep8, pylint configs - Simplified setup.py to platform-specific wheel logic only - Delete pytest.ini (moved to pyproject.toml)
📊 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.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 69.4%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.1%
mssql_python.cursor.py: 84.7%🔗 Quick Links
|
|
It seems this lacks build support for FreeBSD, which would be necessary for my organization to use this project. |
…tection - Remove unused 'sys' and 'Path' imports from build_backend.py - Remove unused 'os' import from setup.py - Fix find_pybind_dir() to check for platform-appropriate build script (build.bat on Windows, build.sh on Unix)
….com/microsoft/mssql-python into bewithgaurav/pyproject-modernization
- Change Development Status from Beta to Production/Stable (GA release) - Add Python 3.14 to classifiers and Black target versions - Remove unused [tool.flake8] section (flake8 doesn't read pyproject.toml natively)
….com/microsoft/mssql-python into bewithgaurav/pyproject-modernization
- Remove flake8 from pyproject.toml lint dependencies - Remove flake8 from requirements.txt - Remove flake8 step from lint-check.yml workflow - Remove .flake8 trigger from workflow paths - Delete .flake8 config file Flake8 was never enforced (continue-on-error: true), only Black is blocking.
There was a problem hiding this 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 PR modernizes the build system for mssql-python by introducing a custom PEP 517 build backend and consolidating configuration into pyproject.toml. The changes automate native extension compilation, eliminate the need for pytest.ini, and provide both a standalone CLI tool and automatic build integration.
Changes:
- Introduced
build_ddbcpackage as a PEP 517-compliant build backend with CLI support for compiling native extensions - Consolidated project metadata, dependencies, and tool configurations from setup.py into pyproject.toml
- Migrated pytest configuration from pytest.ini to pyproject.toml and deleted pytest.ini
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added comprehensive build system configuration, project metadata, dependencies, and tool configurations (pytest, coverage, black, autopep8, pylint, mypy) |
| setup.py | Simplified to focus only on platform-specific package discovery and wheel platform tag customization; imports from build_ddbc package |
| pytest.ini | Deleted (configuration moved to pyproject.toml) |
| build_ddbc/init.py | Package initialization exporting compile_ddbc and get_platform_info functions |
| build_ddbc/main.py | CLI entry point for standalone compilation with architecture and coverage options |
| build_ddbc/compiler.py | Core compilation logic with platform detection and build script execution |
| build_ddbc/build_backend.py | PEP 517 build backend wrapper that auto-compiles extensions before building wheels |
Comments suppressed due to low confidence (1)
pyproject.toml:192
- The flake8 configuration was removed from pyproject.toml, but flake8 doesn't support reading configuration from pyproject.toml natively (it only reads from .flake8, setup.cfg, or tox.ini). Since a .flake8 file already exists in the repository with the appropriate configuration, this removal is correct. However, the PR description states 'Added coverage, mypy, black, autopep8, pylint configs' which could be misleading since the flake8 config was removed rather than consolidated. The existing .flake8 file will continue to be used.
# Type Checking - MyPy
# =============================================================================
[tool.mypy]
python_version = "3.10"
warn_return_any = true
warn_unused_configs = true
ignore_missing_imports = true
exclude = [
"build",
"dist",
"tests",
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@codexterous - thanks for reaching out to us, if you feel this is something we can help you with, please feel free to open a new issue here - https://github.com/microsoft/mssql-python/issues |
- compiler.py: Move platform import to module level - compiler.py: Fix libc detection logic (default to glibc when detection fails) - compiler.py: Lower macOS platform tag from 15.0 to 11.0 (Big Sur) - compiler.py: Update coverage param docs to say Linux/macOS - compiler.py: Print stdout on build failure (not just stderr) - build_backend.py: Handle build_editable import gracefully with better error - pyproject.toml: Fix license field to SPDX string format with license-files - pyproject.toml: Add explicit build_ddbc/tests/benchmarks exclusion - pyproject.toml: Expand self-reference in optional deps for older pip - setup.py: Fix circular dependency by duplicating get_platform_info
- Removed duplicate get_platform_info() from setup.py - Import directly from build_ddbc.compiler instead - Single source of truth for platform detection logic
dlevy-msft-sql
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: PR #408 - Modernize Build System
PR: #408
Author: @bewithgaurav
Reviewer: @dlevy-msft-sql
Overview
This PR modernizes the build system by:
- Adding a
build_ddbcPEP 517 build backend for automatic native extension compilation - Consolidating configuration into
pyproject.toml - Removing Flake8 in favor of Black and Pylint
Overall, this is a well-structured modernization effort. The PEP 517 build backend is a great addition. However, I found several issues that should be addressed before merging.
🔴 Critical Issues
1. Security: shell=True in subprocess (compiler.py:128-134)
result = subprocess.run(
cmd,
cwd=pybind_dir,
shell=True, # ⚠️ Security concern
check=False,
capture_output=not verbose,
)Problem: Using shell=True with subprocess.run on Windows is a security risk and is unnecessary when passing a list of arguments.
Fix: Remove shell=True:
result = subprocess.run(
cmd,
cwd=pybind_dir,
check=False,
capture_output=not verbose,
)2. Alpine/musl Detection Broken (compiler.py:41-52)
Old behavior (setup.py):
is_musl = libc_name == "" or "musl" in libc_name.lower()New behavior (compiler.py):
if not libc_name:
print("[build_ddbc] Warning: libc detection failed...", file=sys.stderr)
is_musl = False # ⚠️ Breaking change!
else:
is_musl = "musl" in libc_name.lower()Problem: The old code treated empty libc_name as musl (Alpine). The new code treats it as glibc (manylinux). This is a breaking behavioral change for Alpine Linux - wheels will have incorrect platform tags.
Fix: Add explicit musl detection fallback:
import glob
if not libc_name:
# Fallback: check for musl linker (Alpine Linux)
is_musl = bool(glob.glob("/lib/ld-musl*"))
if not is_musl:
print("[build_ddbc] Warning: libc detection failed; defaulting to glibc.", file=sys.stderr)
else:
is_musl = "musl" in libc_name.lower()🟠 High-Priority Issues
3. Import Failure in setup.py (setup.py:18)
from build_ddbc.compiler import get_platform_infoProblem: If build_ddbc package doesn't exist (e.g., clean git clone without package installation), setup.py will fail with an import error before any build can happen.
Fix: Add fallback handling:
try:
from build_ddbc.compiler import get_platform_info
except ImportError:
# Fallback for environments where build_ddbc isn't installed
def get_platform_info():
# ... inline fallback implementation
pass4. Version Duplication (init.py:18, main.py:54)
Files:
build_ddbc/__init__.py:__version__ = "1.0.0"build_ddbc/__main__.py:version="%(prog)s 1.0.0"
Problem: Version is hardcoded in two places. These can drift during updates.
Fix: Import version from __init__.py:
# In __main__.py
from . import __version__
parser.add_argument(
"--version", "-V",
action="version",
version=f"%(prog)s {__version__}",
)5. Missing OSError Handling (build_backend.py:114-125)
try:
compile_ddbc(verbose=True)
except FileNotFoundError:
print("...")
except RuntimeError as e:
print(f"[build_backend] Compilation failed: {e}")
raiseProblem: get_platform_info() can raise OSError for unsupported platforms, but this exception is not caught in build_editable().
Fix: Add OSError to the exception handling:
except (FileNotFoundError, RuntimeError, OSError) as e:
if isinstance(e, FileNotFoundError):
print("[build_backend] Build scripts not found, assuming pre-compiled binaries")
else:
print(f"[build_backend] Compilation failed: {e}")
raise🟡 Medium-Priority Issues
6. Hardcoded macOS 15 Version (compiler.py:35)
return "universal2", "macosx_15_0_universal2"Problem: macOS 15 (Sequoia) is hardcoded. This limits backward compatibility and requires manual updates for future macOS versions.
Suggestion: Consider using a lower minimum version for broader compatibility:
return "universal2", "macosx_10_15_universal2"7. Inconsistent Architecture CLI Handling (main.py:32-36)
parser.add_argument(
"--arch", "-a",
choices=["x64", "x86", "arm64", "x86_64", "aarch64", "universal2"],
help="Target architecture (Windows: x64, x86, arm64)",
)Problem: The help text says "Windows only" but the choices include Unix architectures (x86_64, aarch64, universal2). Also, these Unix values are accepted but never normalized in compile_ddbc().
Fix: Either:
- Remove Unix architectures from choices, OR
- Add normalization in
compile_ddbc():
if arch in ["x86_64"]:
arch = "x64"
elif arch in ["aarch64"]:
arch = "arm64"8. Package Data Too Greedy (pyproject.toml)
[tool.setuptools.package-data]
mssql_python = [
"ddbc_bindings.cp*.pyd",
"ddbc_bindings.cp*.so",
"libs/*",
"libs/**/*", # ⚠️ This is greedy
"*.dll",
"*.pyi",
]Problem: libs/**/* will include ALL files under libs, including potentially large debug symbols, intermediate files, or files not needed at runtime.
Suggestion: Be more specific:
"libs/**/*.dll",
"libs/**/*.so",
"libs/**/*.dylib",
"libs/**/LICENSING",🟢 Low-Priority / Style Issues
9. Duplicate Optional Dependencies (pyproject.toml:69-93)
The [all] optional dependency duplicates entries from [dev] and [lint]:
all = [
# Dev dependencies (duplicated from above)
"pytest",
...
]Suggestion: Use dependency references (PEP 685, Python 3.12+):
all = [
"mssql-python[dev,lint]",
]10. Missing __all__ Export (build_backend.py)
PEP 517 hooks aren't exported via __all__. While not required, it's good practice for clarity.
__all__ = [
"get_requires_for_build_wheel",
"get_requires_for_build_sdist",
"prepare_metadata_for_build_wheel",
"build_wheel",
"build_sdist",
"get_requires_for_build_editable",
"build_editable",
]11. Inconsistent Print Prefixes
Different prefixes are used across files:
[build_ddbc]incompiler.pyand__main__.py[build_backend]inbuild_backend.py[setup.py]insetup.py
Suggestion: Standardize to [mssql-python] or use Python's logging module for consistency.
12. No README for build_ddbc Package
The package has no documentation explaining:
- How the build backend works
- How to debug build failures
- Architecture decisions
Suggestion: Add a brief build_ddbc/README.md.
Summary Table
| Severity | Issue | File | Line(s) |
|---|---|---|---|
| 🔴 Critical | shell=True security risk |
compiler.py | 128-134 |
| 🔴 Critical | Alpine musl detection broken | compiler.py | 41-52 |
| 🟠 High | Import failure in setup.py | setup.py | 18 |
| 🟠 High | Version duplication | init.py, main.py | 18, 54 |
| 🟠 High | Missing OSError handling | build_backend.py | 114-125 |
| 🟡 Medium | Hardcoded macOS 15 version | compiler.py | 35 |
| 🟡 Medium | Inconsistent arch CLI handling | main.py | 32-36 |
| 🟡 Medium | Package data too greedy | pyproject.toml | - |
| 🟢 Low | Duplicate optional dependencies | pyproject.toml | 69-93 |
| 🟢 Low | Missing __all__ export |
build_backend.py | - |
| 🟢 Low | Inconsistent print prefixes | Various | - |
| 📝 Docs | No build_ddbc README | - | - |
Recommended Actions Before Merge
- Fix Alpine/musl detection - This is a breaking change for existing Alpine users
- Remove
shell=True- Security best practice - Add import fallback in setup.py - Ensures clean clones work
- Consolidate version definitions - Prevents drift
- Add OSError handling - Prevents cryptic errors on unsupported platforms
Review generated by Claude Opus 4.5: January 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix Alpine musl detection with glob fallback for /lib/ld-musl* - Remove shell=True from Windows subprocess call (security) - Fix version duplication - import __version__ from __init__.py - Add OSError handling in build_wheel and build_editable - Sync build_ddbc version to 1.2.0 (matches package)
- Bump setuptools>=64.0.0 (required for PEP 660 editable installs) - Remove [tool.coverage] section - .coveragerc is single source of truth
Work Item / Issue Reference
Summary
This pull request introduces a new build backend for the project, modernizes and consolidates the build and lint configuration, and removes Flake8 in favor of Black and Pylint for Python linting. The most significant changes are the addition of the
build_ddbcpackage for native extension compilation, migration of all build and lint configuration topyproject.toml, and the removal of Flake8 from the codebase and CI workflow.Build system modernization and native extension support:
build_ddbcpackage, including__init__.py,__main__.py,compiler.py, andbuild_backend.py, which provides a PEP 517 build backend for automatic compilation of nativeddbc_bindingsduring wheel and editable installs. This enables platform-aware builds and CLI usage for compiling native extensions. [1] [2] [3] [4]Configuration consolidation and modernization:
pyproject.tomlfile, removing the need for separate configuration files like.flake8andpytest.ini. [1] [2] [3] [4]Linting and CI workflow simplification:
.flake8config, removing Flake8 installation and checks from the GitHub Actions workflow, and updating the workflow summary to reference only Pylint for Python linting. [1] [2] [3] [4] [5]Other improvements:
pyproject.toml.