Skip to content

Commit

Permalink
Ensure repository names don't start with ~
Browse files Browse the repository at this point in the history
By ensuring that repository names do not start with `~`, runfiles paths are certain not to begin with tildes, which means that they don't need to be shell escaped.

Prevents #16560 (comment)

Closes #16564.

PiperOrigin-RevId: 484232215
Change-Id: Ie70940aa709f6c7a886564189a3579780731dca6
  • Loading branch information
fmeum authored and copybara-github committed Oct 27, 2022
1 parent 30f6c82 commit 07c5c1a
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,16 @@ static BazelModuleResolutionValue createValue(
// Calculate a unique name for each used extension id.
BiMap<String, ModuleExtensionId> extensionUniqueNames = HashBiMap.create();
for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
String bestName =
id.getBzlFileLabel().getRepository().getName() + "~" + id.getExtensionName();
// Ensure that the resulting extension name (and thus the repository names derived from it) do
// not start with a tilde.
RepositoryName repository = id.getBzlFileLabel().getRepository();
String nonEmptyRepoPart;
if (repository.isMain()) {
nonEmptyRepoPart = "_main";
} else {
nonEmptyRepoPart = repository.getName();
}
String bestName = nonEmptyRepoPart + "~" + id.getExtensionName();
if (extensionUniqueNames.putIfAbsent(bestName, id) == null) {
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ public final class RepositoryName {

@SerializationConstant public static final RepositoryName MAIN = new RepositoryName("");

private static final Pattern VALID_REPO_NAME = Pattern.compile("[\\w\\-.~]*");
// Repository names must not start with a tilde as shells treat unescaped paths starting with them
// specially.
// https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html
private static final Pattern VALID_REPO_NAME = Pattern.compile("|[\\w\\-.][\\w\\-.~]*");

// Must start with a letter. Can contain ASCII letters and digits, underscore, dash, and dot.
private static final Pattern VALID_USER_PROVIDED_NAME = Pattern.compile("[a-zA-Z][-.\\w]*$");
Expand Down Expand Up @@ -156,7 +159,7 @@ static void validate(String name) throws LabelSyntaxException {
if (!VALID_REPO_NAME.matcher(name).matches()) {
throw LabelParser.syntaxErrorf(
"invalid repository name '@%s': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.'"
+ " and '~'",
+ " and '~' and must not start with '~'",
StringUtilities.sanitizeControlChars(name));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1041,7 +1041,7 @@ public void generatedReposHaveCorrectMappings_strictDepsViolation() throws Excep
assertThat(result.hasError()).isTrue();
assertThat(result.getError().getException())
.hasMessageThat()
.contains("No repository visible as '@foo' from repository '@~ext~ext'");
.contains("No repository visible as '@foo' from repository '@_main~ext~ext'");
}

@Test
Expand Down Expand Up @@ -1082,7 +1082,7 @@ public void importNonExistentRepo() throws Exception {
scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
scratch.file(
workspaceRoot.getRelative("data.bzl").getPathString(),
"load('@@~ext~ext//:data.bzl', ext_data='data')",
"load('@@_main~ext~ext//:data.bzl', ext_data='data')",
"data=ext_data");

SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
Expand Down Expand Up @@ -1216,7 +1216,7 @@ public void extensionRepoCtxReadsFromAnotherExtensionRepo() throws Exception {
workspaceRoot.getRelative("defs.bzl").getPathString(),
"load('@data_repo//:defs.bzl','data_repo')",
"def _ext_impl(ctx):",
" data_file = ctx.read(Label('@@~my_ext2~candy2//:data.bzl'))",
" data_file = ctx.read(Label('@@_main~my_ext2~candy2//:data.bzl'))",
" data_repo(name='candy1',data=data_file)",
"my_ext=module_extension(implementation=_ext_impl)",
"def _ext_impl2(ctx):",
Expand Down Expand Up @@ -1264,7 +1264,8 @@ public void testReportRepoAndBzlCycles_circularExtReposCtxRead() throws Exceptio
SkyKey skyKey =
PackageValue.key(
PackageIdentifier.create(
RepositoryName.createUnvalidated("~my_ext~candy1"), PathFragment.EMPTY_FRAGMENT));
RepositoryName.createUnvalidated("_main~my_ext~candy1"),
PathFragment.EMPTY_FRAGMENT));
EvaluationResult<PackageValue> result =
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
assertThat(result.hasError()).isTrue();
Expand All @@ -1275,11 +1276,11 @@ public void testReportRepoAndBzlCycles_circularExtReposCtxRead() throws Exceptio
assertContainsEvent(
"ERROR <no location>: Circular definition of repositories generated by module extensions"
+ " and/or .bzl files:\n"
+ ".-> @~my_ext~candy1\n"
+ ".-> @_main~my_ext~candy1\n"
+ "| extension 'my_ext' defined in //:defs.bzl\n"
+ "| @~my_ext2~candy2\n"
+ "| @_main~my_ext2~candy2\n"
+ "| extension 'my_ext2' defined in //:defs.bzl\n"
+ "`-- @~my_ext~candy1");
+ "`-- @_main~my_ext~candy1");
}

@Test
Expand Down Expand Up @@ -1310,7 +1311,7 @@ public void testReportRepoAndBzlCycles_circularExtReposLoadInDefFile() throws Ex
SkyKey skyKey =
PackageValue.key(
PackageIdentifier.create(
RepositoryName.createUnvalidated("~my_ext~candy1"),
RepositoryName.createUnvalidated("_main~my_ext~candy1"),
PathFragment.create("data.bzl")));
EvaluationResult<PackageValue> result =
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
Expand All @@ -1322,13 +1323,13 @@ public void testReportRepoAndBzlCycles_circularExtReposLoadInDefFile() throws Ex
assertContainsEvent(
"ERROR <no location>: Circular definition of repositories generated by module extensions"
+ " and/or .bzl files:\n"
+ ".-> @~my_ext~candy1\n"
+ ".-> @_main~my_ext~candy1\n"
+ "| extension 'my_ext' defined in //:defs.bzl\n"
+ "| @~my_ext2~candy2\n"
+ "| @_main~my_ext2~candy2\n"
+ "| extension 'my_ext2' defined in //:defs2.bzl\n"
+ "| //:defs2.bzl\n"
+ "| @~my_ext~candy1//:data.bzl\n"
+ "`-- @~my_ext~candy1");
+ "| @_main~my_ext~candy1//:data.bzl\n"
+ "`-- @_main~my_ext~candy1");
}

@Test
Expand All @@ -1350,7 +1351,7 @@ public void testReportRepoAndBzlCycles_extRepoLoadSelfCycle() throws Exception {
SkyKey skyKey =
PackageValue.key(
PackageIdentifier.create(
RepositoryName.createUnvalidated("~my_ext~candy1"),
RepositoryName.createUnvalidated("_main~my_ext~candy1"),
PathFragment.create("data.bzl")));
EvaluationResult<PackageValue> result =
evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
Expand All @@ -1362,10 +1363,10 @@ public void testReportRepoAndBzlCycles_extRepoLoadSelfCycle() throws Exception {
assertContainsEvent(
"ERROR <no location>: Circular definition of repositories generated by module extensions"
+ " and/or .bzl files:\n"
+ ".-> @~my_ext~candy1\n"
+ ".-> @_main~my_ext~candy1\n"
+ "| extension 'my_ext' defined in //:defs.bzl\n"
+ "| //:defs.bzl\n"
+ "| @~my_ext~candy1//:data.bzl\n"
+ "`-- @~my_ext~candy1");
+ "| @_main~my_ext~candy1//:data.bzl\n"
+ "`-- @_main~my_ext~candy1");
}
}
2 changes: 1 addition & 1 deletion src/test/py/bazel/bzlmod/bazel_module_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ def testRepositoryRuleErrorInModuleExtensionFailsTheBuild(self):
allow_failure=True)
self.AssertExitCode(exit_code, 48, stderr)
self.assertIn(
"ERROR: <builtin>: //pkg:~module_ext~foo: no such attribute 'invalid_attr' in 'repo_rule' rule",
"ERROR: <builtin>: //pkg:_main~module_ext~foo: no such attribute 'invalid_attr' in 'repo_rule' rule",
stderr)
self.assertTrue(
any([
Expand Down

0 comments on commit 07c5c1a

Please sign in to comment.