Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.2.0]Support (workspace) relative paths in --override_module #17906

Merged
merged 1 commit into from
Mar 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,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()) {
Expand All @@ -404,9 +405,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);
Expand Down Expand Up @@ -469,6 +470,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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,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(
Expand All @@ -141,7 +146,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(
Expand Down Expand Up @@ -305,17 +315,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);
}
Expand Down Expand Up @@ -347,15 +350,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
Expand All @@ -368,13 +365,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. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -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");
}
}
91 changes: 85 additions & 6 deletions src/test/py/bazel/bzlmod/bazel_module_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,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(',
Expand All @@ -406,15 +407,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()
Expand Down
18 changes: 11 additions & 7 deletions src/test/shell/bazel/workspace_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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(){
Expand Down