fix(linux): pass sysroot flags to assembly actions

[nradakovic]

Keep linker-specific -Wl,--sysroot flags limited to link actions while applying the compiler sysroot to assembly actions.

resolves #141

[Copilot]

🟡 Changes recommended

Assembly actions receive an inappropriate linker option, and regression coverage is missing.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Extends Linux sysroot handling to assembly and preprocessed assembly actions, with documentation updates.

Changes:

• Applies sysroot configuration to assembly actions.
• Updates maintenance and feature documentation.
• Lacks the promised .S/.s regression coverage.

File summaries

File	Summary
features/native/sysroot_link_flags/features.bzl	Adds assembly action coverage; requires separating link-only -Wl,--sysroot arguments and adding regression coverage.
docs/maintenance.md	Documents assembly sysroot handling.
docs/features.md	Updates feature behavior documentation.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[Copilot]

🟡 Changes recommended

Critical implementation and regression-test issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

docs/features.md:75

• The migration table still describes sysroot_link_flags as link-only and says compile-time handling relies on cxx_builtin_include_directories (docs/migration_guide.md:92), which is now inaccurate for .S/.s assembly. Please update that cross-reference to include the new assembly/preprocess-assemble behavior.

- **`sysroot_link_flags`** (Linux) — Adds `--sysroot` / `-Wl,--sysroot` at link,
  and `--sysroot` for assemble/preprocess-assemble actions (preprocessed `.S`
  sources need it to resolve sysroot headers).

features/native/sysroot_link_flags/features.bzl:3

• The new load statements are placed before the Apache/SPDX header, unlike the header-first layout used by the other repository .bzl files and the repository's copyright-check workflow. Keep the license header at the top and move these loads below it.

load("@rules_cc//cc/toolchains:feature.bzl", "cc_feature")
load("@rules_cc//cc/toolchains/args:sysroot.bzl", "cc_sysroot")

tests/feature_verification/sysroot_assemble.S:21

• <features.h> and _FEATURES_H are also provided by a normal host GCC under /usr/include, so this source can pass with the pre-change action configuration and does not prove that --sysroot was applied. Use a header or macro unique to the selected toolchain sysroot, or another assertion of the action flag, so the regression test fails when the sysroot flag is removed.

#include <features.h>

#ifndef _FEATURES_H
#error "sysroot headers not visible to preprocess-assemble"

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The regression test must uniquely verify sysroot resolution, and the documented/header issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

docs/maintenance.md:60

• The public migration table at docs/migration_guide.md:92 still describes sysroot_link_flags as link-only and says compile-time resolution relies solely on cxx_builtin_include_directories. This change also applies --sysroot to assemble/preprocess-assemble actions, so that table now contradicts the documented behavior; please update it together with this change.

  `cxx_builtin_include_directories`, so no `--sysroot` is needed for c/cxx
  compiles. Link-time `--sysroot` / `-Wl,--sysroot` and assembly-time
  `--sysroot` (assemble / preprocess-assemble; needed for `.S` sources since
  the extra_*_compile_flags include paths do not apply there) are emitted by
  the custom `sysroot_link_flags` feature. There is intentionally no legacy

features/native/sysroot_link_flags/features.bzl:2

• Please keep the SPDX/license header before the load() statements, as in the neighboring feature .bzl files such as features/native/extra_compile_flags/features.bzl:1-15. Moving executable Starlark statements above the header is a new repository-convention violation and can interfere with header/license tooling.

load("@rules_cc//cc/toolchains:feature.bzl", "cc_feature")
load("@rules_cc//cc/toolchains/args:sysroot.bzl", "cc_sysroot")

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

[AlexanderLanin]

GPT Luna generated review.

The new regression test is not actually sensitive to the regression it is intended to prevent. On the x86_64-linux test configuration, I ran: bazel test --lockfile_mode=error --config=x86_64-linux --features=-sysroot_link_flags --test_output=errors //feature_verification:assemble_sysroot_test. It still passed, while bazel aquery confirmed that the .S compile action had no --sysroot. features.h is available through the compiler/host include search path, so including it and checking _FEATURES_H does not prove that the selected toolchain sysroot was used. Please make the fixture depend on a header or macro unique to the configured sysroot, or otherwise assert the preprocess-assemble action arguments, so removing the assembly sysroot args makes the test fail.

Also, docs/migration_guide.md:92 still describes sysroot_link_flags as link-only and says compile-time handling relies on cxx_builtin_include_directories. That is now incomplete for .S/.s assembly and leaves the migration guide inconsistent with the updated feature and maintenance documentation.

[AlexanderLanin]

GPT Luna generated follow-up review.

The migration-guide wording is now corrected, but the latest PR revision no longer contains the .S fixture, assemble_sysroot_test target, or any regression coverage for preprocess-assemble. The current diff only changes the feature definition and documentation, so the original behavior can regress again without CI detecting it. Please keep a regression test that exercises a preprocessed assembly source and verifies a header or macro unique to the configured sysroot (or otherwise asserts the preprocess-assemble action arguments). The feature implementation itself remains correctly scoped: the linker-only -Wl,--sysroot argument is limited to link actions, and the compiler --sysroot argument is applied to assembly actions.

[Copilot]

🟡 Changes recommended

Add regression coverage for assembly sysroot handling and clarify the maintenance documentation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

features/native/sysroot_link_flags/features.bzl:38

• Please add a regression test for the new assembly path, for example a .S target that includes a header resolved only from the configured Linux sysroot (or an action-level assertion that preprocess_assemble receives --sysroot). The existing feature-verification suite covers many compiler/linker features (tests/feature_verification/BUILD:139-148) but contains no assembly source, so this issue can regress without any current CI test failing.

        cc_sysroot(
            name = "sysroot_assembly_flags_args",
            actions = ["@rules_cc//cc/toolchains/actions:assembly_actions"],
            sysroot = sysroot,
        )

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite