Add factory for creating paths relative to well-known roots #15805

Yannic · 2022-07-04T14:59:27Z

This change adds a factory for creating PathFragments relative to
pre-defined (named) roots (e.g., relative to %workspace%).

The syntax is choosen to match existing ad-hoc solutions for %workspace%,
or %builtins% in other places (so that we can ideally migrate them in
a follow-up).

We'll use this for parsing paths from the command-line (e.g.,
--credential_helper=%workspace%/foo).

Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md

This change adds a factory for creating `PathFragments` relative to pre-defined (named) roots (e.g., relative to `%workspace%`). The syntax is choosen to match existing ad-hoc solutions for `%workspace%`, or `%builtins%` in other places (so that we can ideally migrate them in a follow-up). We'll use this for parsing paths from the command-line (e.g., `--credential_helper=%workspace%/foo`). Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md

Yannic · 2022-07-04T22:51:44Z

@tjgq PTAL

tjgq · 2022-07-05T10:19:34Z

src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java

+ * (e.g., {@code %workspace%/foo} becomes {@code </path/to/workspace>/foo}).
+ */
+public final class CommandLinePathFactory {
+  private static final Pattern REPLACEMENT_PATTERN = Pattern.compile("^(%([a-z_]+)%/)?([^%].*)$");


Make the last * a + so that an empty path is rejected.

* is actually correct. We reject the empty path because of the required [^%] before. Changing it to + would wrongly reject single-char paths (which are fine)

Oops, you're right :)

tjgq · 2022-07-05T10:28:26Z

src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java

+      if (root == null) {
+        throw new IllegalArgumentException(String.format(Locale.US, "Unknown root %s", rootName));
+      }
+      return root.getRelative(path);


Out of curiosity, what's the result of

new CommandLinePathFactory(ImmutableMap.of("workspace", "/path/to/workspace")).create("%workspace%/../bar");

Should we make it a requirement that the path must resolve to a descendant of the root (or possible even disallow . and .. segments entirely)?

Changed it to reject upwards references. PathFragment.create() will take care of normalizing it.

tjgq · 2022-07-05T10:29:33Z

src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java

+  }
+
+  @Test
+  public void UnknownRoot() {


Method name should start with lowercase.

of course...

tjgq · 2022-07-05T10:33:52Z

src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java

+    assertThat(factory.create("/absolute/path/2"))
+        .isEqualTo(PathFragment.create("/absolute/path/2"));
+  }
+


Add a test to verify that the empty path is rejected.

tjgq · 2022-07-05T10:36:57Z

src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java

+
+/** Tests for {@link CommandLinePathFactory}. */
+@RunWith(JUnit4.class)
+public class CommandLinePathFactoryTest {


Is this class also going to be used in the PATH lookup case? (i.e., --credential_helper=somehelper)? If so, we should have tests here for paths that don't start with a slash, or without slashes at all.

Sorry, let me clarify:

If we're going to use this class for PATH lookup (e.g. --credential_helper=foo), we should have a test for them.

Since we agreed in the proposal to disallow non-absolute paths with slashes (e.g. --credential_helper=foo/bar) unless they're relative to a %root%, we should reject them (and have a test to verify that we do).

Added PATH lookup (I was originally planning to do it somewhere else, but we may as well do it in this factory)

Actually, I'm not so sure PATH lookup belongs here. (What I had in mind was to simply store the non-absolute path, and resolve it later.)

I expect this code will run once at options parsing time, which means the result will be cached during the lifetime of the Bazel server process. Hence, if you add or remove a binary from one of the PATH directories that could affect the resolution of a particular helper, it won't take effect until you restart the server, which might be surprising to users.

WDYT? Should we defer PATH resolution until the helper is invoked?

Actually, I think what I said above only applies to startup options. If we can make --credential_helper a non-startup option, then I think it's fine to do the resolution here.

I was thinking of not making it the same option kind as --remote_{cache,executor}, which I think is build. It feels weird to make the credential helper a startup option when those aren't.

I suspect build is not enough (e.g. repository fetching is necessary for the query command to work) but I'm struggling to imagine a reason for it to be available at startup. Let's go with the current version.

Ack. I think query actually implies build for historical reasons.

Yannic · 2022-07-07T21:10:05Z

PTAL

tjgq · 2022-07-08T14:15:10Z

src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java

+
+/** Tests for {@link CommandLinePathFactory}. */
+@RunWith(JUnit4.class)
+public class CommandLinePathFactoryTest {


Actually, I'm not so sure PATH lookup belongs here. (What I had in mind was to simply store the non-absolute path, and resolve it later.)

I expect this code will run once at options parsing time, which means the result will be cached during the lifetime of the Bazel server process. Hence, if you add or remove a binary from one of the PATH directories that could affect the resolution of a particular helper, it won't take effect until you restart the server, which might be surprising to users.

WDYT? Should we defer PATH resolution until the helper is invoked?

tjgq · 2022-07-08T14:17:01Z

src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java

+
+    String rootName = matcher.group(2);
+    PathFragment path = PathFragment.create(matcher.group(3));
+    if (path.containsUplevelReferences()) {


Should we just use PathFragment.isNormalized here? I can't imagine a good reason to include a . component either.

I don't think we should. IIUC, PathFragment.isNormalized would mean that you need to pass / instead of \ on Windows, which feels like an unnecessary restriction.

Ack, I thought it only checked for . and ...

tjgq · 2022-07-08T15:05:26Z

src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java

+
+    String rootName = matcher.group(2);
+    PathFragment path = PathFragment.create(matcher.group(3));
+    if (path.containsUplevelReferences()) {


Ack, I thought it only checked for . and ...

tjgq · 2022-07-08T15:07:21Z

src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java

+
+/** Tests for {@link CommandLinePathFactory}. */
+@RunWith(JUnit4.class)
+public class CommandLinePathFactoryTest {


I suspect build is not enough (e.g. repository fetching is necessary for the query command to work) but I'm struggling to imagine a reason for it to be available at startup. Let's go with the current version.

Yannic · 2022-07-08T20:29:37Z

@bazel-io flag

ckolli5 · 2022-07-11T16:26:33Z

@bazel-io fork 5.3.0

ckolli5 · 2022-07-19T18:54:25Z

Hey @Yannic, we are trying to cherry-pick these changes to release-5.3.0. But pre-submit checks are failing due to missing dependencies. So, could you please help us in cherry picking these changes to release-5.3.0?

Yannic · 2022-07-20T12:08:45Z

Taking a look @ckolli5

This change adds a factory for creating `PathFragments` relative to pre-defined (named) roots (e.g., relative to `%workspace%`). The syntax is choosen to match existing ad-hoc solutions for `%workspace%`, or `%builtins%` in other places (so that we can ideally migrate them in a follow-up). We'll use this for parsing paths from the command-line (e.g., `--credential_helper=%workspace%/foo`). Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md Closes bazelbuild#15805. PiperOrigin-RevId: 460950483 Change-Id: Ie263fb6d6c2ea938a850a72793d551135df6862e

* Add factory for creating paths relative to well-known roots This change adds a factory for creating `PathFragments` relative to pre-defined (named) roots (e.g., relative to `%workspace%`). The syntax is choosen to match existing ad-hoc solutions for `%workspace%`, or `%builtins%` in other places (so that we can ideally migrate them in a follow-up). We'll use this for parsing paths from the command-line (e.g., `--credential_helper=%workspace%/foo`). Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md Closes #15805. PiperOrigin-RevId: 460950483 Change-Id: Ie263fb6d6c2ea938a850a72793d551135df6862e * Remove use of var

This change adds a factory for creating `PathFragments` relative to pre-defined (named) roots (e.g., relative to `%workspace%`). The syntax is choosen to match existing ad-hoc solutions for `%workspace%`, or `%builtins%` in other places (so that we can ideally migrate them in a follow-up). We'll use this for parsing paths from the command-line (e.g., `--credential_helper=%workspace%/foo`). Progress on https://github.com/bazelbuild/proposals/blob/main/designs/2022-06-07-bazel-credential-helpers.md Closes bazelbuild#15805. PiperOrigin-RevId: 460950483 Change-Id: Ie263fb6d6c2ea938a850a72793d551135df6862e

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

tjgq requested changes Jul 5, 2022

View reviewed changes

Yannic added 2 commits July 7, 2022 20:10

Merge branch 'master' into credhelper-cmdline-patg-factory

4f6e76e

address comments

0f81366

Yannic added 2 commits July 7, 2022 23:12

Error message

2327e52

fix

178d56b

tjgq requested changes Jul 8, 2022

View reviewed changes

tjgq approved these changes Jul 8, 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 Jul 8, 2022

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

bazel-io mentioned this pull request Jul 11, 2022

[5.3.0] Add factory for creating paths relative to well-known roots #15857

Closed

copybara-service bot closed this in 8374291 Jul 14, 2022

ckolli5 mentioned this pull request Jul 14, 2022

Add factory for creating paths relative to well-known roots #15885

Closed

Yannic deleted the credhelper-cmdline-patg-factory branch July 20, 2022 12:08

Yannic mentioned this pull request Jul 20, 2022

Add factory for creating paths relative to well-known roots #15931

Merged

ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add factory for creating paths relative to well-known roots #15805

Add factory for creating paths relative to well-known roots #15805

Yannic commented Jul 4, 2022

Yannic commented Jul 4, 2022

tjgq Jul 5, 2022

Yannic Jul 7, 2022

tjgq Jul 8, 2022

tjgq Jul 5, 2022

Yannic Jul 7, 2022

tjgq Jul 5, 2022

Yannic Jul 7, 2022

tjgq Jul 5, 2022

Yannic Jul 7, 2022

tjgq Jul 5, 2022

tjgq Jul 7, 2022

Yannic Jul 7, 2022

tjgq Jul 8, 2022

tjgq Jul 8, 2022

Yannic Jul 8, 2022

tjgq Jul 8, 2022

Yannic Jul 8, 2022

Yannic commented Jul 7, 2022

tjgq Jul 8, 2022

tjgq Jul 8, 2022

Yannic Jul 8, 2022

tjgq Jul 8, 2022

tjgq Jul 8, 2022

tjgq Jul 8, 2022

Yannic commented Jul 8, 2022

ckolli5 commented Jul 11, 2022

ckolli5 commented Jul 19, 2022

Yannic commented Jul 20, 2022

Add factory for creating paths relative to well-known roots #15805

Add factory for creating paths relative to well-known roots #15805

Conversation

Yannic commented Jul 4, 2022

Yannic commented Jul 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic commented Jul 8, 2022

ckolli5 commented Jul 11, 2022

ckolli5 commented Jul 19, 2022

Yannic commented Jul 20, 2022