Support (workspace) relative paths in --override_module

closes bazelbuild#17551

PiperOrigin-RevId: 519710834
Change-Id: I992d68e40b600ba921000d34d878186701baad2c

Author: SalmaSamy

src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java

@@ -394,7 +394,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
         // We use a LinkedHashMap to preserve the iteration order.
         Map<RepositoryName, PathFragment> overrideMap = new LinkedHashMap<>();
         for (RepositoryOverride override : repoOptions.repositoryOverrides) {
-          overrideMap.put(override.repositoryName(), override.path());
+          String repoPath = getAbsolutePath(override.path(), env);
+          overrideMap.put(override.repositoryName(), PathFragment.create(repoPath));
         }
         ImmutableMap<RepositoryName, PathFragment> newOverrides = ImmutableMap.copyOf(overrideMap);
         if (!Maps.difference(overrides, newOverrides).areEqual()) {
@@ -406,9 +407,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
 
       if (repoOptions.moduleOverrides != null) {
         Map<String, ModuleOverride> moduleOverrideMap = new LinkedHashMap<>();
-        for (RepositoryOptions.ModuleOverride modOverride : repoOptions.moduleOverrides) {
-          moduleOverrideMap.put(
-              modOverride.moduleName(), LocalPathOverride.create(modOverride.path()));
+        for (RepositoryOptions.ModuleOverride override : repoOptions.moduleOverrides) {
+          String modulePath = getAbsolutePath(override.path(), env);
+          moduleOverrideMap.put(override.moduleName(), LocalPathOverride.create(modulePath));
         }
         ImmutableMap<String, ModuleOverride> newModOverrides =
             ImmutableMap.copyOf(moduleOverrideMap);
@@ -471,6 +472,21 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     }
   }
 
+  /**
+   * If the given path is absolute path, leave it as it is. If the given path is a relative path, it
+   * is relative to the current working directory. If the given path starts with '%workspace%, it is
+   * relative to the workspace root, which is the output of `bazel info workspace`.
+   *
+   * @return Absolute Path
+   */
+  private String getAbsolutePath(String path, CommandEnvironment env) {
+    path = path.replace("%workspace%", env.getWorkspace().getPathString());
+    if (!PathFragment.isAbsolute(path)) {
+      path = env.getWorkingDirectory().getRelative(path).getPathString();
+    }
+    return path;
+  }
+
   @Override
   public ImmutableList<Injected> getPrecomputedValues() {
     return ImmutableList.of(

src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java

@@ -151,7 +151,12 @@ public class RepositoryOptions extends OptionsBase {
       converter = RepositoryOverrideConverter.class,
       documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
       effectTags = {OptionEffectTag.UNKNOWN},
-      help = "Overrides a repository with a local directory.")
+      help =
+          "Override a repository with a local path in the form of <repository name>=<path>. If the"
+              + " given path is an absolute path, it will be used as it is. If the given path is a"
+              + " relative path, it is relative to the current working directory. If the given path"
+              + " starts with '%workspace%, it is relative to the workspace root, which is the"
+              + " output of `bazel info workspace`")
   public List<RepositoryOverride> repositoryOverrides;
 
   @Option(
@@ -161,7 +166,12 @@ public class RepositoryOptions extends OptionsBase {
       converter = ModuleOverrideConverter.class,
       documentationCategory = OptionDocumentationCategory.BZLMOD,
       effectTags = {OptionEffectTag.UNKNOWN},
-      help = "Overrides a module with a local directory.")
+      help =
+          "Override a module with a local path in the form of <module name>=<path>. If the given"
+              + " path is an absolute path, it will be used as it is. If the given path is a"
+              + " relative path, it is relative to the current working directory. If the given path"
+              + " starts with '%workspace%, it is relative to the workspace root, which is the"
+              + " output of `bazel info workspace`")
   public List<ModuleOverride> moduleOverrides;
 
   @Option(
@@ -325,17 +335,10 @@ public RepositoryOverride convert(String input) throws OptionsParsingException {
         throw new OptionsParsingException(
             "Repository overrides must be of the form 'repository-name=path'", input);
       }
-      OptionsUtils.AbsolutePathFragmentConverter absolutePathFragmentConverter =
-          new OptionsUtils.AbsolutePathFragmentConverter();
-      PathFragment path;
-      try {
-        path = absolutePathFragmentConverter.convert(pieces[1]);
-      } catch (OptionsParsingException e) {
-        throw new OptionsParsingException(
-            "Repository override directory must be an absolute path", input, e);
-      }
+      OptionsUtils.PathFragmentConverter pathConverter = new OptionsUtils.PathFragmentConverter();
+      String pathString = pathConverter.convert(pieces[1]).getPathString();
       try {
-        return RepositoryOverride.create(RepositoryName.create(pieces[0]), path);
+        return RepositoryOverride.create(RepositoryName.create(pieces[0]), pathString);
       } catch (LabelSyntaxException e) {
         throw new OptionsParsingException("Invalid repository name given to override", input, e);
       }
@@ -367,15 +370,9 @@ public ModuleOverride convert(String input) throws OptionsParsingException {
                 pieces[0]));
       }
 
-      OptionsUtils.AbsolutePathFragmentConverter absolutePathFragmentConverter =
-          new OptionsUtils.AbsolutePathFragmentConverter();
-      try {
-        var path = absolutePathFragmentConverter.convert(pieces[1]);
-        return ModuleOverride.create(pieces[0], path.toString());
-      } catch (OptionsParsingException e) {
-        throw new OptionsParsingException(
-            "Module override directory must be an absolute path", input, e);
-      }
+      OptionsUtils.PathFragmentConverter pathConverter = new OptionsUtils.PathFragmentConverter();
+      String pathString = pathConverter.convert(pieces[1]).getPathString();
+      return ModuleOverride.create(pieces[0], pathString);
     }
 
     @Override
@@ -388,13 +385,13 @@ public String getTypeDescription() {
   @AutoValue
   public abstract static class RepositoryOverride {
 
-    private static RepositoryOverride create(RepositoryName repositoryName, PathFragment path) {
+    private static RepositoryOverride create(RepositoryName repositoryName, String path) {
       return new AutoValue_RepositoryOptions_RepositoryOverride(repositoryName, path);
     }
 
     public abstract RepositoryName repositoryName();
 
-    public abstract PathFragment path();
+    public abstract String path();
   }
 
   /** A module override, represented by a name and an absolute path to a module. */

src/test/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptionsTest.java

@@ -45,21 +45,22 @@ public class RepositoryOptionsTest {
   public void testOverrideConverter() throws Exception {
     RepositoryOverride actual = converter.convert("foo=/bar");
     assertThat(actual.repositoryName()).isEqualTo(RepositoryName.createUnvalidated("foo"));
-    assertThat(actual.path()).isEqualTo(PathFragment.create("/bar"));
+    assertThat(PathFragment.create(actual.path())).isEqualTo(PathFragment.create("/bar"));
   }
 
   @Test
   public void testOverridePathWithEqualsSign() throws Exception {
     RepositoryOverride actual = converter.convert("foo=/bar=/baz");
     assertThat(actual.repositoryName()).isEqualTo(RepositoryName.createUnvalidated("foo"));
-    assertThat(actual.path()).isEqualTo(PathFragment.create("/bar=/baz"));
+    assertThat(PathFragment.create(actual.path())).isEqualTo(PathFragment.create("/bar=/baz"));
   }
 
   @Test
   public void testOverridePathWithTilde() throws Exception {
     RepositoryOverride actual = converter.convert("foo=~/bar");
     assertThat(actual.repositoryName()).isEqualTo(RepositoryName.createUnvalidated("foo"));
-    assertThat(actual.path()).isEqualTo(PathFragment.create(USER_HOME.value() + "/bar"));
+    assertThat(PathFragment.create(actual.path()))
+        .isEqualTo(PathFragment.create(USER_HOME.value() + "/bar"));
   }
 
   @Test
@@ -70,6 +71,15 @@ public void testModuleOverridePathWithTilde() throws Exception {
         .isEqualTo(PathFragment.create(USER_HOME.value() + "/bar"));
   }
 
+  @Test
+  public void testModuleOverrideRelativePath() throws Exception {
+    var converter = new ModuleOverrideConverter();
+    ModuleOverride actual = converter.convert("foo=%workspace%/bar");
+    assertThat(actual.path()).isEqualTo("%workspace%/bar");
+    actual = converter.convert("foo=../../bar");
+    assertThat(actual.path()).isEqualTo("../../bar");
+  }
+
   @Test
   public void testInvalidOverride() throws Exception {
     expectedException.expect(OptionsParsingException.class);
@@ -85,10 +95,4 @@ public void testInvalidRepoOverride() throws Exception {
     converter.convert("foo/bar=/baz");
   }
 
-  @Test
-  public void testInvalidPathOverride() throws Exception {
-    expectedException.expect(OptionsParsingException.class);
-    expectedException.expectMessage("Repository override directory must be an absolute path");
-    converter.convert("foo=bar");
-  }
 }

src/test/py/bazel/bzlmod/bazel_module_test.py

@@ -389,7 +389,8 @@ def testRepositoryRuleErrorInModuleExtensionFailsTheBuild(self):
             in line for line in stderr
         ]))
 
-  def testCommandLineModuleOverride(self):
+  def testCmdAbsoluteModuleOverride(self):
+    # test commandline_overrides takes precedence over local_path_override
     self.ScratchFile('MODULE.bazel', [
         'bazel_dep(name = "ss", version = "1.0")',
         'local_path_override(',
@@ -416,15 +417,93 @@ def testCommandLineModuleOverride(self):
     ])
     self.ScratchFile('bb/WORKSPACE')
 
-    _, _, stderr = self.RunBazel([
-        'build', '--experimental_enable_bzlmod', '@ss//:all',
-        '--override_module', 'ss=' + self.Path('bb')
-    ],
-                                 allow_failure=False)
+    _, _, stderr = self.RunBazel(
+        ['build', '@ss//:all', '--override_module', 'ss=' + self.Path('bb')],
+        allow_failure=False,
+    )
     # module file override should be ignored, and bb directory should be used
     self.assertIn(
         'Target @ss~override//:choose_me up-to-date (nothing to build)', stderr)
 
+  def testCmdRelativeModuleOverride(self):
+    self.ScratchFile('aa/WORKSPACE')
+    self.ScratchFile(
+        'aa/MODULE.bazel',
+        [
+            'bazel_dep(name = "ss", version = "1.0")',
+        ],
+    )
+    self.ScratchFile('aa/BUILD')
+
+    self.ScratchFile('aa/cc/BUILD')
+
+    self.ScratchFile('bb/WORKSPACE')
+    self.ScratchFile(
+        'bb/MODULE.bazel',
+        [
+            'module(name="ss")',
+        ],
+    )
+    self.ScratchFile(
+        'bb/BUILD',
+        [
+            'filegroup(name = "choose_me")',
+        ],
+    )
+
+    _, _, stderr = self.RunBazel(
+        [
+            'build',
+            '@ss//:all',
+            '--override_module',
+            'ss=../../bb',
+            '--enable_bzlmod',
+        ],
+        cwd=self.Path('aa/cc'),
+        allow_failure=False,
+    )
+    self.assertIn(
+        'Target @ss~override//:choose_me up-to-date (nothing to build)', stderr
+    )
+
+  def testCmdWorkspaceRelativeModuleOverride(self):
+    self.ScratchFile('WORKSPACE')
+    self.ScratchFile(
+        'MODULE.bazel',
+        [
+            'bazel_dep(name = "ss", version = "1.0")',
+        ],
+    )
+    self.ScratchFile('BUILD')
+    self.ScratchFile('aa/BUILD')
+    self.ScratchFile('bb/WORKSPACE')
+    self.ScratchFile(
+        'bb/MODULE.bazel',
+        [
+            'module(name="ss")',
+        ],
+    )
+    self.ScratchFile(
+        'bb/BUILD',
+        [
+            'filegroup(name = "choose_me")',
+        ],
+    )
+
+    _, _, stderr = self.RunBazel(
+        [
+            'build',
+            '@ss//:all',
+            '--override_module',
+            'ss=%workspace%/bb',
+        ],
+        cwd=self.Path('aa'),
+        allow_failure=False,
+    )
+    self.assertIn(
+        'Target @ss~override//:choose_me up-to-date (nothing to build)', stderr
+    )
+
   def testDownload(self):
     data_path = self.ScratchFile('data.txt', ['some data'])
     data_url = pathlib.Path(data_path).resolve().as_uri()

src/test/shell/bazel/workspace_test.sh

@@ -263,29 +263,33 @@ local_repository(
     path = "original",
 )
 EOF
+  # Test absolute path
   bazel build --override_repository="o=$PWD/override" @o//:gen &> $TEST_log \
     || fail "Expected build to succeed"
   assert_contains "override" bazel-genfiles/external/o/gen.out
-
+  # Test no override used
   bazel build @o//:gen &> $TEST_log \
     || fail "Expected build to succeed"
   assert_contains "original" bazel-genfiles/external/o/gen.out
-
-  bazel build --override_repository="o=$PWD/override" @o//:gen &> $TEST_log \
+  # Test relative path (should be relative to working directory)
+  bazel build --override_repository="o=override" @o//:gen &> $TEST_log \
     || fail "Expected build to succeed"
   assert_contains "override" bazel-genfiles/external/o/gen.out
 
-  bazel build @o//:gen &> $TEST_log \
-    || fail "Expected build to succeed"
-  assert_contains "original" bazel-genfiles/external/o/gen.out
-
   # For multiple override options, the latest should win
   bazel build --override_repository=o=/ignoreme \
         --override_repository="o=$PWD/override" \
         @o//:gen &> $TEST_log \
     || fail "Expected build to succeed"
   assert_contains "override" bazel-genfiles/external/o/gen.out
 
+  # Test workspace relative path
+  mkdir -p dummy
+  cd dummy
+  bazel build --override_repository="o=%workspace%/override" @o//:gen &> $TEST_log \
+    || fail "Expected build to succeed"
+  assert_contains "override" ../bazel-genfiles/external/o/gen.out
+
 }
 
 function test_workspace_override_starlark(){