Add support for path mapping to `CppArchive` by fmeum · Pull Request #25081 · bazelbuild/bazel

fmeum · 2025-01-27T11:19:03Z

This mostly requires wiring up the existing machinery for structured variables and (most) link build variables.

Utility methods on PathMappers are moved to PathMapper so that they can be dependend on by AbstractCommandLine without creating a cycle.

Work towards #22658 (reply in thread)
Work towards #6526

This mostly requires wiring up the existing machinery for structured variables and (most) link build variables. Utility methods on `PathMappers` are moved to `PathMapper` so that they can be dependend on by `AbstractCommandLine` without creating a cycle.

fmeum · 2025-01-27T13:17:32Z

@bazel-io fork 8.1.0

src/main/java/com/google/devtools/build/lib/actions/PathMapper.java

comius · 2025-01-28T11:01:53Z

src/main/java/com/google/devtools/build/lib/actions/PathMapper.java

+        String execPathString = execPath.getPathString();
+        int startOfConfigSegment = execPathString.indexOf('/') + 1;
+        return PathFragment.createAlreadyNormalized(
+            execPathString.substring(0, startOfConfigSegment)
+                + "pm-"
+                + execPathString.substring(startOfConfigSegment));


Even though this piece of code is just moved from PathMappers class, I'd like to start a bit of discussion on it - and perhaps opening a bug if there's a better way to do it.

I hope there's a way that avoids string manipulation. In current form it might get broken by other changes to output paths we're doing.
cc @gregestren @aranguyen

Also I wonder about edge cases - what if the path doesn't contain a '/'?

That's a good point, I am checking for / now. Since, at least outside of C++ rules, user-specified paths don't end up as PathFragments, I think that a plain bazel-out path can't happen, but it's better to be safe.

I could rewrite this to use PathFragment methods, but those would equally depend on output paths being of the form (bazel|blaze)-out<separator><cfg><separator>everything_else. This would also depend on the layout of output paths though, but I don't see a way around that for what path mapping needs to too. This snippet uses string manipulation mostly because this code path is assumed to be hot and it should't allocate unnecessarily.

@comius what other changes to output paths are you thinking of?

Are you concerned about resource efficiency, correctness, both?

I meant changes to output paths made in other locations of Blaze. For example replacing cpu with platform name. But my guess is, that you're not able to change the path much without breaking subtle/hard coded integrations.

Those efforts should be unaffected: Path mapping assumes that output paths of generated files start with bazel-out/$CFG/ for some value of $CFG, but it treats that value as a blackbox. It may replace it, prepend to it, append to it, etc. but is specified to not branch on the particular value.

src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java

src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java

src/main/java/com/google/devtools/build/lib/actions/PathMapper.java

comius

A couple of linter sugesstions.

src/main/java/com/google/devtools/build/lib/actions/PathMapper.java

comius · 2025-01-29T12:54:28Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java

        toolchainEnv,
-        ImmutableMap.copyOf(executionInfo));
+        ImmutableMap.copyOf(executionInfo),
+        PathMappers.getOutputPathsMode(linkActionConstruction.getConfig()));


This is not reflected in src/main/starlark/builtins_bzl/common/cc/link/finalize_link_action.bzl

(it triggers IFTTT in the file)

That's expected, finalize_link_action.bzl goes through ctx.actions.run, which automatically retains the OutputPathsMode in SpawnAction. This doesn't require manual code.

src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java

This mostly requires wiring up the existing machinery for structured variables and (most) link build variables. Utility methods on `PathMappers` are moved to `PathMapper` so that they can be dependend on by `AbstractCommandLine` without creating a cycle. Work towards bazelbuild#22658 (reply in thread) Work towards bazelbuild#6526 Closes bazelbuild#25081. PiperOrigin-RevId: 721499678 Change-Id: I4551f268093c7b851791a41deb57292b2c23afb7

This mostly requires wiring up the existing machinery for structured variables and (most) link build variables. Utility methods on `PathMappers` are moved to `PathMapper` so that they can be dependend on by `AbstractCommandLine` without creating a cycle. Work towards #22658 (reply in thread) Work towards #6526 Closes #25081. PiperOrigin-RevId: 721499678 Change-Id: I4551f268093c7b851791a41deb57292b2c23afb7 Commit 7f6e649 Co-authored-by: Fabian Meumertzheim <[email protected]>

hvadehra · 2025-01-31T10:31:09Z

This broke things internally, and is being reverted.

I see it's already been merged into 8.1.0 so cc @meteorcloudy @Wyverald

fmeum · 2025-01-31T10:47:12Z

Please post any public details on the breakage when you have them.

hvadehra · 2025-01-31T10:51:34Z

The Bazel relevant bits are likely:

Caused by: net.starlark.java.eval.Starlark$UncheckedEvalException: IllegalStateException thrown during Starlark evaluation (REDACTED)
	at <starlark>.cc_toolchain_variables(<builtin>:0)
	at <starlark>.create_link_variables(/virtual_builtins_bzl/common/cc/link/link_build_variables.bzl:170)
	at (REDACTED)
	at (REDACTED)
Caused by: java.lang.IllegalStateException: Unexpected value: None (class net.starlark.java.eval.NoneType)
	at com.google.devtools.build.lib.rules.cpp.CcStarlarkInternal.getCcToolchainVariables(CcStarlarkInternal.java:147)

Note this is just the first place it failed, I can't say for certain if there would have been more later.

Reverts #25146 Due to #25081 (comment)

fmeum · 2025-01-31T12:10:23Z

@hvadehra Thanks, that was really helpful. I submitted a reland: #25154

fmeum requested review from a team, comius, lberki, pzembrod and trybka as code owners January 27, 2025 11:19

fmeum requested review from gregestren and removed request for a team January 27, 2025 11:19

github-actions bot added team-Performance Issues for Performance teams team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Jan 27, 2025

fmeum removed request for gregestren, lberki, pzembrod and trybka January 27, 2025 11:19

fmeum force-pushed the path-map-cpp-archive branch from 1f8abd6 to 580a076 Compare January 27, 2025 11:20

Fix tests

eb4ca63

bazel-io mentioned this pull request Jan 27, 2025

[8.1.0] Add support for path mapping to CppArchive #25084

Closed

comius requested changes Jan 28, 2025

View reviewed changes

fmeum added 2 commits January 28, 2025 12:34

Address comments

48b4996

Fix prefix check

447bd08

fmeum force-pushed the path-map-cpp-archive branch from 756d579 to 5f49286 Compare January 28, 2025 12:53

fmeum requested a review from comius January 28, 2025 13:03

Update cc_toolchain_variables for new variable types

c0e0b4b

fmeum force-pushed the path-map-cpp-archive branch from 5f49286 to c0e0b4b Compare January 28, 2025 13:26

comius approved these changes Jan 28, 2025

View reviewed changes

src/main/java/com/google/devtools/build/lib/actions/PathMapper.java Outdated Show resolved Hide resolved

comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 28, 2025

comius requested changes Jan 29, 2025

View reviewed changes

comius added awaiting-user-response Awaiting a response from the author and removed awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Jan 29, 2025

Address comments

bf69039

fmeum requested a review from comius January 29, 2025 13:38

fmeum added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jan 29, 2025

comius approved these changes Jan 29, 2025

View reviewed changes

copybara-service bot closed this in 7f6e649 Jan 30, 2025

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jan 30, 2025

bazel-io mentioned this pull request Jan 30, 2025

[8.1.0] Add support for path mapping to CppArchive #25146

Merged

meteorcloudy mentioned this pull request Jan 31, 2025

Revert "[8.1.0] Add support for path mapping to CppArchive" #25153

Merged

github-merge-queue bot pushed a commit that referenced this pull request Jan 31, 2025

Revert "[8.1.0] Add support for path mapping to CppArchive" (#25153)

4c00ef2

Reverts #25146 Due to #25081 (comment)

fmeum deleted the path-map-cpp-archive branch January 31, 2025 12:10

Conversation

fmeum commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Jan 27, 2025

Uh oh!

Uh oh!

comius Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

fmeum Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregestren Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

comius Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

fmeum Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

comius Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

fmeum Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvadehra commented Jan 31, 2025

Uh oh!

fmeum commented Jan 31, 2025

Uh oh!

hvadehra commented Jan 31, 2025

Uh oh!

fmeum commented Jan 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fmeum commented Jan 27, 2025 •

edited

Loading

fmeum Jan 28, 2025 •

edited

Loading

fmeum Jan 29, 2025 •

edited

Loading