Allow labels in the '--aspects' parameter.
--
MOS_MIGRATED_REVID=139573590
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index b29d2a2..5cd90d2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -75,6 +75,9 @@
import com.google.devtools.build.lib.skyframe.SkyframeBuildView;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.SkylarkImport;
+import com.google.devtools.build.lib.syntax.SkylarkImports;
+import com.google.devtools.build.lib.syntax.SkylarkImports.SkylarkImportSyntaxException;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.RegexFilter;
@@ -465,7 +468,22 @@
// TODO(jfield): For consistency with Skylark loads, the aspect should be specified
// as an absolute path. Also, we probably need to do at least basic validation of
// path well-formedness here.
- PathFragment bzlFile = new PathFragment("/" + aspect.substring(0, delimiterPosition));
+ String bzlFileLoadLikeString = aspect.substring(0, delimiterPosition);
+ if (!bzlFileLoadLikeString.startsWith("//") && !bzlFileLoadLikeString.startsWith("@")) {
+ // "Legacy" behavior of '--aspects' parameter.
+ bzlFileLoadLikeString = new PathFragment("/" + bzlFileLoadLikeString).toString();
+ if (bzlFileLoadLikeString.endsWith(".bzl")) {
+ bzlFileLoadLikeString = bzlFileLoadLikeString.substring(0,
+ bzlFileLoadLikeString.length() - ".bzl".length());
+ }
+ }
+ SkylarkImport skylarkImport;
+ try {
+ skylarkImport = SkylarkImports.create(bzlFileLoadLikeString);
+ } catch (SkylarkImportSyntaxException e) {
+ throw new ViewCreationFailedException(
+ String.format("Invalid aspect '%s': %s", aspect, e.getMessage()), e);
+ }
String skylarkFunctionName = aspect.substring(delimiterPosition + 1);
for (TargetAndConfiguration targetSpec : topLevelTargetsWithConfigs) {
@@ -479,7 +497,7 @@
// aspect and the base target while the top-level configuration is untrimmed.
targetSpec.getConfiguration(),
targetSpec.getConfiguration(),
- bzlFile,
+ skylarkImport,
skylarkFunctionName));
}
} else {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 1d3b0fd..d4366fa 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -26,9 +26,8 @@
import com.google.devtools.build.lib.packages.AspectClass;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Package;
-import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.syntax.SkylarkImport;
import com.google.devtools.build.skyframe.SkyFunctionName;
-
import javax.annotation.Nullable;
/**
@@ -223,20 +222,20 @@
private final Label targetLabel;
private final BuildConfiguration aspectConfiguration;
private final BuildConfiguration targetConfiguration;
- private final PathFragment extensionFile;
+ private final SkylarkImport skylarkImport;
private final String skylarkValueName;
private SkylarkAspectLoadingKey(
Label targetLabel,
BuildConfiguration aspectConfiguration,
BuildConfiguration targetConfiguration,
- PathFragment extensionFile,
+ SkylarkImport skylarkImport,
String skylarkFunctionName) {
this.targetLabel = targetLabel;
this.aspectConfiguration = aspectConfiguration;
this.targetConfiguration = targetConfiguration;
- this.extensionFile = extensionFile;
+ this.skylarkImport = skylarkImport;
this.skylarkValueName = skylarkFunctionName;
}
@@ -245,16 +244,16 @@
return SkyFunctions.LOAD_SKYLARK_ASPECT;
}
- public PathFragment getExtensionFile() {
- return extensionFile;
+ public Label getTargetLabel() {
+ return targetLabel;
}
public String getSkylarkValueName() {
return skylarkValueName;
}
- public Label getTargetLabel() {
- return targetLabel;
+ public SkylarkImport getSkylarkImport() {
+ return skylarkImport;
}
/**
@@ -274,7 +273,35 @@
@Override
public String getDescription() {
// Skylark aspects are referred to on command line with <file>%<value ame>
- return String.format("%s%%%s of %s", extensionFile.toString(), skylarkValueName, targetLabel);
+ return String.format("%s%%%s of %s", skylarkImport.getImportString(),
+ skylarkValueName, targetLabel);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(targetLabel,
+ aspectConfiguration,
+ targetConfiguration,
+ skylarkImport,
+ skylarkValueName);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o == this) {
+ return true;
+ }
+
+ if (!(o instanceof SkylarkAspectLoadingKey)) {
+ return false;
+ }
+ SkylarkAspectLoadingKey that = (SkylarkAspectLoadingKey) o;
+ return Objects.equal(targetLabel, that.targetLabel)
+ && Objects.equal(aspectConfiguration, that.aspectConfiguration)
+ && Objects.equal(targetConfiguration, that.targetConfiguration)
+ && Objects.equal(skylarkImport, that.skylarkImport)
+ && Objects.equal(skylarkValueName, that.skylarkValueName);
+
}
}
@@ -350,9 +377,9 @@
Label targetLabel,
BuildConfiguration aspectConfiguration,
BuildConfiguration targetConfiguration,
- PathFragment skylarkFile,
+ SkylarkImport skylarkImport,
String skylarkExportName) {
return new SkylarkAspectLoadingKey(
- targetLabel, aspectConfiguration, targetConfiguration, skylarkFile, skylarkExportName);
+ targetLabel, aspectConfiguration, targetConfiguration, skylarkImport, skylarkExportName);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
index 5d2c163..4d6dbf2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
@@ -14,8 +14,9 @@
package com.google.devtools.build.lib.skyframe;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.AspectDescriptor;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
@@ -24,12 +25,11 @@
import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException;
import com.google.devtools.build.lib.skyframe.AspectValue.SkylarkAspectLoadingKey;
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
-import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.syntax.SkylarkImport;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
import javax.annotation.Nullable;
/**
@@ -47,13 +47,17 @@
throws LoadSkylarkAspectFunctionException, InterruptedException {
SkylarkAspectLoadingKey aspectLoadingKey = (SkylarkAspectLoadingKey) skyKey.argument();
String skylarkValueName = aspectLoadingKey.getSkylarkValueName();
- PathFragment extensionFile = aspectLoadingKey.getExtensionFile();
+ SkylarkImport extensionFile = aspectLoadingKey.getSkylarkImport();
// Find label corresponding to skylark file, if one exists.
- ImmutableMap<PathFragment, Label> labelLookupMap;
+ ImmutableMap<String, Label> labelLookupMap;
try {
labelLookupMap =
- SkylarkImportLookupFunction.labelsForAbsoluteImports(ImmutableSet.of(extensionFile), env);
+ SkylarkImportLookupFunction.findLabelsForLoadStatements(
+ ImmutableList.of(extensionFile),
+ Label.parseAbsoluteUnchecked("//:empty"),
+ env
+ );
} catch (SkylarkImportFailedException e) {
env.getListener().handle(Event.error(e.getMessage()));
throw new LoadSkylarkAspectFunctionException(
@@ -63,16 +67,17 @@
return null;
}
- SkylarkAspect skylarkAspect = null;
+ SkylarkAspect skylarkAspect;
+ Label extensionFileLabel = Iterables.getOnlyElement(labelLookupMap.values());
try {
skylarkAspect = AspectFunction.loadSkylarkAspect(
- env, labelLookupMap.get(extensionFile), skylarkValueName);
+ env, extensionFileLabel, skylarkValueName);
if (skylarkAspect == null) {
return null;
}
if (!skylarkAspect.getParamAttributes().isEmpty()) {
throw new AspectCreationException("Cannot instantiate parameterized aspect "
- + skylarkAspect.getName() + " at the top level.", labelLookupMap.get(extensionFile));
+ + skylarkAspect.getName() + " at the top level.", extensionFileLabel);
}
} catch (AspectCreationException e) {
throw new LoadSkylarkAspectFunctionException(e);
@@ -101,4 +106,4 @@
super(cause, Transience.PERSISTENT);
}
}
-}
+}
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java
index 95ddf96..e581436 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImport.java
@@ -17,6 +17,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.Serializable;
+import javax.annotation.Nullable;
/**
* Encapsulates the four syntactic variants of Skylark imports: Absolute paths, relative
@@ -45,7 +46,7 @@
*
* @throws IllegalStateException if this import takes the form of an absolute path.
*/
- Label getLabel(Label containingFileLabel);
+ Label getLabel(@Nullable Label containingFileLabel);
/**
* True if this import takes the form of an absolute path.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java
index 2910beb..44d4a9b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.cmdline.LabelValidator;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.Objects;
/**
* Factory class for creating appropriate instances of {@link SkylarkImports}.
@@ -31,7 +32,11 @@
// Default implementation class for SkylarkImport.
private abstract static class SkylarkImportImpl implements SkylarkImport {
- protected String importString;
+ private final String importString;
+
+ protected SkylarkImportImpl(String importString) {
+ this.importString = importString;
+ }
@Override
public String getImportString() {
@@ -53,13 +58,32 @@
public PathFragment getAbsolutePath() {
throw new IllegalStateException("can't request absolute path from a non-absolute import");
}
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(getClass(), importString);
+ }
+
+ @Override
+ public boolean equals(Object that) {
+ if (this == that) {
+ return true;
+ }
+
+ if (!(that instanceof SkylarkImportImpl)) {
+ return false;
+ }
+
+ return Objects.equals(getClass(), that.getClass())
+ && Objects.equals(importString, ((SkylarkImportImpl) that).importString);
+ }
}
private static final class AbsolutePathImport extends SkylarkImportImpl {
private final PathFragment importPath;
private AbsolutePathImport(String importString, PathFragment importPath) {
- this.importString = importString;
+ super(importString);
this.importPath = importPath;
}
@@ -82,13 +106,14 @@
public PathFragment getAbsolutePath() {
return this.importPath;
}
+
}
private static final class RelativePathImport extends SkylarkImportImpl {
private final String importFile;
private RelativePathImport(String importString, String importFile) {
- this.importString = importString;
+ super(importString);
this.importFile = importFile;
}
@@ -113,13 +138,14 @@
throw new IllegalStateException(e);
}
}
+
}
private static final class AbsoluteLabelImport extends SkylarkImportImpl {
private final Label importLabel;
private AbsoluteLabelImport(String importString, Label importLabel) {
- this.importString = importString;
+ super(importString);
this.importLabel = importLabel;
}
@@ -134,13 +160,14 @@
// to the repo of the containing file.
return containingFileLabel.resolveRepositoryRelative(importLabel);
}
+
}
private static final class RelativeLabelImport extends SkylarkImportImpl {
private final String importTarget;
private RelativeLabelImport(String importString, String importTarget) {
- this.importString = importString;
+ super(importString);
this.importTarget = importTarget;
}
@@ -161,6 +188,7 @@
throw new IllegalStateException(e);
}
}
+
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
index d053f05..53da165 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -69,33 +69,80 @@
AnalysisResult analysisResult =
update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
- assertThat(
- transform(
- analysisResult.getTargetsToBuild(),
- new Function<ConfiguredTarget, String>() {
- @Nullable
- @Override
- public String apply(ConfiguredTarget configuredTarget) {
- return configuredTarget.getLabel().toString();
- }
- }))
- .containsExactly("//test:xxx");
- assertThat(
- transform(
- analysisResult.getAspects(),
- new Function<AspectValue, String>() {
- @Nullable
- @Override
- public String apply(AspectValue aspectValue) {
- return String.format(
- "%s(%s)",
- aspectValue.getConfiguredAspect().getName(),
- aspectValue.getLabel().toString());
- }
- }))
+ assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx");
+ assertThat(getAspectDescriptions(analysisResult))
.containsExactly("//test:aspect.bzl%MyAspect(//test:xxx)");
}
+ private Iterable<String> getAspectDescriptions(AnalysisResult analysisResult) {
+ return transform(
+ analysisResult.getAspects(),
+ new Function<AspectValue, String>() {
+ @Nullable
+ @Override
+ public String apply(AspectValue aspectValue) {
+ return String.format(
+ "%s(%s)",
+ aspectValue.getConfiguredAspect().getName(),
+ aspectValue.getLabel().toString());
+ }
+ });
+ }
+
+ @Test
+ public void testAspectCommandLineLabel() throws Exception {
+ scratch.file(
+ "test/aspect.bzl",
+ "def _impl(target, ctx):",
+ " print('This aspect does nothing')",
+ " return struct()",
+ "MyAspect = aspect(implementation=_impl)");
+ scratch.file("test/BUILD", "java_library(name = 'xxx',)");
+
+ AnalysisResult analysisResult =
+ update(ImmutableList.of("//test:aspect.bzl%MyAspect"), "//test:xxx");
+ assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx");
+ assertThat(getAspectDescriptions(analysisResult))
+ .containsExactly("//test:aspect.bzl%MyAspect(//test:xxx)");
+ }
+
+ @Test
+ public void testAspectCommandLineRepoLabel() throws Exception {
+ scratch.overwriteFile(
+ "WORKSPACE",
+ scratch.readFile("WORKSPACE"),
+ "local_repository(name='local', path='local/repo')"
+ );
+ scratch.file(
+ "local/repo/aspect.bzl",
+ "def _impl(target, ctx):",
+ " print('This aspect does nothing')",
+ " return struct()",
+ "MyAspect = aspect(implementation=_impl)");
+ scratch.file("local/repo/BUILD");
+
+ scratch.file("test/BUILD", "java_library(name = 'xxx',)");
+
+ AnalysisResult analysisResult =
+ update(ImmutableList.of("@local//:aspect.bzl%MyAspect"), "//test:xxx");
+ assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx");
+ assertThat(getAspectDescriptions(analysisResult))
+ .containsExactly("@local//:aspect.bzl%MyAspect(//test:xxx)");
+ }
+
+ private Iterable<String> getLabelsToBuild(AnalysisResult analysisResult) {
+ return transform(
+ analysisResult.getTargetsToBuild(),
+ new Function<ConfiguredTarget, String>() {
+ @Nullable
+ @Override
+ public String apply(ConfiguredTarget configuredTarget) {
+ return configuredTarget.getLabel().toString();
+ }
+ });
+ }
+
+
@Test
public void testAspectAllowsFragmentsToBeSpecified() throws Exception {
scratch.file(
@@ -157,17 +204,7 @@
AnalysisResult analysisResult =
update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
- assertThat(
- transform(
- analysisResult.getTargetsToBuild(),
- new Function<ConfiguredTarget, String>() {
- @Nullable
- @Override
- public String apply(ConfiguredTarget configuredTarget) {
- return configuredTarget.getLabel().toString();
- }
- }))
- .containsExactly("//test:xxx");
+ assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx");
AspectValue aspectValue = analysisResult.getAspects().iterator().next();
SkylarkProviders skylarkProviders =
aspectValue.getConfiguredAspect().getProvider(SkylarkProviders.class);
@@ -270,17 +307,7 @@
AnalysisResult analysisResult =
update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx");
- assertThat(
- transform(
- analysisResult.getTargetsToBuild(),
- new Function<ConfiguredTarget, String>() {
- @Nullable
- @Override
- public String apply(ConfiguredTarget configuredTarget) {
- return configuredTarget.getLabel().toString();
- }
- }))
- .containsExactly("//test:xxx");
+ assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx");
AspectValue aspectValue = analysisResult.getAspects().iterator().next();
OutputGroupProvider outputGroupProvider =
aspectValue.getConfiguredAspect().getProvider(OutputGroupProvider.class);
@@ -331,17 +358,7 @@
")");
AnalysisResult analysisResult = update("//test:xxx");
- assertThat(
- transform(
- analysisResult.getTargetsToBuild(),
- new Function<ConfiguredTarget, String>() {
- @Nullable
- @Override
- public String apply(ConfiguredTarget configuredTarget) {
- return configuredTarget.getLabel().toString();
- }
- }))
- .containsExactly("//test:xxx");
+ assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:xxx");
ConfiguredTarget target = analysisResult.getTargetsToBuild().iterator().next();
SkylarkProviders skylarkProviders = target.getProvider(SkylarkProviders.class);
assertThat(skylarkProviders).isNotNull();
@@ -428,7 +445,7 @@
assertThat(result.hasError()).isTrue();
} catch (ViewCreationFailedException expected) {
assertThat(expected.getMessage())
- .contains("Analysis of aspect '/test/aspect.bzl%MyAspect of //test:xxx' failed");
+ .contains("Analysis of aspect '/test/aspect%MyAspect of //test:xxx' failed");
}
assertContainsEvent("//test:aspect.bzl%MyAspect is attached to source file zzz.jar but "
+ "aspects must be attached to rules");
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java b/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java
index 7179b37..6004661 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/Scratch.java
@@ -14,13 +14,13 @@
package com.google.devtools.build.lib.testutil;
+import com.google.common.io.ByteStreams;
import com.google.devtools.build.lib.util.BlazeClock;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
@@ -131,6 +131,12 @@
return file;
}
+ public String readFile(String pathName) throws IOException {
+ return new String(
+ ByteStreams.toByteArray(resolve(pathName).getInputStream()),
+ DEFAULT_CHARSET);
+ }
+
/**
* Like {@code scratch.file}, but the file is first deleted if it already
* exists.
