Skip to content

refactor(bazel): share the platform-independent toolchain config - #3830

Closed
Alexandr-Solovev wants to merge 2 commits into
uxlfoundation:mainfrom
Alexandr-Solovev:dev/asolovev_bazel_toolchain_unify
Closed

Alexandr-Solovev wants to merge 2 commits into
uxlfoundation:mainfrom
Alexandr-Solovev:dev/asolovev_bazel_toolchain_unify

Conversation

@Alexandr-Solovev

Copy link
Copy Markdown
Contributor

Description

Fourth step of the Bazel maintainability series. Addresses the "Toolchain code
unification"
item that has been open in dev/bazel/TODO.md since 2023:

There is logic duplication for toolchain configuration on Linux/Windows.

Stacked on #3829 (shares its load() edits); merge that one first.

What is actually duplicated, and what is not

cc_toolchain_config_lnx.bzl and cc_toolchain_config_win.bzl are 2,177 lines
describing two different compiler drivers. Most of that is not duplication: of
the 30 features both files define under the same name, none were textually
identical, because they differ in the flags they emit — -I against /I,
-include against /FI, -MD -MF against /clang:-MD /clang:-MF.

What was duplicated is the part that contains no flag syntax at all:

  • the action groups — all_compile_actions, all_cpp_compile_actions,
    all_link_actions, lto_index_actions: plain lists of ACTION_NAMES, byte
    for byte the same in both files;
  • ten features and action configs whose flags come entirely from rule
    attributes or from Bazel build variables:
    cpp_link_static_library, supports_dynamic_linker,
    do_not_link_dynamic_dependencies, compiler_input_flags,
    linker_param_file, user_compile_flags, user_link_flags,
    default_link_flags, default_dynamic_libraries and the per-ISA
    <cpu>_flags;
  • the 28 rule attributes both cc_toolchain_config rules declare, in the
    same order with the same types.

Those move to cc_toolchain_config_common.bzl. The per-compiler features stay
where they are: a shared factory with a if is_windows branch inside it would
be harder to follow than the duplicate it replaces, and the module docstring
says so, so the next person does not "finish the job" by merging the rest.

preprocessor_compile_actions and codegen_compile_actions were defined in
both files and referenced in neither; they are dropped rather than moved.

410 lines leave the two platform files; 341 arrive in the shared one, about a
third of which is the documentation the originals never had.

Validation

  • Every moved block was diffed against both originals after normalising
    whitespace and substituting the factory parameters back to the ctx.attr.*
    expressions they replace — 20 comparisons, all identical.
  • bazel build //cpp/daal:core_static //cpp/oneapi/dal/algo:kmeans_dpc after
    the change is a full action-cache hit (1,572 actions, 0 re-executed),
    i.e. not one compile or link command line moved on Linux.
  • bazel build --nobuild //:release //:release_all //examples/oneapi/cpp:all
    analyses clean.

Windows is covered by the WindowsBazel jobs on this PR; the shared blocks are
textually identical to what that toolchain used before.


  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly hard-to-understand areas.
  • I have updated the documentation or contribution guidelines as needed.
  • I have added tests that reproduce the issue or verify the new functionality.
  • All new and existing tests pass.

🤖 Generated with Claude Code

Alexandr-Solovev and others added 2 commits September 30, 2026 01:17
Third step of the Bazel maintainability series. No behavioural change; the
Linux build produces the same actions (verified by a full action-cache hit).

* `dev/bazel/toolchains/extra_toolchian_lnx.tpl.BUILD` -> `extra_toolchain_lnx`.
  The typo was load-bearing: the label in `extra_toolchain_lnx.bzl` spelled it
  the same way, so it worked, but the file did not turn up when grepping for
  the toolchain by name.
* `_copy_dynamic_release_file` in `cc.bzl` had a `cp` branch for non-Windows
  hosts, and the only two call sites are inside `if is_windows:` and pass
  `is_windows = is_windows`. Renamed to `_copy_windows_release_file`, dropped
  the unreachable branch and the flag, and documented why a copy is used
  instead of a symlink.
* `_copy_to_release_impl` in `release.bzl` resolved the `extra` toolchain into
  a local it never read; the rule does not use the toolchain at all, so the
  declaration goes too. `daal.bzl` remains its only consumer.
* Dropped seven load symbols that are not referenced in the file that loads
  them.
* `.bazelrc` opened with an argument-less `common` line, left behind by the
  Bazel 8 upgrade (uxlfoundation#3035) when the flag it was meant to carry ended up on the
  `build` line instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cc_toolchain_config_lnx.bzl` and `cc_toolchain_config_win.bzl` are two
descriptions of two different compiler drivers, so most of their features
genuinely differ -- `-I` against `/I`, `-MD -MF` against `/clang:-MD
/clang:-MF`. What did not differ was copied: the action groups, ten features
whose flags come entirely from rule attributes or Bazel build variables, and
the 28 rule attributes both rules declare.

Move those to `cc_toolchain_config_common.bzl` and have both files load them.
Nothing changes in the generated command lines: building after the change is a
full action-cache hit, and every moved block is textually identical to the two
originals it replaces, modulo whitespace and the parameter names that stand in
for `ctx.attr.*`.

`preprocessor_compile_actions` and `codegen_compile_actions` were defined in
both files and referenced in neither, so they are dropped instead of moved.

Addresses the "Toolchain code unification" item in dev/bazel/TODO.md. The
per-compiler features stay where they are; a shared factory with a
per-platform branch inside would be harder to follow than the duplicate it
replaces.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The shared configuration affects both platform toolchains, so cross-platform behavior warrants final human review.

Review effort: Balanced
Findings: None

What changed in this PR

This PR consolidates platform-independent Bazel C++ toolchain configuration while keeping Linux- and Windows-specific compiler flags separate. It also carries cleanup from the preceding PR, #3829.

Changes:

  • Moves shared action groups, features, and rule attributes into a common toolchain module.
  • Renames the Linux extra-toolchain template and removes unused imports, code, and configuration whitespace.
File Description
dev/​bazel/​toolchains/​extra_toolchain_lnx.tpl.BUILD Renamed Linux extra-toolchain template.
dev/​bazel/​toolchains/​extra_toolchain_lnx.bzl Points to the renamed template.
dev/​bazel/​toolchains/​cc_toolchain_config_win.bzl Uses shared toolchain definitions.
dev/​bazel/​toolchains/​cc_toolchain_config_lnx.bzl Uses shared toolchain definitions.
dev/​bazel/​toolchains/​cc_toolchain_config_common.bzl Defines shared actions, features, and attributes.
dev/​bazel/​release.bzl Removes an unused toolchain declaration.
dev/​bazel/​cc/​link.bzl Removes an unused import.
dev/​bazel/​cc/​common.bzl Removes an unused import.
dev/​bazel/​cc.bzl Removes an unreachable copy branch and an unused import.
.bazelrc Removes an empty directive and trailing whitespace.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@napetrov napetrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified locally: 0 actions re-run vs #3829, release tree byte-identical to base.

@Alexandr-Solovev

Copy link
Copy Markdown
Contributor Author

Superseded by #3833, which now carries this PR's commits unchanged (plus the review fixes made here), rebased on current main, together with the rest of the dev/bazel series. Closing to avoid reviewing and merging the same changes twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants