Skip to content

Commit c42f819

Browse files
committed
fix: Address all Copilot review comments
- 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
1 parent a1667e6 commit c42f819

4 files changed

Lines changed: 95 additions & 19 deletions

File tree

build_ddbc/build_backend.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
# PEP 517 Required Hooks
2828
# =============================================================================
2929

30+
3031
def get_requires_for_build_wheel(config_settings=None):
3132
"""Return build requirements for wheel."""
3233
return _get_requires_for_build_wheel(config_settings)
@@ -97,6 +98,7 @@ def build_sdist(sdist_directory, config_settings=None):
9798
# Optional PEP 660 Hooks (Editable Installs)
9899
# =============================================================================
99100

101+
100102
def get_requires_for_build_editable(config_settings=None):
101103
"""Return build requirements for editable install."""
102104
return get_requires_for_build_wheel(config_settings)
@@ -121,6 +123,12 @@ def build_editable(wheel_directory, config_settings=None, metadata_directory=Non
121123
print(f"[build_backend] Compilation failed: {e}")
122124
raise
123125

124-
# Import here to avoid issues if not available
125-
from setuptools.build_meta import build_editable as _setuptools_build_editable
126+
# Import here and handle absence gracefully for older setuptools versions
127+
try:
128+
from setuptools.build_meta import build_editable as _setuptools_build_editable
129+
except ImportError as exc:
130+
raise RuntimeError(
131+
"Editable installs are not supported with this setuptools version. "
132+
"Please install setuptools>=64.0.0 to use PEP 660 editable installs."
133+
) from exc
126134
return _setuptools_build_editable(wheel_directory, config_settings, metadata_directory)

build_ddbc/compiler.py

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
"""
66

77
import os
8+
import platform
89
import sys
910
import subprocess
1011
from pathlib import Path
@@ -34,13 +35,21 @@ def get_platform_info() -> Tuple[str, str]:
3435
return "x64", "win_amd64"
3536

3637
elif sys.platform.startswith("darwin"):
37-
return "universal2", "macosx_15_0_universal2"
38+
return "universal2", "macosx_11_0_universal2"
3839

3940
elif sys.platform.startswith("linux"):
40-
import platform
4141
target_arch = os.environ.get("targetArch", platform.machine())
4242
libc_name, _ = platform.libc_ver()
43-
is_musl = libc_name == "" or "musl" in libc_name.lower()
43+
# Empty libc_name could indicate detection failure; default to glibc (manylinux)
44+
if not libc_name:
45+
print(
46+
"[build_ddbc] Warning: libc detection failed (platform.libc_ver() "
47+
"returned an empty name); defaulting to glibc (manylinux) tags.",
48+
file=sys.stderr,
49+
)
50+
is_musl = False
51+
else:
52+
is_musl = "musl" in libc_name.lower()
4453

4554
if target_arch == "x86_64":
4655
return "x86_64", "musllinux_1_2_x86_64" if is_musl else "manylinux_2_28_x86_64"
@@ -86,7 +95,7 @@ def compile_ddbc(
8695
8796
Args:
8897
arch: Target architecture (Windows only: x64, x86, arm64)
89-
coverage: Enable coverage instrumentation (Linux only)
98+
coverage: Enable coverage instrumentation (Linux/macOS only)
9099
verbose: Print build output
91100
92101
Returns:
@@ -128,8 +137,11 @@ def _run_windows_build(pybind_dir: Path, arch: str, verbose: bool) -> bool:
128137
)
129138

130139
if result.returncode != 0:
131-
if not verbose and result.stderr:
132-
print(result.stderr.decode(), file=sys.stderr)
140+
if not verbose:
141+
if result.stdout:
142+
print(result.stdout.decode(), file=sys.stderr)
143+
if result.stderr:
144+
print(result.stderr.decode(), file=sys.stderr)
133145
raise RuntimeError(f"build.bat failed with exit code {result.returncode}")
134146

135147
if verbose:
@@ -163,8 +175,11 @@ def _run_unix_build(pybind_dir: Path, coverage: bool, verbose: bool) -> bool:
163175
)
164176

165177
if result.returncode != 0:
166-
if not verbose and result.stderr:
167-
print(result.stderr.decode(), file=sys.stderr)
178+
if not verbose:
179+
if result.stdout:
180+
print(result.stdout.decode(), file=sys.stderr)
181+
if result.stderr:
182+
print(result.stderr.decode(), file=sys.stderr)
168183
raise RuntimeError(f"build.sh failed with exit code {result.returncode}")
169184

170185
if verbose:

pyproject.toml

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ version = "1.2.0"
1515
description = "A Python library for interacting with Microsoft SQL Server"
1616
readme = "PyPI_Description.md"
1717
license = "MIT"
18+
license-files = ["LICENSE"]
1819
requires-python = ">=3.10"
1920
authors = [
2021
{ name = "Microsoft Corporation", email = "mssql-python@microsoft.com" }
@@ -68,7 +69,23 @@ lint = [
6869
"types-setuptools",
6970
]
7071
all = [
71-
"mssql-python[dev,lint]",
72+
# Dev dependencies
73+
"pytest",
74+
"pytest-cov",
75+
"coverage",
76+
"unittest-xml-reporting",
77+
"psutil",
78+
"pybind11",
79+
"setuptools",
80+
"wheel",
81+
"build",
82+
# Lint dependencies
83+
"black",
84+
"autopep8",
85+
"pylint",
86+
"cpplint",
87+
"mypy",
88+
"types-setuptools",
7289
]
7390

7491
# =============================================================================
@@ -81,6 +98,7 @@ include-package-data = true
8198
[tool.setuptools.packages.find]
8299
where = ["."]
83100
include = ["mssql_python*"]
101+
exclude = ["build_ddbc*", "tests*", "benchmarks*"]
84102

85103
[tool.setuptools.package-data]
86104
mssql_python = [

setup.py

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,30 +14,65 @@
1414
pip install -e . # Editable install with auto-compile
1515
"""
1616

17+
import os
18+
import platform
1719
import sys
1820

1921
from setuptools import setup, find_packages
2022
from setuptools.dist import Distribution
2123
from wheel.bdist_wheel import bdist_wheel
2224

23-
from build_ddbc import get_platform_info
25+
26+
def get_platform_info():
27+
"""
28+
Get platform-specific architecture and platform tag information.
29+
30+
Note: This is duplicated from build_ddbc.compiler to avoid circular imports
31+
during fresh installs where build_ddbc isn't available yet.
32+
"""
33+
if sys.platform.startswith("win"):
34+
arch = os.environ.get("ARCHITECTURE", "x64")
35+
if isinstance(arch, str):
36+
arch = arch.strip("\"'")
37+
if arch in ["x86", "win32"]:
38+
return "x86", "win32"
39+
elif arch == "arm64":
40+
return "arm64", "win_arm64"
41+
else:
42+
return "x64", "win_amd64"
43+
elif sys.platform.startswith("darwin"):
44+
return "universal2", "macosx_11_0_universal2"
45+
elif sys.platform.startswith("linux"):
46+
target_arch = os.environ.get("targetArch", platform.machine())
47+
libc_name, _ = platform.libc_ver()
48+
is_musl = libc_name and "musl" in libc_name.lower()
49+
if target_arch == "x86_64":
50+
return "x86_64", "musllinux_1_2_x86_64" if is_musl else "manylinux_2_28_x86_64"
51+
elif target_arch in ["aarch64", "arm64"]:
52+
return "aarch64", "musllinux_1_2_aarch64" if is_musl else "manylinux_2_28_aarch64"
53+
else:
54+
raise OSError(f"Unsupported architecture '{target_arch}' for Linux")
55+
raise OSError(f"Unsupported platform: {sys.platform}")
2456

2557

2658
# =============================================================================
2759
# Platform-Specific Package Discovery
2860
# =============================================================================
2961

62+
3063
def get_platform_packages():
3164
"""Get platform-specific package list."""
3265
packages = find_packages()
3366
arch, _ = get_platform_info()
3467

3568
if sys.platform.startswith("win"):
36-
packages.extend([
37-
f"mssql_python.libs.windows.{arch}",
38-
f"mssql_python.libs.windows.{arch}.1033",
39-
f"mssql_python.libs.windows.{arch}.vcredist",
40-
])
69+
packages.extend(
70+
[
71+
f"mssql_python.libs.windows.{arch}",
72+
f"mssql_python.libs.windows.{arch}.1033",
73+
f"mssql_python.libs.windows.{arch}.vcredist",
74+
]
75+
)
4176
elif sys.platform.startswith("darwin"):
4277
packages.append("mssql_python.libs.macos")
4378
elif sys.platform.startswith("linux"):
@@ -50,6 +85,7 @@ def get_platform_packages():
5085
# Custom Distribution (Force Platform-Specific Wheel)
5186
# =============================================================================
5287

88+
5389
class BinaryDistribution(Distribution):
5490
"""Distribution that forces platform-specific wheel creation."""
5591

@@ -61,6 +97,7 @@ def has_ext_modules(self):
6197
# Custom bdist_wheel Command
6298
# =============================================================================
6399

100+
64101
class CustomBdistWheel(bdist_wheel):
65102
"""Custom wheel builder with platform-specific tags."""
66103

@@ -81,10 +118,8 @@ def finalize_options(self):
81118
setup(
82119
# Package discovery
83120
packages=get_platform_packages(),
84-
85121
# Force binary distribution
86122
distclass=BinaryDistribution,
87-
88123
# Register custom commands
89124
cmdclass={
90125
"bdist_wheel": CustomBdistWheel,

0 commit comments

Comments
 (0)