Error-out if the label in a 'load' statement crosses a subpackage boundary. This behavior is guarded by a flag (default value is "don't error out"), and will eventually become the only behavior.
This implements the feature in #6408.
RELNOTES: None
PiperOrigin-RevId: 217402217
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 43d9846..ecde524 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
@@ -639,6 +639,23 @@
return label == null ? "(unknown)" : label.toString();
}
+ /**
+ * Returns a {@link PathFragment} corresponding to the directory in which {@code label} would
+ * reside, if it were interpreted to be a path.
+ */
+ public static PathFragment getContainingDirectory(Label label) {
+ PathFragment pkg = label.getPackageFragment();
+ String name = label.getName();
+ if (name.equals(".")) {
+ return pkg;
+ }
+ if (PathFragment.isNormalizedRelativePath(name) && !PathFragment.containsSeparator(name)) {
+ // Optimize for the common case of a label like '//pkg:target'.
+ return pkg;
+ }
+ return pkg.getRelative(name).getParentDirectory();
+ }
+
@Override
public boolean isImmutable() {
return true;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
index 42da1f3..4af330c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -265,6 +265,19 @@
public boolean incompatibleDisallowLegacyJavaInfo;
@Option(
+ name = "incompatible_disallow_load_labels_to_cross_package_boundaries",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
+ effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+ metadataTags = {
+ OptionMetadataTag.INCOMPATIBLE_CHANGE,
+ OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+ },
+ help = "If set to true, the label argument to 'load' cannot cross a package boundary."
+ )
+ public boolean incompatibleDisallowLoadLabelsToCrossPackageBoundaries;
+
+ @Option(
name = "incompatible_generate_javacommon_source_jar",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.SKYLARK_SEMANTICS,
@@ -520,6 +533,8 @@
.incompatibleDisallowDictPlus(incompatibleDisallowDictPlus)
.incompatibleDisallowFileType(incompatibleDisallowFileType)
.incompatibleDisallowLegacyJavaInfo(incompatibleDisallowLegacyJavaInfo)
+ .incompatibleDisallowLoadLabelsToCrossPackageBoundaries(
+ incompatibleDisallowLoadLabelsToCrossPackageBoundaries)
.incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd)
.incompatibleDisallowSlashOperator(incompatibleDisallowSlashOperator)
.incompatibleExpandDirectories(incompatibleExpandDirectories)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index bbf7511..93aa454 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -59,6 +59,9 @@
PathFragment filePathFragment = fileLabel.toPathFragment();
// Determine whether the package designated by fileLabel exists.
+ // TODO(bazel-team): After --incompatible_disallow_load_labels_to_cross_package_boundaries is
+ // removed and the new behavior is unconditional, we can instead safely assume the package
+ // exists and pass in the Root in the SkyKey and therefore this dep can be removed.
SkyKey pkgSkyKey = PackageLookupValue.key(fileLabel.getPackageIdentifier());
PackageLookupValue pkgLookupValue = null;
try {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
index 725d201..c9a341e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
@@ -15,9 +15,11 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.Interner;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.skyframe.AbstractSkyKey;
import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -47,6 +49,40 @@
return Key.create(id);
}
+ static String getErrorMessageForLabelCrossingPackageBoundary(
+ Root pkgRoot,
+ Label label,
+ ContainingPackageLookupValue containingPkgLookupValue) {
+ PackageIdentifier containingPkg = containingPkgLookupValue.getContainingPackageName();
+ boolean crossesPackageBoundaryBelow =
+ containingPkg.getSourceRoot().startsWith(label.getPackageIdentifier().getSourceRoot());
+ PathFragment labelNameFragment = PathFragment.create(label.getName());
+ String message = String.format("Label '%s' crosses boundary of %spackage '%s'",
+ label,
+ crossesPackageBoundaryBelow ? "sub" : "",
+ containingPkg);
+ Root containingRoot = containingPkgLookupValue.getContainingPackageRoot();
+ if (pkgRoot.equals(containingRoot)) {
+ PathFragment containingPkgFragment = containingPkg.getPackageFragment();
+ PathFragment labelNameInContainingPackage =
+ crossesPackageBoundaryBelow
+ ? labelNameFragment.subFragment(
+ containingPkgFragment.segmentCount()
+ - label.getPackageFragment().segmentCount(),
+ labelNameFragment.segmentCount())
+ : label.toPathFragment().relativeTo(containingPkgFragment);
+ message += " (perhaps you meant to put the colon here: '";
+ if (containingPkg.getRepository().isDefault() || containingPkg.getRepository().isMain()) {
+ message += "//";
+ }
+ message += containingPkg + ":" + labelNameInContainingPackage + "'?)";
+ } else {
+ message += " (have you deleted " + containingPkg + "/BUILD? "
+ + "If so, use the --deleted_packages=" + containingPkg + " option)";
+ }
+ return message;
+ }
+
@AutoCodec.VisibleForSerialization
@AutoCodec
static class Key extends AbstractSkyKey<PackageIdentifier> {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index da528dc..4a0543f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -675,13 +675,7 @@
Set<SkyKey> containingPkgLookupKeys = Sets.newHashSet();
Map<Target, SkyKey> targetToKey = new HashMap<>();
for (Target target : pkgBuilder.getTargets()) {
- PathFragment dir = getContainingDirectory(target.getLabel());
- if (dir == null) {
- throw new IllegalStateException(
- String.format(
- "Null pkg for label %s as path fragment %s in pkg %s",
- target.getLabel(), target.getLabel().getPackageFragment(), pkgId));
- }
+ PathFragment dir = Label.getContainingDirectory(target.getLabel());
if (dir.equals(pkgDir)) {
continue;
}
@@ -707,27 +701,18 @@
ContainingPackageLookupValue containingPackageLookupValue =
getContainingPkgLookupValueAndPropagateInconsistentFilesystemExceptions(
pkgId, containingPkgLookupValues.get(key), env);
- if (maybeAddEventAboutLabelCrossingSubpackage(pkgBuilder, pkgRoot, target.getLabel(),
- target.getLocation(), containingPackageLookupValue)) {
+ if (maybeAddEventAboutLabelCrossingSubpackage(
+ pkgBuilder,
+ pkgRoot,
+ target.getLabel(),
+ target.getLocation(),
+ containingPackageLookupValue)) {
pkgBuilder.removeTarget(target);
pkgBuilder.setContainsErrors();
}
}
}
- private static PathFragment getContainingDirectory(Label label) {
- PathFragment pkg = label.getPackageFragment();
- String name = label.getName();
- if (name.equals(".")) {
- return pkg;
- }
- if (PathFragment.isNormalizedRelativePath(name) && !PathFragment.containsSeparator(name)) {
- // Optimize for the common case of a label like '//pkg:target'.
- return pkg;
- }
- return pkg.getRelative(name).getParentDirectory();
- }
-
@Nullable
private static ContainingPackageLookupValue
getContainingPkgLookupValueAndPropagateInconsistentFilesystemExceptions(
@@ -774,24 +759,8 @@
// exceptions), it reaches here, and we tolerate it.
return false;
}
- PathFragment labelNameFragment = PathFragment.create(label.getName());
- String message = String.format("Label '%s' crosses boundary of subpackage '%s'",
- label, containingPkg);
- Root containingRoot = containingPkgLookupValue.getContainingPackageRoot();
- if (pkgRoot.equals(containingRoot)) {
- PathFragment labelNameInContainingPackage = labelNameFragment.subFragment(
- containingPkg.getPackageFragment().segmentCount()
- - label.getPackageFragment().segmentCount(),
- labelNameFragment.segmentCount());
- message += " (perhaps you meant to put the colon here: '";
- if (containingPkg.getRepository().isDefault() || containingPkg.getRepository().isMain()) {
- message += "//";
- }
- message += containingPkg + ":" + labelNameInContainingPackage + "'?)";
- } else {
- message += " (have you deleted " + containingPkg + "/BUILD? "
- + "If so, use the --deleted_packages=" + containingPkg + " option)";
- }
+ String message = ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
+ pkgRoot, label, containingPkgLookupValue);
pkgBuilder.addEvent(Event.error(location, message));
return true;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index e16b5e9..dd1e50a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -207,6 +207,34 @@
return null;
}
+ if (skylarkSemantics.incompatibleDisallowLoadLabelsToCrossPackageBoundaries()) {
+ PathFragment dir = Label.getContainingDirectory(fileLabel);
+ PackageIdentifier dirId =
+ PackageIdentifier.create(fileLabel.getPackageIdentifier().getRepository(), dir);
+ ContainingPackageLookupValue containingPackageLookupValue;
+ try {
+ containingPackageLookupValue = (ContainingPackageLookupValue) env.getValueOrThrow(
+ ContainingPackageLookupValue.key(dirId),
+ BuildFileNotFoundException.class,
+ InconsistentFilesystemException.class);
+ } catch (BuildFileNotFoundException e) {
+ throw SkylarkImportFailedException.errorReadingFile(
+ fileLabel.toPathFragment(),
+ new ErrorReadingSkylarkExtensionException(e));
+ }
+ if (containingPackageLookupValue == null) {
+ return null;
+ }
+ if (!containingPackageLookupValue.hasContainingPackage()) {
+ throw SkylarkImportFailedException.noBuildFile(fileLabel.toPathFragment());
+ }
+ if (!containingPackageLookupValue.getContainingPackageName().equals(
+ fileLabel.getPackageIdentifier())) {
+ throw SkylarkImportFailedException.labelCrossesPackageBoundary(
+ fileLabel, containingPackageLookupValue);
+ }
+ }
+
// Load the AST corresponding to this file.
ASTFileLookupValue astLookupValue;
try {
@@ -534,6 +562,20 @@
+ " Note that this BUILD file does not need to do anything except exist.", file));
}
+ static SkylarkImportFailedException labelCrossesPackageBoundary(
+ Label fileLabel,
+ ContainingPackageLookupValue containingPackageLookupValue) {
+ return new SkylarkImportFailedException(
+ ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
+ // We don't actually know the proper Root to pass in here (since we don't e.g. know
+ // the root of the bzl/BUILD file that is trying to load 'fileLabel'). Therefore we
+ // just pass in the Root of the containing package in order to still get a useful
+ // error message for the user.
+ containingPackageLookupValue.getContainingPackageRoot(),
+ fileLabel,
+ containingPackageLookupValue));
+ }
+
static SkylarkImportFailedException skylarkErrors(PathFragment file) {
return new SkylarkImportFailedException(String.format("Extension '%s' has errors", file));
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
index d6eb28e..9fba528 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -124,6 +124,8 @@
public abstract boolean incompatibleDisallowLegacyJavaInfo();
+ public abstract boolean incompatibleDisallowLoadLabelsToCrossPackageBoundaries();
+
public abstract boolean incompatibleDisallowOldStyleArgsAdd();
public abstract boolean incompatibleDisallowSlashOperator();
@@ -189,6 +191,7 @@
.incompatibleDisallowDictPlus(false)
.incompatibleDisallowFileType(false)
.incompatibleDisallowLegacyJavaInfo(false)
+ .incompatibleDisallowLoadLabelsToCrossPackageBoundaries(false)
.incompatibleDisallowOldStyleArgsAdd(false)
.incompatibleDisallowSlashOperator(false)
.incompatibleExpandDirectories(false)
@@ -245,6 +248,8 @@
public abstract Builder incompatibleDisallowLegacyJavaInfo(boolean value);
+ public abstract Builder incompatibleDisallowLoadLabelsToCrossPackageBoundaries(boolean value);
+
public abstract Builder incompatibleDisallowOldStyleArgsAdd(boolean value);
public abstract Builder incompatibleDisallowSlashOperator(boolean value);
