Skip to content

refactor(bazel): simplify dev/bazel: dead code, shared toolchain config, stock actions instead of helper scripts, docs - #3833

Open
Alexandr-Solovev wants to merge 14 commits into
uxlfoundation:mainfrom
Alexandr-Solovev:dev/asolovev_bazel_docs
Open

Alexandr-Solovev wants to merge 14 commits into
uxlfoundation:mainfrom
Alexandr-Solovev:dev/asolovev_bazel_docs

Conversation

@Alexandr-Solovev

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

Copy link
Copy Markdown
Contributor

The whole dev/bazel simplification series in one PR: #3829, #3830, #3831 and
#3832 were stacked on each other and are closed as superseded by this one. Each
part is still its own commits, in order, so it can be read one commit at a time;
the sections below are the former PR descriptions. #3827 (dependency
descriptions) and #3834 (oneMath on NVIDIA) are independent and stay separate.

This branch also carries the review fixes made on the individual PRs: the
whole-line kernel-define substitution (#3831), the merge docstring scope (#3832)
and the extra_inputs docstring grammar (#3829).

Validation on the combined branch, rebased on today's main: //cpp/daal:all_static
and //cpp/oneapi/dal:all_static build; --cpu=sse2 keeps only
DAAL_KERNEL_SSE2 in the patched kernel-defines header; host and DPC++ tests
pass (kmeans, array, blas gemm on GPU).

1. Remove dead code and fix the template file name typo (was #3829)

Pure
cleanup: nothing changes about what gets built.

  • extra_toolchian_lnx.tpl.BUILD → extra_toolchain_lnx.tpl.BUILD. The
    typo was self-consistent — the Label() in extra_toolchain_lnx.bzl spelled
    it the same way — so it worked, but the file did not show up when grepping
    dev/bazel/toolchains for the toolchain by name.
  • _copy_dynamic_release_file had an unreachable cp branch. Both call
    sites are inside if is_windows: and pass is_windows = is_windows, so the
    POSIX branch (added speculatively in Enable Bazel build and examples on Windows #3615) could never run. Renamed to
    _copy_windows_release_file, dropped the branch and the flag, and documented
    why the DLL/import-library rename is a copy and not a symlink.
  • copy_to_release declared a toolchain it does not use.
    _copy_to_release_impl resolved //dev/bazel/toolchains:extra into a local
    and never read it; daal.bzl is the real — and now only — consumer of that
    toolchain type.
  • Seven unreferenced load() symbols removed (paths, sets, utils,
    feature_set, tool_path ×2, artifact_name_pattern).
  • .bazelrc started with an argument-less common line, left behind by the
    Bazel 8 upgrade (Upgrade to Bazel 8.0.0 #3035): the flag it was meant to carry
    (--noincompatible_disallow_empty_glob) ended up appended to the build line
    in the same commit. Bazel silently accepts the empty directive, so it just sat
    there. The trailing whitespace a later flag removal left on the build line
    goes with it.

Validation

  • bazel build --nobuild //:release //:release_all — analyses clean.
  • bazel build //cpp/daal:core_static //cpp/oneapi/dal/algo:kmeans_dpc — full
    action-cache hit against the pre-change build, i.e. not one action key moved
    on Linux.
  • The rename is exercised by deleting the onedal_extra_toolchain repository
    marker and re-analysing, which re-runs the repository rule and regenerates its
    BUILD from the renamed template.

The Windows-only changes (_copy_windows_release_file, the tool_path load in
cc_toolchain_config_win.bzl) are covered by the WindowsBazel jobs on this PR.


  • 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

2. Share the platform-independent toolchain config (was #3830)

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.

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

3. Patch the kernel defines with a stock Bazel action (was #3831)

What this removes

cpp/daal/include/services/internal/daal_kernel_defines.h enables every CPU dispatch variant oneDAL can build, and the ISAs that --cpu leaves out have to be stripped from it before it is compiled or released. That one regex substitution was implemented as a custom Bazel toolchain:

file what it was for
toolchains/extra_toolchain.bzl a provider + a toolchain rule
toolchains/extra_toolchain_lnx.bzl, _win.bzl two repository rules
toolchains/extra_toolchain_extension.bzl a module extension to instantiate them
toolchains/extra_toolchain_{lnx,win}.tpl.BUILD two generated BUILD templates
toolchains/tools/patch_daal_kernel_defines.{sh,cmd,ps1} three copies of the patcher
toolchain_type(name = "extra"), 3 lines in MODULE.bazel the registration

Ten files, a custom toolchain type and a module extension, so that one action could run one sed.

ctx.actions.expand_template is the stock Bazel action for exactly this: it performs literal substitutions on a file at execution time, so nothing has to read the header during analysis and no helper tool is needed on either platform. The whole mechanism collapses to:

ctx.actions.expand_template(
    template = ctx.file.src,
    output = kernel_defines,
    substitutions = {
        "#define DAAL_KERNEL_{}".format(cpu.upper()): "\r" if is_windows else ""
        for cpu in disabled_cpus
    },
)

The Makefile's Windows behaviour — sed.eol is empty on Linux and a lone CR on Windows, so a disabled define leaves an empty line with the platform's own line ending — is expressed as the replacement string rather than as a separate script.

net -304 / +33 lines, and the dev/bazel/toolchains directory loses its second toolchain type.

Hermeticity fix

The old rule received the patcher through ExtraToolchainInfo as an attr.string absolute path and passed it to ctx.actions.run(executable = ...) without ever declaring it as an input. Editing patch_daal_kernel_defines.sh therefore did not invalidate the cached output. The new implementation has no tool at all, so the only inputs are the header and the --cpu setting, both of which Bazel already tracks.

Validation

  • The generated header is byte-identical to the previous implementation's output for --cpu=all, --cpu=avx512, --cpu=avx2 and --cpu=sse2 (captured before the change, compared after).
  • bazel build //cpp/daal:core_static //cpp/oneapi/dal/algo:kmeans_dpc is a full action-cache hit against the pre-change cache — no command line changed.
  • bazel build --nobuild //:release //:release_all //examples/oneapi/cpp:all analyses clean.
  • Repo-wide grep confirms no remaining reference to extra_toolchain, ExtraToolchainInfo, patch_daal_kernel_defines or //dev/bazel/toolchains:extra.

The Windows path has no CI coverage here; it is unchanged in intent and the replacement string reproduces what patch_daal_kernel_defines.ps1 wrote.

4. Drop the generated Linux toolchain wrapper scripts (was #3832)

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.

5. Refresh the dev/bazel TODO and README; fail on a missing optional tool (was #3833)

dev/bazel/TODO.md

Untouched since 2023, and three of its five items have been done for a while:

  • Windows support — toolchains/cc_toolchain_win.bzl configures Intel icx with an MSVC cl fallback; //:release and //:release_all build there in both CRT flavours under CI.
  • Release to oneAPI structure — //:release writes the full daal/latest tree, and nightly CI diffs it against the Make one.
  • Automatic host architecture identification — that is --cpu=auto, the default.

The compiler matrix still marked every Intel and MSVC cell :x:. It now reflects what the toolchains actually configure, and says plainly that Clang is recognised by detect_compiler but has no flag set, so it is untested.

The remaining items are rewritten to name the work instead of a heading:

  • Toolchain flag tables as data replaces "toolchain code unification". After refactor(bazel): share the platform-independent toolchain config #3830 the two cc_toolchain_config files share everything they can; no same-named feature emits the same flags on both platforms, so the rest is not duplication. The duplication that is real sits in toolchains/common.bzl's per-compiler / per-OS / per-ISA flag lists.
  • Hermetic toolchain — the compiler, archiver, linker and strip tool are absolute repo_ctx.which() paths, so builds depend on the host PATH and nothing invalidates the cache when the compiler behind it changes.
  • Windows DPC++ execution — release artifacts can include the DPC++ libraries, but nothing runs a SYCL queue there yet.
  • Remove the remaining helper scripts — each of the four left is listed with the specific thing Bazel or rules_cc does not expose, so the next person does not have to rediscover why it is still a script. (This series removed five others.)

dev/bazel/README.md

  • The Windows Bazelisk snippet pinned v1.28.1 while the Linux one said v1.29.0; both now match the installers and point at .ci/env/bazelisk.{sh,ps1}.
  • --cpu=auto is described the way config.bzl implements it — highest detected ISA plus the always-built sse2 dispatch baseline — which is what the release section 400 lines below already said.
  • Dropped the "What is missing in this guide: how to get make-like release structure" stub; "Build release artifacts" documents exactly that.

tool_not_found (second commit)

A real bug, found while inventorying the scripts. When repo_ctx.which() cannot find a non-mandatory tool — today only the DPC++ compiler — the toolchain points every action that would use it at tool_not_found, which printed a message to stdout and exited 0. Bazel then reported not all outputs were created for the first such action with the explanation buried in its stdout. Both stubs now write to stderr and exit non-zero, so the action fails with the message that says what to install.

🤖 Generated with Claude Code

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 stacked diff changes cross-platform Bazel toolchain and release behavior that warrants final CI-backed human review.

Review effort: Balanced
Findings: None

What changed in this PR

Refreshes Bazel documentation while incorporating stacked toolchain simplifications and clearer missing-tool failures.

Changes:

  • Consolidates shared Linux/Windows toolchain configuration.
  • Replaces generated patch/link/archive helpers with native Bazel actions.
  • Updates Bazel guidance, TODO status, and failure stubs.
File Description
.bazelrc Removes empty configuration and trailing whitespace.
MODULE.bazel Removes the obsolete extra toolchain registration.
dev/​bazel/​README.md Refreshes installation and CPU guidance.
dev/​bazel/​TODO.md Updates completed and outstanding Bazel work.
dev/​bazel/​cc.bzl Restricts release-copy helper to Windows.
dev/​bazel/​cc/​common.bzl Removes an unused import.
dev/​bazel/​cc/​link.bzl Uses native actions for archive merging and linking.
dev/​bazel/​daal.bzl Replaces header-patching scripts with template expansion.
dev/​bazel/​release.bzl Removes the unused extra toolchain dependency.
dev/​bazel/​toolchains/​BUILD Removes the extra toolchain type.
dev/​bazel/​toolchains/​action_names.bzl Deletes the obsolete custom action name.
dev/​bazel/​toolchains/​cc_toolchain_config_common.bzl Adds shared toolchain configuration factories.
dev/​bazel/​toolchains/​cc_toolchain_config_lnx.bzl Adopts shared configuration and direct tools.
dev/​bazel/​toolchains/​cc_toolchain_config_win.bzl Adopts shared configuration.
dev/​bazel/​toolchains/​cc_toolchain_lnx.bzl Removes generated Linux tool wrappers.
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 Deletes the Linux linker wrapper.
dev/​bazel/​toolchains/​extra_toolchain.bzl Deletes the 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_toolchain_win.tpl.BUILD Deletes the Windows template.
dev/​bazel/​toolchains/​extra_toolchian_lnx.tpl.BUILD Deletes the Linux template.
dev/​bazel/​toolchains/​merge_static_libs_lnx.tpl.sh Deletes the archive wrapper.
dev/​bazel/​toolchains/​tools/​patch_daal_kernel_defines.cmd Deletes the PowerShell launcher.
dev/​bazel/​toolchains/​tools/​patch_daal_kernel_defines.ps1 Deletes the Windows patcher.
dev/​bazel/​toolchains/​tools/​patch_daal_kernel_defines.sh Deletes the Linux patcher.
dev/​bazel/​toolchains/​tools/​tool_not_found.tpl.bat Reports errors on stderr and fails.
dev/​bazel/​toolchains/​tools/​tool_not_found.tpl.sh Reports errors on stderr and fails.

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

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

Seems like positive changes and cleanup assuming CI is still in good shape

Comment thread dev/bazel/toolchains/tools/tool_not_found.tpl.bat Outdated
Comment thread dev/bazel/toolchains/tools/tool_not_found.tpl.sh Outdated
Comment thread dev/bazel/cc.bzl
Comment thread dev/bazel/toolchains/cc_toolchain_lnx.tpl.BUILD Outdated
@ethanglaser

Copy link
Copy Markdown
Contributor

I guess my comments may have been more relevant to other PRs in the stack (#3830, #3831, #3832) but note that stacking does not work from PRs from forks unfortunately. So it's hard to tell which diff comes from each individual PR because they are all against main. Not sure about a solution for this other than either opening them incrementally once the previous one is merged, or combining all 4 into a single PR.

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

Series summary (this head contains #3827-#3832), since the stack can't be shown per PR from a fork. Each row is measured against base:

host release tree actions re-run vs previous PR (one output base)
#3827 identical 0 (1 internal)
#3829 identical 0
#3830 identical 0
#3831 identical 2 internal (expand_template)
#3832 identical 8 (4 .a + 4 .so links)
#3833 identical 0

Also on this head: --backend_config=ref (static OpenBLAS) and DPC //:release byte-identical to base; examples + kmeans/pca/linear_regression tests 193/193 (same as base).

tool_not_found: with no icpx on PATH, base fails with a misleading error while parsing .d file ... (No such file...) because the stub exits 0; this head fails with tool_not_found.sh failed + icpx is not found!. Clear improvement. Host builds without icpx still configure fine.

Two doc issues inline (TODO Clang claim, README --cpu). Minor: the Windows Clang entry in the TODO table goes stale once #3795 lands.

Comment thread dev/bazel/TODO.md Outdated

Intel `icx`/`icpx` is preferred whenever it is on `PATH`; `CC` overrides the
choice. `detect_compiler` in `toolchains/common.bzl` recognises `clang`, but
there is no Clang entry in the flag tables, so that path is untested.

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.

This isn't accurate: flags.bzl has Clang entries (lines 139 and 211). I built host //:release with CC=clang (clang 18.1.3) on this head. It fails with a single error class: 2478x argument unused during compilation: '-fno-strict-overflow' [-Werror,-Wunused-command-line-argument], from flags.bzl:150, which adds that flag for every non-icx compiler. With "clang" added to that exclusion list the full host //:release builds green.

Suggest either making that one-line fix here, or having the TODO name that as the blocker.

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.

You are right, and the claim was wrong on both halves — thanks for digging into it.

flags.bzl does have Clang entries, and they are not new in this stack: -Wno-pass-failed at flags.bzl:139 and the -march set in get_cpu_flags at flags.bzl:211 are both present at base 3120d0ab. So "no Clang entry in the flag tables" was simply false, and "untested" was hiding a single concrete blocker.

I reproduced your failure on this head (CC=clang / clang++, clang 18.1.3, bazel build --cpu=avx2 --release_dpc=false //:release):

clang: error: argument unused during compilation: '-fno-strict-overflow' [-Werror,-Wunused-command-line-argument]

Worth noting why it is unused, because the flag is not unsupported by clang: it is only redundant here. On its own clang accepts it silently; it becomes unused only next to -fwrapv, which lnx_cc_common_flags passes unconditionally:

$ clang++-18 -c -O2 -Werror -fno-strict-overflow t.cpp     # exit 0
$ clang++-18 -c -O2 -Werror -fwrapv -fno-strict-overflow t.cpp
clang++-18: error: argument unused during compilation: '-fno-strict-overflow' [-Werror,-Wunused-command-line-argument]

Took the one-line fix, in 019d86e — but narrowed to gcc rather than adding clang to the exclusion list:

        if compiler_id == "gcc":
            # Matches COMPILER.all.gnu in dev/make/compiler_definitions/gnu.32e.mk,
            # which pairs it with `-fwrapv`. Clang treats it as implied by
            # `-fwrapv` above and reports it unused, which `-Werror` makes fatal.
            flags = flags + ["-fno-strict-overflow"]

The reason for == "gcc" instead of not in ["icx", "icpx", "clang"] is Make parity: -fno-strict-overflow appears only in the gnu definitions (gnu.32e.mk:49, gnu.ref.arm.mk:46, both paired with -fwrapv), and clang.mk / clang.ref.arm.mk / clang.ref.riscv64.mk pass neither. A positive gcc condition therefore also covers the cross-clang targets instead of leaving them for the next person to hit.

Verified:

  • host //:release with CC=clang CXX=clang++ is green at --cpu=avx2 and --cpu=all;
  • aquery 'mnemonic("CppCompile", deps(//:release))' under clang shows the Clang branch of get_cpu_flags actually in use — -march=nocona / -march=haswell / -march=skylake-avx512 (the -march=skx entries are the icpx DPC++ targets still in the graph);
  • -fno-strict-overflow is gone from clang compile actions, -fwrapv and -Wno-pass-failed remain;
  • gcc is untouched: aquery 'mnemonic("CppCompile", //cpp/daal:services)' with CC=gcc is byte-identical before and after the change (14 actions, -fno-strict-overflow still present in each).

No CI lane changes behaviour: the only Bazel jobs that pin a compiler are LinuxBazelGNU_OpenBLAS_rv64 (CC=riscv64-linux-gnu-gcc) and the native AArch64 lane in ci-aarch64.yml (default, i.e. gcc on that image); everything else resolves to icx/icpx. Windows-arm64-clang-sve is a Make job (.ci/scripts/build.bat), not Bazel.

TODO updated to match in 9b96a4d — Linux/Clang is now a check mark, with the honest caveat attached:

Linux Clang builds //:release, but no CI job selects it, so regressions in that flag set are only found by hand. Windows Clang (clang-cl) is added by #3795.

That last sentence is also the answer to your "goes stale once #3795 lands" note: the Windows Clang cell stays :x: here with a pointer at the PR that flips it, rather than pre-announcing a state this branch cannot verify.

Comment thread dev/bazel/README.md Outdated
dispatch baseline, which is always included on x86.
- `modern` Compiles for `sse2`, `avx2`, `avx512`.
- `all` Compiles for all instruction sets listed below.
- Any comma-separated combination of the following values:

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.

Since this paragraph is being rewritten: the comma form doesn't work. On this head:

$ bazel build --cpu="avx2,avx512" //cpp/oneapi/dal:core
ERROR: .../+declare_onedal_config+config/BUILD:12:9: in cpu_info rule @@+declare_onedal_config+config//:cpu:
Error in fail: Unsupported CPU extensions: ["avx2,avx512"]

config/config.bzl:164 splits on spaces; --cpu="avx2 avx512" works. Please change "comma-separated" here and the example --cpu="avx2,avx512" a few lines below.

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.

Correct — the docs have been describing a separator the code never accepted. config/config.bzl:164 is

        isa_extensions = ctx.build_setting_value.split(" ")

Reproduced both forms on this head:

$ bazel build --nobuild --cpu="avx2,avx512" //cpp/oneapi/dal:core
Error in fail: Unsupported CPU extensions: ["avx2,avx512"]
$ bazel build --nobuild --cpu="avx2 avx512" //cpp/oneapi/dal:core
INFO: Build completed successfully

Fixed in 9b96a4d: "Any comma-separated combination" → "Any space-separated combination", and the example is now bazel test --cpu="avx2 avx512" //cpp/oneapi/dal:tests.

Checked that nothing else was relying on the comma form: README.md:216 was the only place in the repo that passed more than one ISA at all. .bazelrc, .ci/pipeline/ci.yml, .github/workflows/* and dev/bazel/tests/* all pass a single ISA or auto/all.

Teaching the parser to accept commas as well would be a one-line change in config.bzl, but that is a behaviour change rather than a doc fix, so I kept it out of this PR; happy to open it separately if you would rather the documented spelling win over the implemented one.

@Alexandr-Solovev

Copy link
Copy Markdown
Contributor Author

Thanks for the per-PR series measurements and for actually running tool_not_found without icpx on PATH — that is the case the second commit exists for.

Both inline points were right and are fixed; replies are on the respective threads. Head moved cee61417a → 9b96a4dc8:

  • 019d86e fix(bazel): restrict -fno-strict-overflow to gcc — the blocker behind the TODO's Clang claim. -fno-strict-overflow is not unsupported by clang, it is redundant next to the -fwrapv that lnx_cc_common_flags always passes, and -Werror makes that redundancy fatal. Narrowed to gcc rather than excluding clang, since Make passes the pair only in the gnu definitions.
  • 9b96a4d docs(bazel): correct the Clang status and the --cpu separator — TODO Clang row/text, and --cpu documented as space-separated (which is what config/config.bzl implements).

One caveat on your table, since the first of those is a functional change rather than a doc change: the "identical release tree / 0 actions re-run" result for #3833 was measured on the old head. The change is a no-op for every compiler CI uses, and I verified that rather than asserting it — aquery 'mnemonic("CppCompile", //cpp/daal:services)' with CC=gcc is byte-identical before and after, and the only Bazel lanes that pin a compiler pin gcc (LinuxBazelGNU_OpenBLAS_rv64, and the native AArch64 job in ci-aarch64.yml). What does change is CC=clang, which went from ~2.5k errors to a green host //:release at both --cpu=avx2 and --cpu=all.

If you would prefer the flags.bzl fix to live in its own PR instead of a docs one, say so and I will split it out — it is a self-contained commit either way.

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

Thanks, both points addressed. Re-checked on 9b96a4dc8: gcc host //:release is byte-identical to the previous head cee61417a built the same day in the same output base, so the -fno-strict-overflow narrowing is a no-op for gcc.

Approving the commits specific to this PR. Note that this branch also carries #3831, whose unanchored-replace thread is still open, so merge order matters.

Alexandr-Solovev and others added 11 commits October 6, 2026 05:36
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>
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>
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>
`TODO.md` had not been touched since 2023. "Windows support", "Release to
oneAPI structure" and "Automatic host architecture identification" are all
implemented and CI-covered, and the compiler matrix marked every Intel and
MSVC cell as unsupported. Those move to a Done section with a pointer to what
implements them, and the matrix now says what is actually configured.

The open items are rewritten to name concrete work rather than a heading:
the flag tables in `toolchains/common.bzl` rather than "toolchain code
unification" (the two `cc_toolchain_config` files no longer share anything
they could), the non-hermetic `repo_ctx.which()` tool discovery, Windows
DPC++ execution, and each of the four remaining helper scripts with the
reason it cannot be a Bazel action yet.

README: the Windows Bazelisk snippet still pinned v1.28.1 while the Linux one
said v1.29.0 and the installers now pin v1.29.0; both snippets point at
`.ci/env/bazelisk.{sh,ps1}`; `--cpu=auto` is described the way
`config.bzl` implements it, which the release section already stated
differently a few hundred lines further down; and the "What is missing in this
guide: how to get make-like release structure" stub is gone, since "Build
release artifacts" documents exactly that.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When `repo_ctx.which()` cannot find a non-mandatory tool -- today only the
DPC++ compiler -- the toolchain points every action that would use it at
`tool_not_found`, which printed a message to stdout and exited 0. Bazel then
reported "not all outputs were created" for the first such action, with the
explanation buried in the action's stdout, or worse let an empty output through
to a later step.

Both stubs now write to stderr and exit non-zero, so the action fails with the
message that says what to install.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`lnx_cc_common_flags` already passes `-fwrapv`, and clang reports
`-fno-strict-overflow` as implied by it, which `-Werror` turns into a
hard error on every compile action. Make passes the pair only for gnu
(`COMPILER.all.gnu` in gnu.32e.mk / gnu.ref.arm.mk); clang.mk passes
neither.

Signed-off-by: Alexandr-Solovev <aleksandr.solovev@intel.com>
The flag tables do have Clang entries, so the TODO's claim that the
path is untested was wrong; what blocked it was the flag fixed in the
previous commit. `--cpu` is split on spaces by `config/config.bzl`, not
on commas.

Signed-off-by: Alexandr-Solovev <aleksandr.solovev@intel.com>
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>
(cherry picked from commit e8293ca)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Alexandr-Solovev and others added 2 commits October 6, 2026 05:36
A literal key also matched longer macro names such as DAAL_KERNEL_AVX2_VNNI
and mentions inside comments; sed's ^...\b never did.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 force-pushed the dev/asolovev_bazel_docs branch from 6998fa4 to 6333f02 Compare October 6, 2026 12:39
@Alexandr-Solovev Alexandr-Solovev changed the title docs(bazel): refresh the dev/bazel TODO and README refactor(bazel): simplify dev/bazel: dead code, shared toolchain config, stock actions instead of helper scripts, docs Oct 6, 2026
@Alexandr-Solovev

Copy link
Copy Markdown
Contributor Author

@napetrov this PR now holds the whole dev/bazel series: #3829, #3830, #3831 and #3832 are closed as superseded, and their commits are here unchanged, in order, rebased on today's main, with the review fixes from those PRs included. You approved #3829, #3830 and the docs-only version of this PR. Could you confirm the approval covers the combined diff, in particular the #3831 (kernel-defines patch) and #3832 (Linux wrapper scripts) parts, which had no approval of their own?

This branch has not been deployed

No deployments
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.

4 participants