Skip to content

Comments

fix: Only enable path mapping if supported#1243

Merged
fmeum merged 1 commit intomainfrom
fmeum/fix-path-mapping
Feb 24, 2026
Merged

fix: Only enable path mapping if supported#1243
fmeum merged 1 commit intomainfrom
fmeum/fix-path-mapping

Conversation

@fmeum
Copy link
Member

@fmeum fmeum commented Feb 24, 2026

#1217 unconditionally enabled support for path mapping for rules that may use location expansion or hardcode paths in other ways. Instead, only enable it when it is clearly safe to do so.

Verified manually by running bazel test //... --experimental_output_paths=strip.

Work towards cerisier/toolchains_llvm_bootstrapped#334

@aspect-workflows
Copy link

aspect-workflows bot commented Feb 24, 2026

Bazel 8 (Test)

All tests were cache hits

137 tests (100.0%) were fully cached saving 16s.


Bazel 9 (Test)

2 test targets passed

Targets
//lib/tests/copy_to_directory:case_22_test [k8-fastbuild]                    68ms
//lib/tests/copy_to_directory_bin_action:test [k8-fastbuild]                 68ms

Total test execution time was 136ms. 135 tests (98.5%) were fully cached saving 18s.


Bazel 7 (Test)

e2e/api_entries

All tests were cache hits

1 test (100.0%) was fully cached saving 24ms.


Bazel 8 (Test)

e2e/api_entries

All tests were cache hits

1 test (100.0%) was fully cached saving 30ms.


Bazel 9 (Test)

e2e/api_entries

All tests were cache hits

1 test (100.0%) was fully cached saving 50ms.


Bazel 7 (Test)

e2e/copy_action

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.


Bazel 8 (Test)

e2e/copy_action

All tests were cache hits

1 test (100.0%) was fully cached saving 26ms.


Bazel 9 (Test)

e2e/copy_action

All tests were cache hits

1 test (100.0%) was fully cached saving 32ms.


Bazel 7 (Test)

e2e/copy_to_directory

All tests were cache hits

6 tests (100.0%) were fully cached saving 297ms.


Bazel 8 (Test)

e2e/copy_to_directory

All tests were cache hits

6 tests (100.0%) were fully cached saving 286ms.


Bazel 9 (Test)

e2e/copy_to_directory

All tests were cache hits

6 tests (100.0%) were fully cached saving 349ms.


Bazel 7 (Test)

e2e/coreutils

All tests were cache hits

4 tests (100.0%) were fully cached saving 247ms.


Bazel 8 (Test)

e2e/coreutils

All tests were cache hits

4 tests (100.0%) were fully cached saving 216ms.


Bazel 9 (Test)

e2e/coreutils

All tests were cache hits

4 tests (100.0%) were fully cached saving 198ms.


Bazel 7 (Test)

e2e/external_copy_to_directory

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.


Bazel 8 (Test)

e2e/external_copy_to_directory

Buildkite build #450 is running...


Bazel 9 (Test)

e2e/external_copy_to_directory

All tests were cache hits

1 test (100.0%) was fully cached saving 32ms.


Bazel 7 (Test)

e2e/smoke

All tests were cache hits

4 tests (100.0%) were fully cached saving 611ms.


Bazel 8 (Test)

e2e/smoke

All tests were cache hits

4 tests (100.0%) were fully cached saving 508ms.


Bazel 9 (Test)

e2e/smoke

All tests were cache hits

4 tests (100.0%) were fully cached saving 618ms.


Bazel 7 (Test)

e2e/write_source_files

All tests were cache hits

1 test (100.0%) was fully cached saving 39ms.


Bazel 8 (Test)

e2e/write_source_files

All tests were cache hits

1 test (100.0%) was fully cached saving 33ms.


Bazel 9 (Test)

e2e/write_source_files

All tests were cache hits

1 test (100.0%) was fully cached saving 40ms.

@fmeum fmeum force-pushed the fmeum/fix-path-mapping branch from 970c84b to 3690617 Compare February 24, 2026 09:19
@fmeum fmeum changed the title Only enable path mapping if supported fix: Only enable path mapping if supported Feb 24, 2026
@fmeum fmeum requested review from alexeagle, Copilot and dzbarsky and removed request for dzbarsky February 24, 2026 09:19
Verified manually by running `bazel test //... --experimental_output_paths=strip`.
@fmeum fmeum force-pushed the fmeum/fix-path-mapping branch from 3690617 to 01ba0ec Compare February 24, 2026 09:20
@fmeum fmeum marked this pull request as ready for review February 24, 2026 09:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refines the path mapping support that was unconditionally enabled in PR #1217. It makes path mapping conditional, only enabling it when actions don't use location/make variable expansion or embed paths in configuration files or environment variables. This ensures compatibility with Bazel's --experimental_output_paths=strip flag.

Changes:

  • Added conditional path mapping to run_binary and expand_template rules based on whether expansion occurs
  • Removed path mapping support from copy_to_directory due to paths embedded in JSON config
  • Renamed COPY_EXECUTION_REQUIREMENTS to SUPPORTS_PATH_MAPPING for clarity

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/private/copy_common.bzl Renamed constant from COPY_EXECUTION_REQUIREMENTS to SUPPORTS_PATH_MAPPING for better clarity
lib/private/copy_file.bzl Updated to use renamed constant; continues to support path mapping
lib/private/copy_directory.bzl Updated to use renamed constant; continues to support path mapping
lib/private/copy_to_directory.bzl Removed path mapping support and added TODO comment explaining the JSON config prevents path mapping
lib/private/expand_template.bzl Added logic to conditionally enable path mapping only when substitutions don't use expansion
lib/private/run_binary.bzl Added logic to conditionally enable path mapping based on args/env expansion and stamping usage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@fmeum fmeum merged commit 12a09a5 into main Feb 24, 2026
20 checks passed
@fmeum fmeum deleted the fmeum/fix-path-mapping branch February 24, 2026 19:32
@fmeum
Copy link
Member Author

fmeum commented Feb 24, 2026

@alexeagle Could you cut a release with this change?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants