Skip to content

Commit 6477e98

Browse files
leofangleofang
committed
build: per-project cxx_std + tweak injection; fix Andy's review findings
resolve_toolchain gains a required keyword-only ``cxx_std`` argument and an optional ``tweak`` post-hook. The shared ``_build_flags`` no longer picks a default C++ standard — the two backends legitimately differ (bindings stays on c++14 to avoid a c++17 variadic-template regression on kernel-launch paths; core is on c++17), and there is no defensible shared default. See NVIDIA#1882 for the audit. - ``cuda_bindings/build_hooks.py`` adds ``_tweak_flags`` which layers ``-Wno-deprecated-declarations`` on Linux for the 38 warnings ``cudaMemcpy*Array*`` / ``cudaGetDriverEntryPoint`` produce in 13.4 headers. Without it, once NVIDIA#2966 turns on ``-Werror``, bindings would stop building. - Call site passes ``cxx_std=14, tweak=_tweak_flags``. - ``cuda_core/build_hooks.py`` passes ``cxx_std=17`` (no tweak). Also addresses Andy-Jost's line-anchored review comments: - ``TestForceReachesBuildExt._finalized_build_ext`` now patches ``sys.modules["_build_shared"]`` instead of ``build_hooks``. Because ``build_hooks.force_build_ext`` is a module-level ``__getattr__`` fallback, monkey-patching it on ``build_hooks`` creates a real attribute that shadows the fallback and persists across teardown — a poison state that made ``TestBuildConfigStamp`` order-dependent (reproduces under ``pytest -p randomly --randomly-seed=1``). - Restores ``test_flag_set_forces_rebuild`` — the only direct check that ``force_build_ext=True`` reaches ``build_ext.force`` through the ``__getattr__`` re-export. Without it, a later ``from _build_shared import force_build_ext`` would silently snapshot ``False`` and forced rebuilds would stop with no failing test. - Restores ``test_default_preserves_existing_cc`` in ``ResolveToolchainSharedMixin``. A green wheel build wouldn't catch a regression here — if the default toolchain overwrote ``CC``, sccache would silently stop and ``sccache-summary`` only warns. - ``CONTRIBUTING.md`` recovery recipe now runs ``git config core.symlinks true`` (no ``--global``) inside the existing clone. ``git clone`` probes symlink support and writes ``core.symlinks=false`` into the *repo-local* config on failure; repo-local overrides ``--global``, so the previous recipe couldn't fix an existing clone. Test bookkeeping: - Per-package ``TestResolveToolchain.test_llvm_sets_env_and_flags`` restored in both packages — bindings asserts ``-std=c++14`` + ``-Wno-deprecated-declarations``, core asserts ``-std=c++17`` and the absence of ``-Wno-deprecated-declarations``. Removed from the shared mixin because it now asserts package-specific flag content. - Other shared-mixin resolve tests just pick ``cxx_std=17`` as a placeholder — they test env/name/error mechanics, not flag content. Pass counts: bindings 29 (was 28), core 61 (was 59). Randomized-order runs (``-p randomly --randomly-seed={1,2,4,42}``) all green.
1 parent 727f231 commit 6477e98

7 files changed

Lines changed: 111 additions & 40 deletions

File tree

‎CONTRIBUTING.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,10 +162,14 @@ to actually build.
162162

163163
Then clone as usual (see [Cloning the repository](#cloning-the-repository)).
164164

165-
If you already cloned without these settings, run the `git config` command
166-
above, then reset the affected files so Git re-materializes them as symlinks:
165+
If you already cloned without these settings, note that `git clone` probes
166+
symlink support at clone time and writes `core.symlinks=false` into the
167+
repo-local config when the probe fails. Repo-local config overrides
168+
`--global`, so you must clear it *inside the existing clone* — the global
169+
setting alone won't take effect:
167170

168171
```console
172+
$ git config core.symlinks true # no --global — clears the repo-local override
169173
$ git rm --cached cuda_core/_build_shared.py
170174
$ git checkout HEAD -- cuda_core/_build_shared.py
171175
```

‎cuda_bindings/_build_shared.py‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -195,22 +195,21 @@ def _check_toolchain_available(name):
195195
)
196196

197197

198-
def _build_flags(name, debug, compile_for_coverage):
198+
def _build_flags(name, cxx_std, debug, compile_for_coverage):
199199
"""Compile/link flags for a resolved toolchain (shared across backends).
200200
201-
The two backends used to carry slightly different flag sets — bindings had
202-
``-std=c++14``, ``-fpermissive``, ``-fno-var-tracking-assignments``, and
203-
``-Wno-deprecated-declarations``; core had ``-std=c++17`` and no MSVC
204-
``/std:``. See https://github.com/NVIDIA/cuda-python/issues/1882 for the
205-
audit that traced the difference to legacy drift, not intent.
201+
``cxx_std`` is required — the two backends can legitimately differ (bindings
202+
stays on ``c++14`` to avoid a c++17 variadic-template regression on the
203+
kernel-launch code paths; core is on ``c++17``), and there is no defensible
204+
shared default. See https://github.com/NVIDIA/cuda-python/issues/1882.
206205
"""
207206
extra_compile_args = []
208207
extra_link_args = []
209208

210209
if name == "msvc":
211-
extra_compile_args += ["/std:c++17"]
210+
extra_compile_args += [f"/std:c++{cxx_std}"]
212211
else:
213-
extra_compile_args += ["-std=c++17"]
212+
extra_compile_args += [f"-std=c++{cxx_std}"]
214213
if name == "llvm":
215214
extra_link_args += ["-fuse-ld=lld"]
216215
if debug:
@@ -227,19 +226,26 @@ def _build_flags(name, debug, compile_for_coverage):
227226
return extra_compile_args, extra_link_args
228227

229228

230-
def resolve_toolchain(debug=False, compile_for_coverage=False):
229+
def resolve_toolchain(*, cxx_std, debug=False, compile_for_coverage=False, tweak=None):
231230
"""Resolve the C/C++ toolchain from CUDA_PYTHON_TOOLCHAIN.
232231
233232
Returns (name, cc, cxx, extra_compile_args, extra_link_args). The default
234233
toolchain (gnu on Linux, msvc on Windows) does not touch CC/CXX/LDSHARED,
235234
so an externally-set compiler (e.g. CC="sccache cc") keeps working. An
236235
explicit CUDA_PYTHON_TOOLCHAIN (llvm on Linux) sets CC/CXX/LDSHARED to
237236
the toolchain's binaries so distutils' customize_compiler picks them up.
237+
238+
``cxx_std`` is required — each backend chooses its own C++ standard.
239+
``tweak`` is an optional post-hook ``(name, cargs, largs) -> (cargs, largs)``
240+
for package-specific flag layering (e.g. bindings adds
241+
``-Wno-deprecated-declarations``).
238242
"""
239243
name, _allowed, cc, cxx, explicit = _resolve_toolchain_name()
240244
if name == "msvc" and debug:
241245
raise RuntimeError("Debuggable builds are not supported on Windows.")
242-
extra_compile_args, extra_link_args = _build_flags(name, debug, compile_for_coverage)
246+
extra_compile_args, extra_link_args = _build_flags(name, cxx_std, debug, compile_for_coverage)
247+
if tweak is not None:
248+
extra_compile_args, extra_link_args = tweak(name, extra_compile_args, extra_link_args)
243249
_apply_toolchain_env(cc, cxx, explicit)
244250
return name, cc, cxx, extra_compile_args, extra_link_args
245251

‎cuda_bindings/build_hooks.py‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,22 @@
5353
_extensions = None
5454

5555

56+
# -----------------------------------------------------------------------
57+
# Toolchain flag tweaks
58+
59+
60+
def _tweak_flags(name, extra_compile_args, extra_link_args):
61+
"""cuda.bindings-specific flag layering on top of _build_shared._build_flags.
62+
63+
``cudaMemcpy*Array*`` and ``cudaGetDriverEntryPoint`` are deprecated but
64+
still supported; the resulting warnings (~38 in the 13.4 headers) are not
65+
interesting today and would break the build once #2966 lands ``-Werror``.
66+
"""
67+
if name != "msvc":
68+
extra_compile_args = [*extra_compile_args, "-Wno-deprecated-declarations"]
69+
return extra_compile_args, extra_link_args
70+
71+
5672
# -----------------------------------------------------------------------
5773
# Toolchain stamp
5874
#
@@ -160,7 +176,7 @@ def _build_cuda_bindings(debug=False):
160176
# Linux, msvc on Windows) reproduces the previous build behavior and does
161177
# not touch CC/CXX, so an externally-set compiler (e.g. sccache) survives.
162178
toolchain, _cc, _cxx, extra_compile_args, extra_link_args = resolve_toolchain(
163-
debug=debug, compile_for_coverage=compile_for_coverage
179+
cxx_std=14, debug=debug, compile_for_coverage=compile_for_coverage, tweak=_tweak_flags
164180
)
165181
_check_toolchain_available(toolchain)
166182
extra_cythonize_kwargs = {}

‎cuda_bindings/tests/test_build_hooks.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,28 @@ class TestResolveToolchainShared(ResolveToolchainSharedMixin):
138138
build_hooks = build_hooks
139139

140140

141+
class TestResolveToolchain:
142+
"""cuda.bindings-specific ``resolve_toolchain(cxx_std=14, tweak=_tweak_flags)`` assertions."""
143+
144+
@pytest.mark.agent_authored(model="glm-5.2")
145+
def test_llvm_sets_env_and_flags(self, monkeypatch):
146+
if sys.platform == "win32":
147+
pytest.skip("llvm only valid on Linux")
148+
monkeypatch.setenv("CUDA_PYTHON_TOOLCHAIN", "llvm")
149+
monkeypatch.delenv("CC", raising=False)
150+
monkeypatch.delenv("CXX", raising=False)
151+
monkeypatch.delenv("LDSHARED", raising=False)
152+
name, cc, cxx, cargs, largs = build_hooks.resolve_toolchain(cxx_std=14, tweak=build_hooks._tweak_flags)
153+
assert name == "llvm"
154+
assert (cc, cxx) == ("clang", "clang++")
155+
assert "-fuse-ld=lld" in largs
156+
# bindings keeps c++14 (avoids the c++17 variadic-template regression
157+
# on kernel launch) and layers on -Wno-deprecated-declarations via
158+
# _tweak_flags.
159+
assert "-std=c++14" in cargs
160+
assert "-Wno-deprecated-declarations" in cargs
161+
162+
141163
class TestCheckToolchainAvailable(CheckToolchainAvailableSharedMixin):
142164
build_hooks = build_hooks
143165

‎cuda_core/build_hooks.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ def module_names():
220220
# Linux, msvc on Windows) reproduces the previous build behavior and does
221221
# not touch CC/CXX, so an externally-set compiler (e.g. sccache) survives.
222222
toolchain, _cc, _cxx, extra_compile_args, extra_link_args = resolve_toolchain(
223-
debug=debug, compile_for_coverage=COMPILE_FOR_COVERAGE
223+
cxx_std=17, debug=debug, compile_for_coverage=COMPILE_FOR_COVERAGE
224224
)
225225
_check_toolchain_available(toolchain)
226226
extra_cythonize_kwargs = {}

‎cuda_core/tests/test_build_hooks.py‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -388,12 +388,19 @@ def _finalized_build_ext(force_flag, monkeypatch):
388388

389389
setup_py = _load_setup_py(monkeypatch)
390390
assert setup_py.build_hooks is build_hooks
391-
monkeypatch.setattr(build_hooks, "force_build_ext", force_flag)
391+
# Patch _build_shared, not build_hooks: build_hooks.force_build_ext
392+
# comes from a module __getattr__, so monkey-patching it on build_hooks
393+
# creates a real attribute that shadows the __getattr__ fallback and
394+
# persists across teardown, poisoning any later stamp-fixture reset.
395+
monkeypatch.setattr(sys.modules["_build_shared"], "force_build_ext", force_flag)
392396

393397
cmd = setup_py.build_ext(Distribution({"name": "cuda-core", "version": "0"}))
394398
cmd.finalize_options()
395399
return cmd
396400

401+
def test_flag_set_forces_rebuild(self, monkeypatch):
402+
assert self._finalized_build_ext(True, monkeypatch).force
403+
397404
def test_flag_clear_leaves_default(self, monkeypatch):
398405
assert not self._finalized_build_ext(False, monkeypatch).force
399406

@@ -533,6 +540,26 @@ class TestResolveToolchainShared(ResolveToolchainSharedMixin):
533540
build_hooks = build_hooks
534541

535542

543+
class TestResolveToolchain:
544+
"""cuda.core-specific ``resolve_toolchain(cxx_std=17)`` assertions."""
545+
546+
@pytest.mark.agent_authored(model="glm-5.2")
547+
def test_llvm_sets_env_and_flags(self, monkeypatch):
548+
if sys.platform == "win32":
549+
pytest.skip("llvm only valid on Linux")
550+
monkeypatch.setenv("CUDA_PYTHON_TOOLCHAIN", "llvm")
551+
monkeypatch.delenv("CC", raising=False)
552+
monkeypatch.delenv("CXX", raising=False)
553+
monkeypatch.delenv("LDSHARED", raising=False)
554+
name, cc, cxx, cargs, largs = build_hooks.resolve_toolchain(cxx_std=17)
555+
assert name == "llvm"
556+
assert (cc, cxx) == ("clang", "clang++")
557+
assert "-fuse-ld=lld" in largs
558+
assert "-std=c++17" in cargs
559+
# -Wno-deprecated-declarations is bindings-only.
560+
assert "-Wno-deprecated-declarations" not in cargs
561+
562+
536563
class TestCheckToolchainAvailable(CheckToolchainAvailableSharedMixin):
537564
build_hooks = build_hooks
538565

‎cuda_python_test_helpers/cuda_python_test_helpers/build_shared.py‎

Lines changed: 21 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -26,31 +26,43 @@
2626

2727

2828
class ResolveToolchainSharedMixin:
29-
"""Common ``resolve_toolchain`` assertions covering the shared flag policy.
29+
"""Common ``resolve_toolchain`` assertions that don't depend on the flag set.
3030
3131
Subclasses set ``build_hooks`` (the loaded build_hooks module). The
32-
default-toolchain happy paths (env not touched, external CC preserved,
33-
gnu compile) are exercised by every green Linux wheel build in CI, so
34-
they aren't asserted here. Only behavior that a passing wheel build
35-
would not surface is kept: error paths, the llvm-override case, and the
36-
llvm flag set (llvm has no CI, so it's not covered by wheels either).
32+
``cxx_std`` value passed here is arbitrary — these tests exercise the
33+
env/name/error mechanics, not flag content. Per-package tests assert the
34+
specific standard each backend chooses (``c++14`` for bindings, ``c++17``
35+
for core) and the presence of any package-specific tweaks.
3736
"""
3837

3938
build_hooks = None
4039

40+
@pytest.mark.agent_authored(model="glm-5.2")
41+
def test_default_preserves_existing_cc(self, monkeypatch):
42+
# An externally-set CC (e.g. sccache) must survive the default toolchain.
43+
# A green wheel build alone wouldn't catch a regression here: if the
44+
# default toolchain silently overwrote CC, the build would still pass —
45+
# only sccache would stop working, and sccache-summary only warns.
46+
monkeypatch.delenv("CUDA_PYTHON_TOOLCHAIN", raising=False)
47+
monkeypatch.setenv("CC", "sccache cc")
48+
monkeypatch.setenv("CXX", "sccache c++")
49+
self.build_hooks.resolve_toolchain(cxx_std=17)
50+
assert os.environ["CC"] == "sccache cc"
51+
assert os.environ["CXX"] == "sccache c++"
52+
4153
@pytest.mark.agent_authored(model="glm-5.2")
4254
def test_case_insensitive(self, monkeypatch):
4355
if sys.platform == "win32":
4456
pytest.skip("llvm only valid on Linux")
4557
monkeypatch.setenv("CUDA_PYTHON_TOOLCHAIN", "LLVM")
46-
name, _cc, _cxx, _cargs, _largs = self.build_hooks.resolve_toolchain()
58+
name, _cc, _cxx, _cargs, _largs = self.build_hooks.resolve_toolchain(cxx_std=17)
4759
assert name == "llvm"
4860

4961
@pytest.mark.agent_authored(model="glm-5.2")
5062
def test_invalid_value_raises(self, monkeypatch):
5163
monkeypatch.setenv("CUDA_PYTHON_TOOLCHAIN", "icc")
5264
with pytest.raises(RuntimeError, match="not supported"):
53-
self.build_hooks.resolve_toolchain()
65+
self.build_hooks.resolve_toolchain(cxx_std=17)
5466

5567
@pytest.mark.agent_authored(model="glm-5.2")
5668
def test_llvm_overrides_external_cc(self, monkeypatch):
@@ -60,24 +72,8 @@ def test_llvm_overrides_external_cc(self, monkeypatch):
6072
# external CC (e.g. "sccache cc") is replaced, not kept.
6173
monkeypatch.setenv("CUDA_PYTHON_TOOLCHAIN", "llvm")
6274
monkeypatch.setenv("CC", "sccache cc")
63-
_name, _cc, _cxx, _cargs, _largs = self.build_hooks.resolve_toolchain()
64-
assert os.environ["CC"] == "clang"
65-
66-
@pytest.mark.agent_authored(model="glm-5.2")
67-
def test_llvm_sets_env_and_flags(self, monkeypatch):
68-
if sys.platform == "win32":
69-
pytest.skip("llvm only valid on Linux")
70-
monkeypatch.setenv("CUDA_PYTHON_TOOLCHAIN", "llvm")
71-
monkeypatch.delenv("CC", raising=False)
72-
monkeypatch.delenv("CXX", raising=False)
73-
monkeypatch.delenv("LDSHARED", raising=False)
74-
name, cc, cxx, cargs, largs = self.build_hooks.resolve_toolchain()
75-
assert name == "llvm"
76-
assert (cc, cxx) == ("clang", "clang++")
75+
self.build_hooks.resolve_toolchain(cxx_std=17)
7776
assert os.environ["CC"] == "clang"
78-
assert os.environ["CXX"] == "clang++"
79-
assert "-fuse-ld=lld" in largs
80-
assert "-std=c++17" in cargs
8177

8278

8379
class CheckToolchainAvailableSharedMixin:

0 commit comments

Comments
 (0)