Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for path mapping to CppArchive #25081

Closed
wants to merge 6 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jan 27, 2025

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

@fmeum fmeum requested review from a team, lberki, comius, pzembrod and trybka as code owners January 27, 2025 11:19
@fmeum fmeum requested review from gregestren and removed request for a team January 27, 2025 11:19
@github-actions 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
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 fmeum force-pushed the path-map-cpp-archive branch from 1f8abd6 to 580a076 Compare January 27, 2025 11:20
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 27, 2025

@bazel-io fork 8.1.0

Comment on lines 307 to 312
String execPathString = execPath.getPathString();
int startOfConfigSegment = execPathString.indexOf('/') + 1;
return PathFragment.createAlreadyNormalized(
execPathString.substring(0, startOfConfigSegment)
+ "pm-"
+ execPathString.substring(startOfConfigSegment));
Copy link
Contributor

Choose a reason for hiding this comment

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

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 '/'?

Copy link
Collaborator Author

@fmeum fmeum Jan 28, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Are you concerned about resource efficiency, correctness, both?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@fmeum fmeum Jan 29, 2025

Choose a reason for hiding this comment

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

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.

@fmeum fmeum force-pushed the path-map-cpp-archive branch from 756d579 to 5f49286 Compare January 28, 2025 12:53
@fmeum fmeum requested a review from comius January 28, 2025 13:03
@fmeum fmeum force-pushed the path-map-cpp-archive branch from 5f49286 to c0e0b4b Compare January 28, 2025 13:26
@comius 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
Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

A couple of linter sugesstions.

@@ -1122,7 +1123,8 @@ private CppLinkAction buildLinkAction(
linkCommandLine,
linkActionConstruction.getConfig().getActionEnvironment(),
toolchainEnv,
ImmutableMap.copyOf(executionInfo));
ImmutableMap.copyOf(executionInfo),
PathMappers.getOutputPathsMode(linkActionConstruction.getConfig()));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

(it triggers IFTTT in the file)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@comius 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
@fmeum fmeum requested a review from comius January 29, 2025 13:38
@fmeum 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
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jan 30, 2025
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Jan 30, 2025
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
github-merge-queue bot pushed a commit that referenced this pull request Jan 31, 2025
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 <fabian@meumertzhe.im>
@hvadehra
Copy link
Member

This broke things internally, and is being reverted.

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

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 31, 2025

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

@hvadehra
Copy link
Member

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.

@fmeum fmeum deleted the path-map-cpp-archive branch January 31, 2025 12:10
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 31, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Performance Issues for Performance teams team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants