You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Bump toolchains_llvm to 1.11.2, drop #791 archive_override - #423
Bumps toolchains_llvm from 1.8.0 to 1.11.2 in MODULE.bazel.
Drops the archive_override pinning an unreleased commit (merge of upstream #791, which dropped an unused -stdlib=libc++ flag that broke this project's Linux -Werror builds). That fix shipped in the 1.9.0 release — verified by diffing cc_toolchain_config.bzl at that tag against the pinned commit (identical) — so a plain bazel_dep pin replaces it.
Confirmed llvm_versions = "21.1.8" (what this project pins for all three platforms) is still a catalogued archive at 1.11.2 (toolchain/distributions/github.jsonc).
Deliberately not adopted here: 1.11.0 introduces a linker = {...} catalogue attribute (e.g. "darwin-aarch64": "auto") that's a more idiomatic way to express exactly the override this project hand-rolled with extra_link_flags — the release notes even call out "LLVM 23 on macOS still needs the native auto linker override for the known SDK/TAPI issue," which is our exact arm64e.x1 problem. Migrating to it is a worthwhile follow-up, but it's a mechanism change, not a side effect of a version bump, so I left it out of scope here.
Updated ci_msan_test's guard: it now checks the bazel_dep version directly and that no override targets toolchains_llvm, using the same full-call-body regex approach as the apple_support guard in Bump apple_support to 2.10.1, drop Xcode 27 crosstool patches #421 (avoids the "module_name must be the next line" fragility flagged there).
Test plan
Run on this macOS 27 / Xcode 27 host — exactly the scenario the original native-linker fix targets:
bazelisk test //python:ci_msan_test — red against the pre-bump 1.8.0 + override state (verified via git stash), green after
bazelisk test //... — 104/104 pass
bazelisk test --config=asan //library/tests/system/... — 22/22 pass
bazelisk test --config=tsan and --config=ubsan //library/tests/system:worker_context_reuse_test — pass
--config=msan is Linux-only and can't run on this host; ci_msan_test's config-wiring checks pass, and a --nobuild analysis probe of its toolchain fails identically before and after this bump (pre-existing macOS-host limitation, not a regression) — real msan verification happens on Linux CI for this PR
toolchains_llvm 1.8.0 emitted an unused -stdlib=libc++ under -nostdinc++,
breaking this project's Linux -Werror builds; the project carried an
archive_override pinned to the unreleased merge commit of upstream PR #791
to work around it. That fix shipped in the 1.9.0 release (verified by
diffing cc_toolchain_config.bzl at that tag against the pinned commit —
identical), so pin to 1.11.2 directly and drop the override.
Confirmed llvm_versions = "21.1.8" is still a catalogued archive at 1.11.2
(toolchain/distributions/github.jsonc), and that the extra_link_flags /
extra_link_libs mechanism this project relies on for the macOS 27 SDK /
Linux native-linker overrides is unchanged (cc_toolchain_config.bzl still
appends them after the toolchain-default --ld-path, so "last wins" still
holds). 1.11.0's new linker = {...} catalogue attribute is not adopted here
-- it's a more idiomatic way to express the same override going forward,
but switching to it is a separate, deliberate migration, not a side effect
of this version bump.
ci_msan_test's guard for the override is replaced with one that checks the
bazel_dep version and that no archive/single_version/git_override targets
toolchains_llvm, using the same full-call-body matching approach as the
apple_support guard (see PR #421) instead of a "next line" regex.
Verified on this macOS 27 / Xcode 27 host, which is exactly the scenario
the original fix targets:
- bazelisk test //... : 104/104 pass
- bazelisk test --config=asan //library/tests/system/... : 22/22 pass
- bazelisk test --config=tsan and --config=ubsan //library/tests/system:worker_context_reuse_test: pass
- msan is Linux-only and can't run on this host; confirmed the Bazel config
wiring test (ci_msan_test) passes, and that an unrelated
--nobuild analysis probe of its toolchain fails identically before and
after this bump (pre-existing macOS-host limitation, not a regression).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ains_llvm guard.
_has_toolchains_llvm_override()'s alternation only matched archive_override,
single_version_override, and git_override -- Bazel's other two module
override directives, local_path_override and multiple_version_override,
silently returned False, so either could reintroduce an override without
the guard noticing. Added both to the alternation and added a test for
each; confirmed both new tests fail against the pre-fix regex (verified
with a standalone repro) before adding the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addressed in 0c9ec92: added `local_path_override` and `multiple_version_override` to the alternation in `_has_toolchains_llvm_override()`, plus a test for each. Confirmed both new tests fail against the pre-fix regex (standalone repro) before adding the fix.
This same gap exists in the identical `_has_apple_support_override()` helper added in #421 — fixing that one too to avoid the same finding there.
Parse bazel_dep fields independently of argument order
python/tests/ci_msan_test.py:30
This parser only recognizes name immediately followed by version. Reordering or multiline formatting the valid bazel_dep arguments makes this guard fail even though the dependency remains correctly pinned; the analogous helper in ci_windows_cppopts_test.py:169-176 parses the whole call body to avoid this. Parse each full bazel_dep(...) body and find both fields independently (and add a reordered-arguments case).
…rder.
_toolchains_llvm_bazel_dep_version() only matched "name" immediately
followed by "version", so reordered or multiline bazel_dep(...) arguments
would fail the guard even with the dependency correctly pinned. Matches
the existing _rules_cc_bazel_dep_version() approach in
ci_windows_cppopts_test.py: parse the whole call body and find both
fields independently. Confirmed the new reordered-args test fails against
the old pattern (standalone repro) before the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addressed in ccf2a1f: `_toolchains_llvm_bazel_dep_version()` now parses the whole `bazel_dep(...)` call body and finds `name`/`version` independently, matching `_rules_cc_bazel_dep_version()` in `ci_windows_cppopts_test.py`. Added a reordered-args test; confirmed it fails against the old pattern (standalone repro) before the fix.
Same gap review caught on the toolchains_llvm guard in #423: the inline
regex only matched "name" immediately followed by "version", so reordered
or multiline bazel_dep(...) arguments would fail the guard even with the
dependency correctly pinned. Extracted _apple_support_bazel_dep_version(),
parsing the whole call body and finding both fields independently, matching
_rules_cc_bazel_dep_version() in ci_windows_cppopts_test.py. Confirmed the
new reordered-args test fails against the old inline pattern (standalone
repro) before the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The [^)]*)\) regex stops at the first ")" even when it appears inside a
string literal, a "#" comment, or a nested call -- a leading comment like
"# see upstream fix (closes #791)" before the real args would truncate the
match and make _toolchains_llvm_bazel_dep_version()/
_has_toolchains_llvm_override() fail or return False despite the file being
correct. _iter_call_bodies() replaces both regexes with a character scanner
that tracks string/comment state and paren depth, so the body always spans
to its true matching ")". Added a test with a ")" in a leading comment for
each helper; confirmed both fail against the old regex (standalone repro)
before the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addressed in 1a817e8. Both `_toolchains_llvm_bazel_dep_version()` and `_has_toolchains_llvm_override()` now use `_iter_call_bodies()`, a character-by-character scanner that tracks string-literal/comment state and paren depth, instead of `[^)]*)\)`. Added a test for each with a `)` inside a leading comment (e.g. `# see upstream fix (closes #791)`) before the real args; confirmed both fail against the old regex (standalone repro) before the fix.
24/24 `//python/...` tests pass.
Note: the same `[^)]*)\)` pattern — including in the `_rules_cc_bazel_dep_version`/`_rules_cc_override_version` helpers in `ci_windows_cppopts_test.py` that earlier rounds pointed to as the model to match — has this identical truncation issue. That file is outside this PR's diff, so I haven't touched it; flagging in case you want it fixed too, either here or separately.
Same bug review found on the toolchains_llvm guard in #423: [^)]*)\)
stops at the first ")" even inside a string, comment, or nested call, so
a leading comment like "# fixes Xcode 27 crosstool warnings (see docs)"
before the real args would truncate the match. _iter_call_bodies()
replaces both regexes with a character scanner tracking string/comment
state and paren depth. Added a test with a ")" in a leading comment for
each helper; confirmed both fail against the old regex (standalone repro)
before the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This pattern also matches call text inside comments, so the guard can inspect declarations that Bazel ignores. For example, a commented bazel_dep(... version = "9.9.9") before the real 1.8.0 dependency makes the minimum-version assertion pass, while a commented override makes _has_toolchains_llvm_override return true. Restrict matches to top-level, non-comment lines (and add a commented-call case) so this CI guard reflects the effective MODULE configuration.
_iter_call_bodies() located each call with a plain regex search for
"name(", which matches that text inside a "#" comment just as readily as
live code -- a commented-out "bazel_dep(... version = \"9.9.9\")" before
the real dependency made the version guard read the dead value, and a
commented-out override made _has_toolchains_llvm_override return a false
positive. Rewritten as a single linear scan that tracks string/comment
state continuously, so a call name is only recognized outside both.
Confirmed both failure modes against the old scanner (standalone repro)
before the fix, and added a dedicated commented-out-call test for each
helper.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addressed in 58d3db7. `_iter_call_bodies()` previously located each call via a plain regex search for `name(`, which matches that text inside a `#` comment just as readily as live code — confirmed both failure modes with a standalone repro: a commented-out `bazel_dep(... version = "9.9.9")` before the real dependency made the version guard read the dead value, and a commented-out override made `_has_toolchains_llvm_override` return a false positive.
Rewrote it as a single linear scan that tracks string/comment state continuously across the whole text, so a call name is only recognized outside both. Added a dedicated commented-out-call test for each helper.
24/24 `//python/...` tests pass. Will mirror this to #424 as well.
Same bug fixed on the toolchains_llvm guard in #423: _iter_call_bodies()
located each call with a plain regex search for "name(", which matches
that text inside a "#" comment too -- a commented-out "bazel_dep(...
version = \"9.9.9\")" before the real dependency made the version guard
read the dead value, and a commented-out override made
_has_apple_support_override return a false positive. Rewritten as a
single linear scan that tracks string/comment state continuously.
Confirmed both failure modes against the old scanner (standalone repro)
before the fix, and added a dedicated commented-out-call test for each
helper.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The override scanner misses valid nested callable grouping, allowing the prohibited override to bypass its guard.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
Recursively unwrap grouped callables when detecting overrides
python/tests/ci_msan_test.py:353
This handles only one layer of callable grouping. Valid Starlark such as ((archive_override))(module_name = "toolchains_llvm", ...) therefore produces no match, so _has_toolchains_llvm_override() returns False and the new guard allows the override it is intended to reject. Recursively unwrap grouped callables (or use a Starlark parser) rather than special-casing exactly (name)(...).
Three fixes:
1. (#423's own flagged finding) _match_parenthesized_call() only
handled exactly one layer of "(" name ")(" grouping, so
"((archive_override))(module_name = \"toolchains_llvm\", ...)" --
valid Starlark, since grouping parens nest arbitrarily -- didn't
match and let the override bypass the guard. Rewrote it to count an
arbitrary number of leading "(" and require the same number of
matching ")", rather than special-casing exactly one layer.
2. (mirrored from #424) _skip_raw_string() decided whether a quote
ended the string by looking at only the single preceding character,
so an even-length run of backslashes before the closing quote (e.g.
r"foo\\") was misread as escaping it, causing the scanner to run
past the real end of the string and corrupt the rest of the parse.
Rewrote it to count the full backslash run and use its parity,
matching real Starlark/Python raw-string semantics (confirmed
against CPython's own r-string evaluation): an odd run escapes the
quote (string continues, backslashes and quote both kept literally
in the value), an even run does not.
3. (mirrored from #424) bazel_dep allows both name and version
positionally (bazel_dep("toolchains_llvm", "1.11.2")), but the
scanner only ever recorded the first positional argument, so a
positional version made _toolchains_llvm_bazel_dep_version() raise
instead of returning it. Replaced the single first-positional slot
with an ordered list (_add_positional/_positional), read by index,
so any caller can look at any positional argument, not just the
first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addressed the latest Copilot finding on this PR: nested grouped-callable parens.
`_match_parenthesized_call()` only handled exactly one layer of `(` name `)(` grouping, so `((archive_override))(module_name = "toolchains_llvm", ...)` — valid Starlark, since grouping parens nest arbitrarily and the result can still be called immediately — didn't match and let the override bypass the guard. Rewrote it to count an arbitrary number of leading `(` and require the same number of matching `)`, rather than special-casing exactly one layer.
Also proactively mirrored #424's two current findings into this file (same parser):
A quote's "is it escaped" decision in `_skip_raw_string()` only looked at the single preceding character, so an even-length run of backslashes before the closing quote (e.g. `r"foo\\"`) was misread as escaping it, running past the real end of the string and corrupting the rest of the parse. Rewrote it to count the full backslash run and use its parity — confirmed against CPython's own raw-string evaluation for several cases, including that the backslash itself stays in the value (not just the quote), which my earlier single-char version got wrong even for the simple case.
`bazel_dep` allows `version` positionally too (`bazel_dep("toolchains_llvm", "1.11.2")`), but the scanner only ever recorded the first positional argument. Replaced the single first-positional slot with an ordered list read by index, so any caller can look at any positional argument.
#423's own finding: _skip_ws() only recognized the bare whitespace
characters " \t\r\n", not a backslash-newline line continuation. A
valid Starlark call like `archive_override\` + newline + `(...)` was
therefore never recognized -- the backslash after the call name
wasn't whitespace, so the "(" lookup failed and
_has_toolchains_llvm_override silently returned False even when
module_name = "toolchains_llvm", letting a real override slip past
the regression guard. Fixed by having _skip_ws() treat a backslash
immediately followed by LF or CRLF as whitespace too, for both line
ending forms.
Mirrored from #424 (same parser):
- The outer top-level scan recognized only ordinary string literals
(checking for a leading quote character, then _skip_string()), so a
raw string like `r"C:\U12345678\emsdk"` outside any recognized call
had its "r" prefix treated as an unrelated bare identifier and its
body fed to the ordinary escape decoder, which raises on sequences
that are only valid because of the "r" prefix. Fixed by using the
same _is_raw_string_start()/_skip_any_string() dispatch already used
inside a call body.
- A comment between a keyword and "=" (e.g. `module_name # note\n =
"emsdk"`) is legal Starlark, but the keyword-matching code used
_skip_ws() right after the identifier, which stops at "#", so the
keyword was misread as a bare positional identifier -- failing
closed and incorrectly tripping the guard for an unrelated module's
override, and raising AssertionError out of
_toolchains_llvm_bazel_dep_version when the misread identifier was a
real bazel_dep's "name". Fixed by using _skip_ws_and_comments()
there instead, matching how comments are already skipped between
"=" and the right-hand side.
All three reproduced against throwaway scripts before the fix and
added as permanent regression tests; full //python/... suite green
(24/24).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Handle backslash-newline continuations in override detection: _skip_ws() only recognized the bare whitespace characters " \t\r\n", not a backslash-newline line continuation. A valid Starlark call like archive_override\ + newline + (module_name = "toolchains_llvm", ...) was therefore never recognized — the backslash right after the call name wasn't whitespace, so the lookup for ( failed and _has_toolchains_llvm_override silently returned False even though module_name = "toolchains_llvm", letting a real override slip past the regression guard. Confirmed for both LF and CRLF continuations before fixing. Fixed by having _skip_ws() treat a backslash immediately followed by LF or CRLF as whitespace too (both line-ending forms), with regression tests for both.
Also proactively mirrored #424's two findings from this same review round (same parser):
The outer top-level scan didn't use the raw-string-aware dispatcher, so a raw string literal outside any recognized call (e.g. SDK_PATH = r"C:\U12345678\emsdk") crashed the guard instead of being skipped correctly.
A comment between a keyword and = (e.g. module_name # note\n = "emsdk") was misread as a bare positional identifier, causing false-positive overrides and a spurious AssertionError in the version helper.
All three reproduced against throwaway scripts before the fix. Full //python/... suite green (24/24).
Detect overrides with comments before the argument list
python/tests/ci_msan_test.py:411
A comment between a callable and its argument list is legal inside a grouped expression, for example (local_path_override # note\n(module_name = "toolchains_llvm", path = "../toolchains_llvm")). Here _skip_ws stops at #, and _match_parenthesized_call does not recognize this layout either. The override detector returns False, so the no-override guard passes despite an actual override. Use _skip_ws_and_comments here and add a regression test for this layout.
…alls.
Three scanner defects, all reported by Copilot review (one of them
previously fixed on #423 but missed here; mirroring it back now):
- _skip_ws() only recognized the bare whitespace characters " \t\r\n",
not a backslash-newline line continuation, so a valid call like
`archive_override\` + newline + `(module_name = "apple_support",
...)` was never recognized -- the backslash right after the call
name wasn't whitespace, so the lookup for "(" failed and
_has_apple_support_override silently returned False even with a
real override present. Fixed by having _skip_ws() treat a backslash
immediately followed by LF or CRLF as whitespace too.
- The direct call-name match (immediately before the "(" lookup) used
_skip_ws() rather than _skip_ws_and_comments(), so a comment between
the name and its own "(" -- e.g. `(local_path_override # note\n
(module_name = "apple_support", ...))`, a whole call wrapped in
redundant grouping parens -- was missed, even though the grouped
"(name)(...)" *indirection* form already tolerated such comments.
Fixed by extracting the direct-match and grouped-indirection logic
into one _match_call_name() helper (replacing the former inline
duplicate), used uniformly by both the outer scan and the new nested
scan below, so both forms get the same comment handling for free.
- The body-scanning loop consumed a whole call's argument list via
paren/bracket/brace balancing only, so a recognized call *nested* in
an argument value -- e.g. `patches = single_version_override(...)
or []` inside another override's call -- was swallowed as inert
text instead of being recognized as the real call it is. Fixed by
extracting the body-scanning loop into _scan_call_body(), which now
also calls _match_call_name() at every position in the body (not
just the call's own name before entry); a match there is scanned
recursively and its own {keyword: value} dict is appended to the
shared results list before resuming the outer call's own scan past
the nested call's span.
All three reproduced against the exact Copilot-supplied examples
before the fix, and added as permanent regression tests. Full
//python/... suite green (24/24).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both reported by Copilot review on #424 (same parser):
- The direct call-name match (immediately before the "(" lookup) used
_skip_ws() rather than _skip_ws_and_comments(), so a comment between
the name and its own "(" -- e.g. `(local_path_override # note\n
(module_name = "toolchains_llvm", ...))`, a whole call wrapped in
redundant grouping parens -- was missed, even though the grouped
"(name)(...)" *indirection* form already tolerated such comments.
Fixed by extracting the direct-match and grouped-indirection logic
into one _match_call_name() helper (replacing the former inline
duplicate), used uniformly by both the outer scan and the new nested
scan below, so both forms get the same comment handling for free.
- The body-scanning loop consumed a whole call's argument list via
paren/bracket/brace balancing only, so a recognized call *nested* in
an argument value -- e.g. `patches = single_version_override(...)
or []` inside another override's call -- was swallowed as inert
text instead of being recognized as the real call it is. Fixed by
extracting the body-scanning loop into _scan_call_body(), which now
also calls _match_call_name() at every position in the body (not
just the call's own name before entry); a match there is scanned
recursively and its own {keyword: value} dict is appended to the
shared results list before resuming the outer call's own scan past
the nested call's span.
Both reproduced against the exact Copilot-supplied examples before
the fix, and added as permanent regression tests. Full //python/...
suite green (24/24).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixed the finding from this round, plus proactively mirrored two more from #424's parallel round (same parser, same code shape):
Detect overrides with comments before the argument list (Bump toolchains_llvm to 1.11.2, drop #791 archive_override #423's own finding): the direct call-name match used _skip_ws() instead of _skip_ws_and_comments(), so (local_path_override # note\n(module_name = "toolchains_llvm", ...)) — a whole call wrapped in redundant grouping parens — was missed (confirmed with the exact example). Fixed by extracting the direct-match and grouped-indirection logic into one _match_call_name() helper used uniformly everywhere.
Nested override calls are omitted from scanner results (mirrored from Cover all override directives in the apple_support CI guard #424): the body-scanning loop only paren/bracket/brace-balanced a call's argument list, so a recognized call nested in an argument value (e.g. patches = single_version_override(...) or []) was swallowed as inert text. Fixed by extracting the body scan into _scan_call_body(), which also matches a nested call at every position in the body and recursively records its own kwargs.
Both reproduced against the exact reported/equivalent examples before fixing, with permanent regression tests added. Full //python/... suite green (24/24).
On Fri, Oct 16, 2026 at 9:12 AM Martin Nygren ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In MODULE.bazel
<#423 (comment)>:
> @@ -24,9 +24,7 @@ bazel_dep(name = "googletest", version = "1.17.0.bcr.2")
bazel_dep(name = "pybind11_bazel", version = "3.0.1")
bazel_dep(name = "rules_python", version = "2.0.1")
# 1.8.0+ adds --features=msan and libcxx_url for an instrumented libc++ overlay.
-# BCR 1.8.0 still emits unused `-stdlib=libc++` under `-nostdinc++`, which fails
-# our Linux `-Werror` builds. Override to the #791 fix until BCR publishes >1.8.0.
-bazel_dep(name = "toolchains_llvm", version = "1.8.0")
+bazel_dep(name = "toolchains_llvm", version = "1.11.2")
Feels strange that this change triggered over 1400 lines of unit tests
code.
—
Reply to this email directly, view it on GitHub
<#423?email_source=notifications&email_token=ABC4PYCUURQPNVD7BFB5TDD5UINEXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNJUHAZDGNRSGU22M4TFMFZW63VQOJSXM2LFO5PXEZLROVSXG5DFMSSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5548236255>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABC4PYHWK5PAUBMLV3BE7TD5UINEXAVCNFSNUABEKJSXA33TNF2G64TZHMZDMOJYHAZDANR3JFZXG5LFHM2TOOJSGE3TCMZYHGQXMAQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
Same shared parser, same two defects Copilot reported on #424:
- "ext. bazel_dep(...)" / "ext. archive_override(...)" (or a
continuation or comment after the ".") was read as a real directive,
since only the character immediately before the name was checked.
Both scans now consume the attribute name after any "."
(_skip_attribute); a "." inside a comment is never seen.
- Tokens inside a keyword's right-hand side could be recorded as
positional arguments, so `repo_name = "toolchains_llvm_" + "9.9.9"`
supplied a fake version to a versionless bazel_dep. The body scan now
tracks top-level "," boundaries and classifies each argument once,
by its first token.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_has_apple_support_override() only matched single_version_override --
archive_override, git_override, local_path_override, and
multiple_version_override all silently returned False. Same gap flagged
by review on the equivalent toolchains_llvm guard in dds-bridge#423. Added the
remaining four directives to the alternation and a test for each;
confirmed each new test fails against the pre-fix pattern (standalone
repro) before adding the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
wopdevries
pushed a commit
to wopdevries/dds
that referenced
this pull request
Oct 17, 2026
Follow-up to dds-bridge#423, mirroring dds-bridge#424. Starlark's syntax is a subset of
Python's, so ast.parse handles strings, comments, continuations and
statement boundaries exactly. This fixes the scanner joining
"dep_function = bazel_dep" and a following "(...)" tuple into a
fictitious call, and replaces ~1000 lines of lexer and its
tokenizer-level tests with ~70 lines.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Clean Copilot reviewCopilot reviewed and had neither new comments nor new suppressed comments.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tameware
left a comment
Summary
toolchains_llvmfrom 1.8.0 to 1.11.2 inMODULE.bazel.archive_overridepinning an unreleased commit (merge of upstream #791, which dropped an unused-stdlib=libc++flag that broke this project's Linux-Werrorbuilds). That fix shipped in the 1.9.0 release — verified by diffingcc_toolchain_config.bzlat that tag against the pinned commit (identical) — so a plainbazel_deppin replaces it.llvm_versions = "21.1.8"(what this project pins for all three platforms) is still a catalogued archive at 1.11.2 (toolchain/distributions/github.jsonc).extra_link_flags/extra_link_libsmechanism this project relies on for the macOS 27 SDK / Linux native-linker overrides (see Release v3.1.1: fix macOS 27 SDK / Xcode 27 build break #417/Guard and document the macOS/Linux native-linker override and ThinLTO #418) is unchanged incc_toolchain_config.bzl— still appended after the toolchain-default--ld-path, so "last flag wins" still holds.linker = {...}catalogue attribute (e.g."darwin-aarch64": "auto") that's a more idiomatic way to express exactly the override this project hand-rolled withextra_link_flags— the release notes even call out "LLVM 23 on macOS still needs the nativeautolinker override for the known SDK/TAPI issue," which is our exactarm64e.x1problem. Migrating to it is a worthwhile follow-up, but it's a mechanism change, not a side effect of a version bump, so I left it out of scope here.ci_msan_test's guard: it now checks thebazel_depversion directly and that no override targetstoolchains_llvm, using the same full-call-body regex approach as theapple_supportguard in Bump apple_support to 2.10.1, drop Xcode 27 crosstool patches #421 (avoids the "module_name must be the next line" fragility flagged there).Test plan
Run on this macOS 27 / Xcode 27 host — exactly the scenario the original native-linker fix targets:
bazelisk test //python:ci_msan_test— red against the pre-bump 1.8.0 + override state (verified viagit stash), green afterbazelisk test //...— 104/104 passbazelisk test --config=asan //library/tests/system/...— 22/22 passbazelisk test --config=tsanand--config=ubsan //library/tests/system:worker_context_reuse_test— pass--config=msanis Linux-only and can't run on this host;ci_msan_test's config-wiring checks pass, and a--nobuildanalysis probe of its toolchain fails identically before and after this bump (pre-existing macOS-host limitation, not a regression) — real msan verification happens on Linux CI for this PR🤖 Generated with Claude Code