From f7627e00bf96c9159ab79a32afc4f6a622f0deeb Mon Sep 17 00:00:00 2001 From: salma-samy Date: Mon, 27 Mar 2023 07:05:22 -0700 Subject: [PATCH] Support (workspace) relative paths in --override_module closes https://github.com/bazelbuild/bazel/issues/17551 PiperOrigin-RevId: 519710834 Change-Id: I992d68e40b600ba921000d34d878186701baad2c --- .../lib/bazel/BazelRepositoryModule.java | 24 ++++- .../bazel/repository/RepositoryOptions.java | 43 ++++----- .../repository/RepositoryOptionsTest.java | 22 +++-- src/test/py/bazel/bzlmod/bazel_module_test.py | 91 +++++++++++++++++-- src/test/shell/bazel/workspace_test.sh | 18 ++-- 5 files changed, 149 insertions(+), 49 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index ca3f462a32dbab..671b43e0d3c97c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/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 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 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 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 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 getPrecomputedValues() { return ImmutableList.of( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java index a5b392e1a95b3f..d99b7dcce59a79 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptions.java +++ b/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 =. 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 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 =. 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 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. */ diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptionsTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptionsTest.java index 5de4cc8b5a8ec5..930000918b369c 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/RepositoryOptionsTest.java +++ b/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"); - } } diff --git a/src/test/py/bazel/bzlmod/bazel_module_test.py b/src/test/py/bazel/bzlmod/bazel_module_test.py index c645262f754e4a..4f61be2c8809c1 100644 --- a/src/test/py/bazel/bzlmod/bazel_module_test.py +++ b/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() diff --git a/src/test/shell/bazel/workspace_test.sh b/src/test/shell/bazel/workspace_test.sh index 144610ef08c871..6faf3cec504768 100755 --- a/src/test/shell/bazel/workspace_test.sh +++ b/src/test/shell/bazel/workspace_test.sh @@ -263,22 +263,19 @@ 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" \ @@ -286,6 +283,13 @@ EOF || 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(){