Add `$(rlocationpath(s) ...)` expansion #16428

fmeum · 2022-10-08T07:50:54Z

The new location expansion pattern rlocationpath and its plural version rlocationpaths resolve to the path(s) suitable for the Rlocation function offered by runfiles libraries.

Compared to the rootpath pattern, which can returns ../repo_name/pkg/file for a file in an external repository and pkg/file for a file in the main repository, the path returned by rlocationpath is always of the form repo_name/pkg/file.

RELNOTES: The new path variable $(rlocationpath ...) and its plural form $(rlocationpaths ...) can be used to expand labels to the paths accepted by the Rlocation function of runfiles libraries. This is the preferred way to access data dependencies at runtime and works on all platforms, even when runfiles are not enabled (e.g., on Windows by default).

Work towards #16124
Fixes #10923

fmeum · 2022-10-08T09:55:57Z

@lberki Can you review this (or delegate to someone else)?

I haven't included any changes to docs yet, but could add them if the rest of the PR looks good. There are no docs on runfiles libraries yet though, so whatever I include in this PR will still be lacking context.

lberki · 2022-10-10T11:17:31Z

I'll defer to @comius here.

/cc @brandjon

comius

I was thinking of alternatives and I couldn't come up with any.

If I understand correctly, please confirm, rlocation always needs repo_name/path/in/the/repo as input. And this can be _main/path/in/the/main/repo. Right? What happens in the case this is a generated file - that is in bazel-bin/reponame/...?

Possible alternative would be $(reponame @repo//label), but this worse since you'd need to use 2 functions.

About the name. location(s) function is deprecated. I'm guessing you chose the name so it fits rlocation function (for example the one here https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash). So this then looks like rlocation $(rlocation //label). Does the plural form make sense here?

Because of deprecation of location, perhaps consider another name, like repopath? Maybe rlocation $(repopath //label) looks better?

Slightly out of scope, but do you think there's a way to simplify copy-pasted lines we have in almost all shell tests https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/archive_contents_test.sh#L20-L40 or https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/android/android_helper.sh#L17-L26?

One more naive question. Can we provide a function that directly returns what rlocation $(rlocation //label) would return? Eliminating the need for first/external rlocation.

comius · 2022-10-26T14:02:02Z

Also note, the name might cause some confusion when bash's $() is used. If you're doing substitutions you'll need something like $$(rlocation $(rlocation //label)) where inner one is substituted and $$ is unescaped.

fmeum · 2022-10-26T14:04:49Z

If I understand correctly, please confirm, rlocation always needs repo_name/path/in/the/repo as input.
Exactly.

And this can be _main/path/in/the/main/repo. Right?

Yes, for all files in the main repo, with Bzlmod enabled. Without Bzlmod, this would be __main__/path or <workspace name>/path (if a name is set).

What happens in the case this is a generated file - that is in bazel-bin/reponame/...?

Both generated and source file's use the same repo/package/path/in/repo/name path structure, just like in the runfiles directory.

Possible alternative would be $(reponame @repo//label), but this worse since you'd need to use 2 functions.

It's also difficult to get the remaining part since rootpath for an external repository doesn't just return package/path/in/repo/name, AFAIK it returns ../repo/package/path/in/repo/name.

About the name. location(s) function is deprecated. I'm guessing you chose the name so it fits rlocation function (for example the one here https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash). So this then looks like rlocation $(rlocation //label).

I chose rlocationpath instead of rlocation for two reasons:

To prevent confusing this with the deprecated location function, making it more similar to the recommended {exec,root}path functions.
To distinguish it from the rlocation functions defined by runfiles libraries, which are supposed to take whatever $(rlocationpath //label) evaluates to.

Your example would be rlocation $1 with args = ["$(rlocationpath //label)"].

Does the plural form make sense here?

I think so, yes. In Java, it would be easy to do Arrays.stream(args).map(arg -> r.rlocation(arg)), making use of the implicit shell tokenization of the args attribute.

Because of deprecation of location, perhaps consider another name, like repopath? Maybe rlocation $(repopath //label) looks better?

See above, do you think rlocationpath does a sufficient job at preventing this confusion? This expansion has one very clear purpose, providing the correct path for the rlocation function, so I think that rlocation should be in the name - otherwise it will be very hard to distinguish execpath and rootpath from the new expansion.

Slightly out of scope, but do you think there's a way to simplify copy-pasted lines we have in almost all shell tests https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/archive_contents_test.sh#L20-L40 or https://github.com/bazelbuild/bazel/blob/master/src/test/shell/bazel/android/android_helper.sh#L17-L26?

The latter one is more modern and already quite a bit shorter. I don't see a way to fundamentally improve on that.

fmeum · 2022-10-26T14:07:52Z

One more naive question. Can we provide a function that directly returns what rlocation $(rlocation //label) would return? Eliminating the need for first/external rlocation.

Also note, the name might cause some confusion when bash's $() is used. If you're doing substitutions you'll need something like $$(rlocation $(rlocation //label)) where inner one is substituted and $$ is unescaped.

Answering these in one as they are related: You wouldn't commonly do this since $(rlocationpath //label) has to be evaluated by Bazel, whereas $(rlocation repo/some/path) is an actual Bash function call happening at runtime.

We could think about ways to make $(rlocation //label) work from Bash directly, but that would require major changes to the runfiles tree - essentially, we would need to dump an additional manifest file that lists the contents of DefaultInfo.files for all Bazel targets contributing runfiles to a given executable. @lberki who has been interested in improving the usability of runfiles in this direction.

comius

Thanks for explanations! This makes it much easier to review. Couple of nits, but overall it looks ok.

src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java

comius · 2022-10-26T14:45:22Z

src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java

@@ -181,8 +189,10 @@ public void otherPathExternalExpansionNoLegacyExternalRunfiles() throws Exceptio
        .matches("foo .*-out/.*/external/r/p/foo\\.txt bar");
    assertThat(expander.expand("foo $(execpaths @r//p:foo) bar"))
        .matches("foo .*-out/.*/external/r/p/foo\\.txt bar");
-    assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
-    assertThat(expander.expand("foo $(rootpaths @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
+    assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).isEqualTo("foo ../r/p/foo.txt bar");


nit: isEqualTo seems better here, since it's not using a regex like matches. Can you use isEqualsTo in all the new code?

The old code should be updated separately, so we don't overload this PR.

I am using isEqualsTo in all new tests and reverted the changes to existing asserts.

src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java

src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java

fmeum

I addressed your comments.

I also added rlocationpath to the docs and fixed some outdated info about the other location functions.

fmeum · 2022-10-26T16:42:18Z

src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java

@@ -181,8 +189,10 @@ public void otherPathExternalExpansionNoLegacyExternalRunfiles() throws Exceptio
        .matches("foo .*-out/.*/external/r/p/foo\\.txt bar");
    assertThat(expander.expand("foo $(execpaths @r//p:foo) bar"))
        .matches("foo .*-out/.*/external/r/p/foo\\.txt bar");
-    assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
-    assertThat(expander.expand("foo $(rootpaths @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
+    assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).isEqualTo("foo ../r/p/foo.txt bar");


I am using isEqualsTo in all new tests and reverted the changes to existing asserts.

src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java

src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java

The new location expansion pattern `rlocationpath` and its plural version `rlocationpaths` resolve to the path(s) suitable for the Rlocation function offered by runfiles libraries. Compared to the `rootpath` pattern, which can returns '../repo_name/pkg/file` for a file in an external repository and `pkg/file` for a file in the main repository, the path returned by `rlocationpath` is always of the form `repo_name/pkg/file`. Work towards bazelbuild#16124 Fixes bazelbuild#10923

comius

Thanks!

fmeum · 2022-10-26T18:50:18Z

@bazel-io flag

comius · 2022-10-26T19:01:33Z

@sgowroji there might be some trouble merging make-variables.vm

sgowroji · 2022-10-31T15:32:05Z

Hello @comius, Above PR is awaiting to merge. What should be done for the internal fixes which we discussed offline, Can you please have a look. Thanks!

meteorcloudy · 2022-11-02T09:13:08Z

@bazel-io fork 6.0.0

fmeum · 2022-11-03T15:59:19Z

@comius If you have no further converns, could you approve? This is the final piece of API I need to be certain about to finalize my BazelCon talk.

meteorcloudy · 2022-11-03T16:11:23Z

@fmeum Ivo is already importing this PR ;)

ShreeM01 · 2022-11-04T22:16:12Z

@bazel-io fork 5.4.0

fmeum force-pushed the 10923-rlocation-path branch from 618aecd to 52fb838 Compare October 8, 2022 09:23

fmeum marked this pull request as ready for review October 8, 2022 09:54

sgowroji added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Oct 9, 2022

lberki requested a review from comius October 10, 2022 11:16

Wyverald assigned comius Oct 18, 2022

comius requested changes Oct 26, 2022

View reviewed changes

fmeum mentioned this pull request Oct 26, 2022

Escape tilde more gracefully #16560

Closed

comius requested changes Oct 26, 2022

View reviewed changes

fmeum force-pushed the 10923-rlocation-path branch from 52fb838 to 362fe14 Compare October 26, 2022 17:08

fmeum commented Oct 26, 2022

View reviewed changes

fmeum requested a review from comius October 26, 2022 17:08

fmeum force-pushed the 10923-rlocation-path branch 2 times, most recently from c019df0 to bb60c1e Compare October 26, 2022 17:10

fmeum force-pushed the 10923-rlocation-path branch from bb60c1e to cb3c0cb Compare October 26, 2022 17:12

comius approved these changes Oct 26, 2022

View reviewed changes

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Oct 26, 2022

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 Oct 26, 2022

bazel-io mentioned this pull request Nov 2, 2022

[6.0.0] Add $(rlocationpath(s) ...) expansion #16629

Closed

fmeum mentioned this pull request Nov 3, 2022

Make Java runfiles library repo mapping aware #16549

Closed

copybara-service bot closed this in a159330 Nov 3, 2022

fmeum deleted the 10923-rlocation-path branch November 3, 2022 19:10

sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 4, 2022

ShreeM01 added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 4, 2022

ShreeM01 added this to the 5.4.0 release bockers milestone Nov 4, 2022

bazel-io mentioned this pull request Nov 4, 2022

[5.4.0] Add $(rlocationpath(s) ...) expansion #16667

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 4, 2022

Add $(rlocationpath(s) ...) expansion #16428

Add $(rlocationpath(s) ...) expansion #16428

Uh oh!

Conversation

fmeum commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Oct 8, 2022

Uh oh!

lberki commented Oct 10, 2022

Uh oh!

comius left a comment • 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!

comius commented Oct 26, 2022

Uh oh!

fmeum commented Oct 26, 2022

Uh oh!

fmeum commented Oct 26, 2022

Uh oh!

comius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

comius Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

fmeum Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmeum left a comment

Choose a reason for hiding this comment

Uh oh!

fmeum Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

comius left a comment

Choose a reason for hiding this comment

Uh oh!

fmeum commented Oct 26, 2022

Uh oh!

comius commented Oct 26, 2022

Uh oh!

sgowroji commented Oct 31, 2022

Uh oh!

meteorcloudy commented Nov 2, 2022

Uh oh!

fmeum commented Nov 3, 2022

Uh oh!

meteorcloudy commented Nov 3, 2022

Uh oh!

ShreeM01 commented Nov 4, 2022

Uh oh!

Uh oh!

Add `$(rlocationpath(s) ...)` expansion #16428

Add `$(rlocationpath(s) ...)` expansion #16428

fmeum commented Oct 8, 2022 •

edited

Loading

comius left a comment •

edited

Loading