From 103ea9d3f3a0d85a0d7c1ad98fd8dbb422505d95 Mon Sep 17 00:00:00 2001 From: wyv Date: Thu, 9 Dec 2021 04:52:37 -0800 Subject: [PATCH] Remove the concept of a "default" repo name and consolidate label parsing logic This CL started as an effort to clean up references to RepositoryName.DEFAULT. This constant signifies that a label starts without a `@repo` part, and so usually refers to the "current repo". A Label or PackageIdentifier with such a repo name is unusable. This constant is really only ever meaningfully used *while* parsing a label to be relative from another one (Label#getRelativeWithMapping). So we remove it. This also necessarily meant that we have to clean up the label parsing logic. There are multiple places in the Label, PackageIdentifier, and LabelValidator classes where the raw label string is parsed in some way and often they call each other, making the code nigh impossible to follow. This CL consolidates all the label parsing logic into one class, LabelParser, and makes all Label factory methods use this parser instead of their own logic. PiperOrigin-RevId: 415235412 --- .../lib/analysis/DependencyResolver.java | 4 +- .../analysis/config/OutputDirectories.java | 2 +- .../starlark/StarlarkRuleClassFunctions.java | 1 - .../build/lib/buildtool/SymlinkForest.java | 2 +- .../google/devtools/build/lib/cmdline/BUILD | 1 + .../devtools/build/lib/cmdline/Label.java | 308 ++++++------------ .../build/lib/cmdline/LabelParser.java | 195 +++++++++++ .../build/lib/cmdline/PackageIdentifier.java | 81 ++--- .../build/lib/cmdline/RepositoryMapping.java | 8 +- .../build/lib/cmdline/RepositoryName.java | 25 +- .../build/lib/cmdline/TargetPattern.java | 2 - .../lib/packages/AbstractAttributeMapper.java | 3 +- .../build/lib/packages/AspectDefinition.java | 10 +- .../build/lib/packages/BuildType.java | 3 +- .../packages/ConfiguredAttributeMapper.java | 3 +- .../packages/PackageGroupsRuleVisibility.java | 8 +- .../lib/packages/PackageSpecification.java | 2 +- .../build/lib/pkgcache/PackageOptions.java | 8 +- .../lib/pkgcache/PathPackageLocator.java | 2 - .../lib/rules/cpp/CcCompilationHelper.java | 3 +- .../rules/java/JavaCompileActionBuilder.java | 2 +- .../java/JavaHeaderCompileActionBuilder.java | 2 +- .../build/lib/rules/java/JavaRuntime.java | 2 +- .../java/OneVersionCheckActionBuilder.java | 2 +- .../rules/repository/RepositoryFunction.java | 8 - .../build/lib/skyframe/BzlLoadFunction.java | 2 - .../skyframe/ConfiguredTargetFunction.java | 4 +- .../ContainingPackageLookupValue.java | 3 +- .../build/lib/skyframe/PackageFunction.java | 3 - .../lib/skyframe/PackageLookupValue.java | 1 - .../build/lib/skyframe/PackageValue.java | 1 - .../lib/skyframe/ProcessPackageDirectory.java | 2 +- .../build/lib/skyframe/RecursivePkgKey.java | 1 - .../build/lib/skyframe/SkyframeExecutor.java | 2 - .../lib/skyframe/TransitiveTargetKey.java | 1 - .../skyframe/TransitiveTraversalValue.java | 1 - .../FakeStarlarkRuleFunctionsApi.java | 1 - .../analysis/test/TestActionBuilderTest.java | 22 +- .../lib/analysis/util/BuildViewTestCase.java | 4 +- .../devtools/build/lib/cmdline/LabelTest.java | 18 +- .../build/lib/cmdline/RepositoryNameTest.java | 3 +- .../lib/pkgcache/LoadingPhaseRunnerTest.java | 3 +- .../query2/testutil/AbstractQueryTest.java | 11 +- .../lib/rules/LabelBuildSettingTest.java | 2 +- .../RepositoryNameCodecTest.java | 1 - .../StarlarkRuleClassFunctionsTest.java | 7 +- .../build/lib/testutil/BlazeTestUtils.java | 12 - 47 files changed, 386 insertions(+), 406 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java index ec29773f021083..74c11be759af58 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java @@ -679,10 +679,8 @@ private void resolveAttribute( return; } - Label ruleLabel = rule.getLabel(); type.visitLabels( - (depLabel, ctx) -> - outgoingLabels.put(dependencyKind, ruleLabel.resolveRepositoryRelative(depLabel)), + (depLabel, ctx) -> outgoingLabels.put(dependencyKind, depLabel), attributeValue, /*context=*/ null); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java index 58dd68d9221048..a245cb361c1e88 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java @@ -256,7 +256,7 @@ private ArtifactRoot buildDerivedRoot( // instead. However, it requires individually symlinking the top-level elements of external // repositories, which is blocked by a Windows symlink issue #8704. RootType rootType; - if (repository.isMain() || repository.isDefault()) { + if (repository.isMain()) { rootType = isMiddleman ? RootType.SiblingMainMiddleman : RootType.SiblingMainOutput; } else { rootType = isMiddleman ? RootType.SiblingExternalMiddleman : RootType.SiblingExternalOutput; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index b98c164704e9da..14ce51ea507038 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -130,7 +130,6 @@ public Label load(String from) throws Exception { try { return Label.parseAbsolute( from, - /* defaultToMain=*/ false, /* repositoryMapping= */ ImmutableMap.of()); } catch (LabelSyntaxException e) { throw new Exception(from); diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java b/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java index f8ece0f13526de..9a70756ce107d2 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java @@ -364,7 +364,7 @@ public ImmutableList plantSymlinkForest() throws IOException, AbruptExitEx continue; } RepositoryName repository = pkgId.getRepository(); - if (repository.isMain() || repository.isDefault()) { + if (repository.isMain()) { // Record main repo packages. packageRootsForMainRepo.put(entry.getKey(), entry.getValue()); diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD index 21a2e040ddebf0..1acdae6fbc5ad3 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD +++ b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD @@ -55,6 +55,7 @@ java_library( name = "cmdline-primitives", srcs = [ "LabelConstants.java", + "LabelParser.java", "LabelSyntaxException.java", "PackageIdentifier.java", "RepositoryMapping.java", diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index d5295952c6bada..d592862f65e937 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java @@ -13,20 +13,20 @@ // limitations under the License. package com.google.devtools.build.lib.cmdline; +import static com.google.devtools.build.lib.cmdline.LabelParser.validateAndProcessTargetName; + import com.google.common.base.Preconditions; import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Interner; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.actions.CommandLineItem; -import com.google.devtools.build.lib.cmdline.LabelValidator.BadLabelException; +import com.google.devtools.build.lib.cmdline.LabelParser.Parts; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.lib.util.StringUtilities; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -61,58 +61,74 @@ public final class Label implements Comparable