Automatically map paths in `Args#map_each` #19723

fmeum · 2023-10-04T11:12:52Z

When path mapping is enabled, File objects accessed in a user-supplied callback passed to Args#map_each automatically have their paths mapped.

Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on map_each do not need to be modified to use path mapping.

As part of this change, PathMapper has to become an abstract class as interfaces can't override equals and hashCode.

Work towards #6526

fmeum · 2023-10-04T11:13:09Z

Stacked on #19717

fmeum · 2023-10-04T17:59:36Z

@lberki Could you review the second commit? This is what we talked about at BazelCon 2022 (I know, it's been a while :-) ). I settled on an approach where every access to relevant File fields is remapped automatically, see the PR description for the reasoning. Briefly: users have a single choice and we know what that choice should be.

fmeum · 2023-10-04T18:00:17Z

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

+    // It would *not* be correct to just apply PathMapper#map to the exec path of the root: The
+    // root part of the mapped exec path of this artifact may depend on its complete exec path as
+    // well as on e.g. the digest of the artifact.
+    PathFragment mappedExecPath = PathMapper.loadFrom(semantics).map(execPath);


@lberki Would you expect the load from semantics to matter CPU-wise? If so, I could implement two versions of the function that are alternated between based on a semantics flag.

My professional opinion is that I don't know. What I think is that this shouldn't matter much, because it's not called in a very tight loop and thus an extra map lookup, say, per dependency edge shouldn't matter much. When in doubt, I'd rather err on the side of simple code.

lberki · 2023-10-05T12:10:52Z

New thread, who dis? :)

More seriously, I haven't been following the story of PathMapper very closely, but it looks like at the general idea that .path should return the mapped path instead of the non-mapped path when called from map_each is the least of all the bad approaches:

Adding a mapped_path method to File would require whoever creates mappable actions to change their code to work properly
Adding a new argument to map_each, likewise, although one could argue that the intersection of "uses map_each" and "wants path mapping" is small enough that maybe we could get away with incurring some extra work

If this idea proves to be bad, reverting it and going with (1) or (2) would cost the same as doing them right now, wouldn't it?

.path returning different things based on the context it's called in does look like a smell, though. I guess it comes down to whether we think path mappers should be exposed to Starlark. AFIAU the current goal is that they be as transparent as possible, which is what this change does.

fmeum · 2023-10-05T13:12:24Z

If this idea proves to be bad, reverting it and going with (1) or (2) would cost the same as doing them right now, wouldn't it?

Yes, I think so :-) Plus only real-world usage will tell which approach is ultimately better.

Adding a new argument to map_each, likewise, although one could argue that the intersection of "uses map_each" and "wants path mapping" is small enough that maybe we could get away with incurring some extra work

I would argue that the intersection is about as large as the "wants path mapping" set itself: If you want path mapping for a realistically complicated action, you will have to use map_each to get the more complicated arguments path. Without path mapping, you can always flatten the depset and manually construct whatever arguments you want, but with path mapping this doesn't work because e.g. "--resource={}:{}".format(file.short_path, file.path) won't be subject to mapping.

fmeum · 2023-10-09T20:09:14Z

@gregestren @aranguyen This is no longer stacked and ready for review.

aranguyen · 2023-10-10T03:00:51Z

@fmeum Speaking of priority for our presentation, since #19717 is merged and considering your comment here #19717 (comment), do we need this merged in order for us to do some demo with rules go? If not, I do want to spend a bit more time on #18098 (comment)

fmeum · 2023-10-10T10:58:09Z

Yeah, let's solve the issue that came up over at #18098 first. The current PR is required to proclaim general Starlark support and invite ruleset authors to pick it up, which would be pretty nice, but isn't a hard requirement as some Starlark actions will still qualify.

aranguyen · 2023-11-14T22:27:18Z

Not sure where I previously mentioned issue regarding paths get mapped multiple times which results in wrong path with missing /bin or /genfiles. I tried to build the same failing target with this PR hoping it would resolve the issue but it's still failing. Changing this pattern here to Pattern.compile(outputRoot + "/[\\w_-]+[^bin|genfiles]/") to make sure that mapped path doesn't get replaced again seems to work for me with pure luck because I don't think i'm expressing this correctly. Any idea how to express not bin / not genfiles properly?

fmeum · 2023-11-15T12:44:10Z

I think I can resolve this by introducing the fixed cfg segment. I will work on a separate PR for that.

aranguyen · 2023-11-15T15:05:14Z

I think I can resolve this by introducing the fixed cfg segment. I will work on a separate PR for that.

by fixed cfg, do you mean instead of removing the configuration, we decide on a fixed string to replace it with?

fmeum · 2023-11-15T15:33:47Z

@aranguyen Yes, exactly. in addition to improved compatibility, this also gives us a way to more easily
recognize mapped paths.

src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java

fmeum · 2024-03-05T16:14:46Z

@aranguyen I talked to Ivo offline and learned that lazy depset transformations may not be coming soon after all.

Instead, I would add a ctx.actions.path_mapping_manifest function that registers a action to create a File containing the mapping from mapped paths back to original paths. This would allow e.g. rules_kotlin to fix up its jdeps files after the actual compilation action, similar to what JavaCompilationAction does in native rule code.

This can wait for a separate PR though.

When path mapping is enabled, `File` objects accessed in a user-supplied callback passed to `Args#map_each` automatically have their paths mapped. Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on `map_each` do not need to be modified to use path mapping.

aranguyen · 2024-04-04T14:33:20Z

@fmeum I fixed the merge conflict internally and will send this out for internal review soon.

fmeum · 2024-04-04T14:37:21Z

@aranguyen It looks like this broke the GitHub checks. Do you want me to fix them here?

fmeum · 2024-04-04T21:44:16Z

@bazel-io fork 7.2.0

When path mapping is enabled, `File` objects accessed in a user-supplied callback passed to `Args#map_each` automatically have their paths mapped. Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on `map_each` do not need to be modified to use path mapping. As part of this change, `PathMapper` has to become an abstract class as interfaces can't override `equals` and `hashCode`. Work towards bazelbuild#6526 Closes bazelbuild#19723. PiperOrigin-RevId: 621972618 Change-Id: If76e5beee86a354aa97cb3000315b75b2ed24028

fmeum force-pushed the starlark-path-mapping branch 2 times, most recently from 08a4169 to a801bf7 Compare October 4, 2023 11:29

fmeum marked this pull request as ready for review October 4, 2023 11:29

fmeum requested review from a team and lberki as code owners October 4, 2023 11:29

fmeum requested review from gregestren and aranguyen and removed request for a team and lberki October 4, 2023 11:29

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions team-Performance Issues for Performance teams team-Rules-Java Issues for Java rules labels Oct 4, 2023

gregestren assigned fmeum Oct 4, 2023

gregestren added this to the Bazel 7.0: config-free output paths milestone Oct 4, 2023

fmeum requested a review from lberki October 4, 2023 17:58

fmeum commented Oct 4, 2023

View reviewed changes

fmeum mentioned this pull request Oct 4, 2023

Allow actions to opt into path mapping via execution requirements #19717

Closed

fmeum force-pushed the starlark-path-mapping branch 2 times, most recently from fabfd8c to dbedc66 Compare October 9, 2023 20:07

fmeum force-pushed the starlark-path-mapping branch 2 times, most recently from 9cfbb95 to 670f1b3 Compare October 9, 2023 20:26

comius removed the team-Rules-Java Issues for Java rules label Oct 10, 2023

aranguyen reviewed Jan 9, 2024

View reviewed changes

src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java Show resolved Hide resolved

fmeum force-pushed the starlark-path-mapping branch 2 times, most recently from 3bcf76c to fb72613 Compare January 10, 2024 06:56

fmeum requested a review from a team as a code owner January 10, 2024 06:56

fmeum mentioned this pull request Mar 4, 2024

Multiplatform output paths are safe, correct, and efficient #6526

Open

fmeum force-pushed the starlark-path-mapping branch from fb72613 to 2c83949 Compare March 8, 2024 18:51

Merge branch 'master' into starlark-path-mapping

df5ec8b

copybara-service bot closed this in 955b31e Apr 4, 2024

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Apr 4, 2024

fmeum deleted the starlark-path-mapping branch April 4, 2024 21:43

bazel-io mentioned this pull request Apr 4, 2024

[7.2.0] Automatically map paths in Args#map_each #21903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically map paths in `Args#map_each` #19723

Automatically map paths in `Args#map_each` #19723

fmeum commented Oct 4, 2023 •

edited

Loading

fmeum commented Oct 4, 2023

fmeum commented Oct 4, 2023

fmeum Oct 4, 2023

lberki Oct 5, 2023 •

edited

Loading

lberki commented Oct 5, 2023 •

edited

Loading

fmeum commented Oct 5, 2023

fmeum commented Oct 9, 2023

aranguyen commented Oct 10, 2023 •

edited

Loading

fmeum commented Oct 10, 2023 •

edited

Loading

aranguyen commented Nov 14, 2023 •

edited

Loading

fmeum commented Nov 15, 2023

aranguyen commented Nov 15, 2023

fmeum commented Nov 15, 2023

fmeum commented Mar 5, 2024 •

edited

Loading

aranguyen commented Apr 4, 2024

fmeum commented Apr 4, 2024

fmeum commented Apr 4, 2024

Automatically map paths in Args#map_each #19723

Automatically map paths in Args#map_each #19723

Conversation

fmeum commented Oct 4, 2023 • edited Loading

fmeum commented Oct 4, 2023

fmeum commented Oct 4, 2023

fmeum Oct 4, 2023

Choose a reason for hiding this comment

lberki Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

lberki commented Oct 5, 2023 • edited Loading

fmeum commented Oct 5, 2023

fmeum commented Oct 9, 2023

aranguyen commented Oct 10, 2023 • edited Loading

fmeum commented Oct 10, 2023 • edited Loading

aranguyen commented Nov 14, 2023 • edited Loading

fmeum commented Nov 15, 2023

aranguyen commented Nov 15, 2023

fmeum commented Nov 15, 2023

fmeum commented Mar 5, 2024 • edited Loading

aranguyen commented Apr 4, 2024

fmeum commented Apr 4, 2024

fmeum commented Apr 4, 2024

Automatically map paths in `Args#map_each` #19723

Automatically map paths in `Args#map_each` #19723

fmeum commented Oct 4, 2023 •

edited

Loading

lberki Oct 5, 2023 •

edited

Loading

lberki commented Oct 5, 2023 •

edited

Loading

aranguyen commented Oct 10, 2023 •

edited

Loading

fmeum commented Oct 10, 2023 •

edited

Loading

aranguyen commented Nov 14, 2023 •

edited

Loading

fmeum commented Mar 5, 2024 •

edited

Loading