Skip to content

refactor(bazel): drop the generated Linux toolchain wrapper scripts - #3832

Closed
Alexandr-Solovev wants to merge 6 commits into
uxlfoundation:mainfrom
Alexandr-Solovev:dev/asolovev_bazel_drop_lnx_wrappers
Closed

refactor(bazel): drop the generated Linux toolchain wrapper scripts#3832
Alexandr-Solovev wants to merge 6 commits into
uxlfoundation:mainfrom
Alexandr-Solovev:dev/asolovev_bazel_drop_lnx_wrappers

Conversation

@Alexandr-Solovev

@Alexandr-Solovev Alexandr-Solovev commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fifth PR of the dev/bazel simplification series. Stacked on #3831 — the diff below is the last commit only.

The Linux toolchain templated two bash wrappers into the generated repository at configure time and put them on the critical path of every link and every archive merge. Both are gone; stock Bazel actions do the work.

dynamic_link_lnx.tpl.sh — an extra shell on every link

The wrapper's only real behaviour was scanning the command line for a *.def argument and rewriting it into -u <symbol> flags. Nothing in the tree passes a module-definition file to a Linux link (the single .def, export_win32e.def, is Makefile-only and Windows-only), and a .def has no GNU counterpart anyway — a version script is a different file format. So the branch was dead, and every link action forked a shell to reach %{cc_path} "$@".

Link actions now name the compiler driver directly, exactly like the compile actions already did. cc_link_path / dpcc_link_path move out of COMMON_ATTRS into the Windows rule, which is the only platform where the linker really is a separate binary (lld-link / link.exe), and the two tool() definitions they fed collapse into the existing cc_tool / dpcc_tool.

def_file now fail()s with a clear message off Windows rather than handing the file to gcc as an @response file.

merge_static_libs_lnx.tpl.sh — a custom action name for a stdin redirect

cc_common has no API for merging archives — its archiving action adds an input .a as a member — so oneDAL drives ar itself. GNU ar takes its merge instructions as an MRI script on stdin, and ctx.actions.run cannot redirect stdin, so the MRI script was generated by a shell wrapper. Supporting that required a whole custom action:

  • dev/bazel/toolchains/action_names.bzl declaring cpp_merge_static_libraries
  • an action_config for it whose tool() was the generated script's absolute path
  • an ar_merge_path rule attribute, a %{ar_merge_path} substitution and an ar_deps filegroup
  • cc_common.create_link_variables + get_memory_inefficient_command_line + get_environment_variables calls to build a command line for an action that has no flags

Replaced by:

actions.write(output = mri_script, content = "CREATE ...\nADDLIB ...\nSAVE\n")
actions.run_shell(
    command = '"{}" -M < "{}"'.format(archiver_path, mri_script.path),
    ...
    mnemonic = "MergeStaticLibraries",
    progress_message = "Merging static libraries into %{output}",
)

where archiver_path is the toolchain's resolved cpp_link_static_library tool. The MRI script becomes a tracked file, the custom action name and its action_config disappear, and the action gains a mnemonic and a progress message it never had (it was reported as a bare Action).

Net -204 / +62 lines, two generated scripts and one .bzl file removed.

Validation (Linux)

  • A probe target that merges a oneDAL static library with the MKL archives produces a byte-identical 658 MB .a before and after the change, from the same ADDLIB order.
  • libonedal.so, libonedal_parameters.so and libonedal_dpc.so relink byte-identically with the compiler driver called directly instead of through the wrapper — including the DPC++ link.
  • //:release, //:release_all and //examples/oneapi/cpp:all analyse clean.
  • Archive and link actions do re-execute once after this change, because the toolchain's ar_files / linker_files filegroups no longer contain the deleted scripts; their outputs are the byte-identical ones above.
  • ar_files and linker_files were :ar_deps / :linker_deps (the wrapper scripts) and are now omitted, i.e. empty, like dwp_files / objcopy_files / strip_files. The host ar and linker are therefore untracked absolute paths, which is already the case for the compiler driver's own tools; outputs are unchanged (host, DPC++ and --backend_config=ref release trees byte-identical to base, measured by @napetrov).

Windows is untouched apart from gaining the two link-path attributes it already used and a stale comment fix.

Alexandr-Solovev and others added 4 commits September 30, 2026 01:17
@Alexandr-Solovev @claude
refactor(bazel): remove dead code and fix the template file name typo
8ffcb3a 
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>
@Alexandr-Solovev @claude
refactor(bazel): share the platform-independent toolchain config
e103b1b 
`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>
@Alexandr-Solovev @claude
refactor(bazel): patch the kernel defines with a stock Bazel action
ef5acc7 
Removing one `#define` per disabled ISA from
`services/internal/daal_kernel_defines.h` was implemented as a custom
toolchain: a toolchain type, a provider, a repository rule, a module
extension, two generated `BUILD` templates and three copies of the patcher
itself (bash, batch, PowerShell). Ten files, so that an action could run one
regex substitution.

`ctx.actions.expand_template` performs the substitution at execution time, so
nothing needs to read the header during analysis and no helper tool is needed
on either platform. The Makefile's Windows line ending -- a disabled define
becomes a lone CR, leaving `\r\n` -- is expressed as the replacement string.

Also removes a real hermeticity bug: the patcher was handed to
`ctx.actions.run` as an `attr.string` absolute path, not as a file, so it was
not an input of the action and editing it did not invalidate the cached
output.

Verified byte for byte against the previous implementation's output for
`--cpu=all`, `avx512`, `avx2` and `sse2`, and the following build is a full
action-cache hit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aleksandr.solovev @claude
refactor(bazel): drop the generated Linux toolchain wrapper scripts
30f1fbd 
The Linux toolchain templated two bash wrappers into the generated
repository at configure time.

`dynamic_link_lnx.tpl.sh` sat between Bazel and the compiler on every link
action. Its only job was to turn a Windows module-definition file into
`-u <symbol>` flags; no target in the tree passes one, and a `.def` has no
GNU equivalent anyway (a version script is a different format), so the
branch was dead and every link paid for an extra shell. The link actions now
name the compiler driver directly, exactly like the compile actions do, and
`cc_link_path` / `dpcc_link_path` move out of the shared attributes into the
Windows rule, which is where a standalone linker really is used. `def_file`
now fails with a clear message off Windows instead of passing the file to
gcc as a response file.

`merge_static_libs_lnx.tpl.sh` wrote an MRI script for `ar -M`, and existed
only because `ctx.actions.run` cannot redirect stdin. Driving that from a
generated script also required a custom `cpp_merge_static_libraries` action
name and an `action_config` whose tool was a hard-coded absolute path.
`actions.write` produces the MRI script as a tracked file and
`actions.run_shell` redirects it into the toolchain's own, resolved
`cpp_link_static_library` archiver, so the custom action name, its
`action_config`, the `ar_merge_path` attribute and `action_names.bzl` all
disappear. The action also gains a mnemonic and a progress message, which it
never had.

Verified on Linux:

* a probe target that merges a static library with the MKL archives produces
  a byte-identical 658 MB result before and after;
* `libonedal.so`, `libonedal_parameters.so` and `libonedal_dpc.so` relink
  byte-identically without the wrapper;
* `//:release`, `//:release_all` and `//examples/oneapi/cpp:all` analyse
  clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 08:45
Copilot AI reviewed Sep 30, 2026
View reviewed changes

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

It changes critical cross-platform toolchain behavior while the Windows path remains unvalidated.

Review effort: Balanced
Findings: None

What changed in this PR

Simplifies Bazel toolchains by removing generated Linux wrappers and the stacked custom header-patching toolchain.

Changes:

  • Links directly through compiler drivers and merges archives using an MRI script.
  • Consolidates shared Linux/Windows toolchain configuration.
  • Replaces kernel-definition patch scripts with expand_template.
File Description
.bazelrc Removes empty configuration and trailing whitespace.
MODULE.bazel Unregisters the extra toolchain.
dev/​bazel/​cc.bzl Restricts release-file copying to Windows.
dev/​bazel/​cc/​common.bzl Removes an unused import.
dev/​bazel/​cc/​link.bzl Uses direct archiver and linker actions.
dev/​bazel/​daal.bzl Patches kernel defines with expand_template.
dev/​bazel/​release.bzl Removes extra-toolchain resolution.
dev/​bazel/​toolchains/​BUILD Removes the obsolete toolchain type.
dev/​bazel/​toolchains/​action_names.bzl Deletes the custom merge action name.
dev/​bazel/​toolchains/​cc_toolchain_config_common.bzl Adds shared toolchain features and attributes.
dev/​bazel/​toolchains/​cc_toolchain_config_lnx.bzl Uses direct tools and shared configuration.
dev/​bazel/​toolchains/​cc_toolchain_config_win.bzl Reuses shared configuration factories.
dev/​bazel/​toolchains/​cc_toolchain_lnx.bzl Stops generating Linux wrapper scripts.
dev/​bazel/​toolchains/​cc_toolchain_lnx.tpl.BUILD Removes wrapper inputs and attributes.
dev/​bazel/​toolchains/​cc_toolchain_win.bzl Clarifies Windows linker behavior.
dev/​bazel/​toolchains/​dynamic_link_lnx.tpl.sh Removes the Linux link wrapper.
dev/​bazel/​toolchains/​merge_static_libs_lnx.tpl.sh Removes the archive-merge wrapper.
dev/​bazel/​toolchains/​extra_toolchain.bzl Deletes extra-toolchain implementation.
dev/​bazel/​toolchains/​extra_toolchain_extension.bzl Deletes its module extension.
dev/​bazel/​toolchains/​extra_toolchain_lnx.bzl Deletes Linux extra-toolchain setup.
dev/​bazel/​toolchains/​extra_toolchain_win.bzl Deletes Windows extra-toolchain setup.
dev/​bazel/​toolchains/​extra_toolchian_lnx.tpl.BUILD Deletes Linux toolchain template.
dev/​bazel/​toolchains/​extra_toolchain_win.tpl.BUILD Deletes Windows toolchain template.
dev/​bazel/​toolchains/​tools/​patch_daal_kernel_defines.sh Deletes Linux patch helper.
dev/​bazel/​toolchains/​tools/​patch_daal_kernel_defines.cmd Deletes Windows launcher.
dev/​bazel/​toolchains/​tools/​patch_daal_kernel_defines.ps1 Deletes Windows patch helper.

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

@Alexandr-Solovev Alexandr-Solovev mentioned this pull request Sep 30, 2026
Open
10 tasks done
napetrov
napetrov reviewed Oct 2, 2026
View reviewed changes

@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.

Measured locally: Linux x86, Bazel 9.2.0, gcc 13.3, icpx 2025.2; clean env (env -i, no MKLROOT/TBBROOT); one worktree + output base per head, base = merge-base 3120d0ab.

  • Linux host: walking #3831 -> this head in one output base re-runs exactly 8 actions (the 4 .a + 4 .so links); all outputs byte-identical to base.
  • DPC: the libonedal_dpc.so link now execs icpx directly (was .../onedal_cc_toolchain/dpc_dynamic_link.sh); DPC //:release tree (1096 entries) at #3833, which contains this PR, is byte-identical to base.
  • Coverage of the new MRI path, inline.
  • Minor: on Linux only .a files reach the MRI branch (objects go through cc_common first), so "object files and" in the _merge_static_libs docstring applies to Windows only.

Comment thread dev/bazel/cc/link.bzl
feature_configuration = feature_configuration,
action_name = CPP_MERGE_STATIC_LIBRARIES,
variables = merger_variables,
mri_script = actions.declare_file(filename + ".mri")

@napetrov napetrov Oct 2, 2026

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.

Coverage note: on x86 with MKL this branch never runs. aquery 'mnemonic("MergeStaticLibraries", ...)' finds 0 actions for host //:release, DPC //:release, and //cpp/... + //examples/.... It only fires when a prebuilt .a is in the linking context, i.e. --backend_config=ref (libonedal_thread.a <- libonedal_thread_no_deps.a + libopenblas.a).

I built static OpenBLAS 0.3.34 with .ci/env/openblas.sh and ran that config: the MRI script works with the bzlmod + paths and the release tree is byte-identical to base. So the aarch64/rv64 OpenBLAS jobs are the only CI coverage of this code - worth stating in the description.

@Alexandr-Solovev Alexandr-Solovev Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed independently, and agreed it belongs in the description.

Reproduced the 0-action result here on x86 with MKL:

aquery 'mnemonic("MergeStaticLibraries", //:release)'                        -> 0 actions
aquery 'mnemonic("MergeStaticLibraries", //cpp/... union //examples/...)'    -> 0 actions

And pinned down exactly where the flag that does reach it lives — --backend_config=ref appears in two places in CI and nowhere else:

  • .github/workflows/ci-aarch64.yml lines 253, 305, 330 — the LinuxBazel_OpenBLAS(SVE) jobs
  • .ci/pipeline/ci.yml line 717 — CI (LinuxBazelGNU_OpenBLAS_rv64)

So your conclusion holds: the aarch64 and rv64 OpenBLAS jobs are the only CI coverage of the MRI branch. The x86 jobs exercise the Windows lib branch and the unchanged link paths instead. I could not re-run the ref config locally — this worktree has no static OpenBLAS, and aquery --backend_config=ref stops at Cannot locate +openblas_repo+openblas dependency — so I am taking your byte-identical ref release tree on report rather than claiming it myself.

Your docstring point is also right, and it was wrong in both the summary line and the Args: entry. Only lines 121 and 252 pass all_object_list, and both sit inside if is_windows; the Linux-reachable call sites at 150, 198 and 207 pass unpacked_linking_context.static_libraries alone. Fixed in 3497aba:

    """Archive whole static libraries into a single library.
    ...
    On Linux `static_libs` holds archives only, since object files go
    through `cc_common` first.

    Args:
        ...
        static_libs: static libraries to merge, plus loose object files on
                     Windows.

//:release still analyses clean after it. Docstring-only, so no action re-execution.

The coverage paragraph above is not in the description yet — I will get it in there.

@Alexandr-Solovev
docs(bazel): scope the merge docstring to what each platform passes
3497aba 
Only Windows passes loose object files; the Linux callers pass archives
alone, since objects go through `cc_common` first.

Signed-off-by: Alexandr-Solovev <aleksandr.solovev@intel.com>
@Alexandr-Solovev

Alexandr-Solovev commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

On the Copilot overview's "the Windows path remains unvalidated" — Windows Bazel is covered, and it is green on this head:

  • CI (WindowsBazel) — the Azure job at .ci/pipeline/ci.yml:757
  • Nightly-test / oneDAL Windows Bazel test — nightly-test.yml's bazel_win, on windows-2025

plus CI (WindowsMakeVC), CI (WindowsMakeIntel), CI (WindowsReleaseCompare) and Windows-arm64-clang-sve, all passing. So it is two independent Bazel-on-Windows jobs rather than a gap.

I think the flag comes from the file table in that overview, which lists extra_toolchain*.bzl and patch_daal_kernel_defines.{sh,cmd,ps1}. Those belong to #3831, which this PR is stacked on, not to the commit under review here — so the Windows surface it saw includes deleted .cmd/.ps1 helpers that this diff does not touch.

One thing in that direction is fair, though: the description's "Windows is untouched apart from..." undersells it slightly. def_file now fail()s off Windows, and cc_link_path / dpcc_link_path move out of COMMON_ATTRS into the Windows rule. Both are Windows-visible, both are covered by the jobs above.

@Alexandr-Solovev @claude
review: drop the empty file attributes from the Linux cc_toolchain
e8293ca 
None of the cc_toolchain file attributes is mandatory, and an omitted one
resolves to an empty depset, so listing :empty only adds noise.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Alexandr-Solovev

Alexandr-Solovev commented Oct 6, 2026

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

Reviewers

@napetrov napetrov napetrov left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@homksei homksei Awaiting requested review from homksei homksei is a code owner
@ahuber21 ahuber21 Awaiting requested review from ahuber21 ahuber21 is a code owner
@ethanglaser ethanglaser Awaiting requested review from ethanglaser ethanglaser is a code owner
@icfaust icfaust Awaiting requested review from icfaust icfaust is a code owner

Assignees

No one assigned

Labels

AI Agents infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Alexandr-Solovev @napetrov