From d86b509d306b18438910326da072aa5f8b823c9e Mon Sep 17 00:00:00 2001
From: nharmata <nharmata@google.com>
Date: Tue, 16 Oct 2018 15:50:21 -0700
Subject: [PATCH] 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
---
site/docs/skylark/backward-compatibility.md | 21 +++
.../devtools/build/lib/cmdline/Label.java | 17 ++
.../lib/packages/SkylarkSemanticsOptions.java | 15 ++
.../lib/skyframe/ASTFileLookupFunction.java | 3 +
.../ContainingPackageLookupValue.java | 36 ++++
.../build/lib/skyframe/PackageFunction.java | 49 +-----
.../skyframe/SkylarkImportLookupFunction.java | 42 +++++
.../build/lib/syntax/SkylarkSemantics.java | 5 +
.../devtools/build/lib/cmdline/LabelTest.java | 12 ++
.../SkylarkSemanticsConsistencyTest.java | 2 +
.../SkylarkImportLookupFunctionTest.java | 158 ++++++++++++++++++
.../shell/integration/loading_phase_test.sh | 23 +++
12 files changed, 343 insertions(+), 40 deletions(-)
diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md
index 536b4e0d055f6b..28592bb0fa2a39 100644
--- a/site/docs/skylark/backward-compatibility.md
+++ b/site/docs/skylark/backward-compatibility.md
@@ -58,6 +58,7 @@ guarded behind flags in the current release:
* [Disable legacy C++ configuration API](#disable-legacy-c-configuration-api)
* [Disable legacy C++ toolchain API](#disable-legacy-c-toolchain-api)
* [Disallow `cfg = "data"`](#disallow-cfg--data)
+* [Load label cannot cross package boundaries](#load-label-cannot-cross-package-boundaries)
### Dictionary concatenation
@@ -956,4 +957,24 @@ fail with an error.
* Default: `false`
* Introduced in: `0.16.0`
+
+### Load label cannot cross package boundaries
+
+Previously, the label argument to the `load` statement (the first argument) was
+checked to ensure that it referenced an existing package but it was not checked
+to ensure that it didn't cross a package boundary.
+
+For example, in
+
+```python
+load('//a:b/c.bzl', 'doesntmatter')
+```
+
+if this flag is set to `true`, the above statement will be in error if `//a/b`
+is a package; in such a case, the correct way to reference `c.bzl` via a label
+would be `//a/b:c.bzl`.
+
+* Flag: `--incompatible_disallow_load_labels_to_cross_package_boundaries`
+* Default: `false`
+
<!-- Add new options here -->
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 43d984628b3fe6..ecde5243512226 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 @@ public static String print(@Nullable Label label) {
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 42da1f3b6728d8..4af330c7c7ef7f 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
@@ -264,6 +264,19 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
)
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",
@@ -520,6 +533,8 @@ public SkylarkSemantics toSkylarkSemantics() {
.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 bbf75110783a9b..93aa4545548d5c 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionExcept
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 725d20193a3d1a..c9a341e42ade7e 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 @@ public static Key key(PackageIdentifier id) {
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 da528dc915b485..4a0543feeb9110 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 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
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 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
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 @@ private static boolean maybeAddEventAboutLabelCrossingSubpackage(
// 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 e16b5e987d027e..dd1e50af1ca30e 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 @@ private SkylarkImportLookupValue computeInternal(
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 @@ static SkylarkImportFailedException noBuildFile(PathFragment file) {
+ " 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 d6eb28e144c5a8..9fba5285f394c9 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 boolean isFeatureEnabledBasedOnTogglingFlags(
public abstract boolean incompatibleDisallowLegacyJavaInfo();
+ public abstract boolean incompatibleDisallowLoadLabelsToCrossPackageBoundaries();
+
public abstract boolean incompatibleDisallowOldStyleArgsAdd();
public abstract boolean incompatibleDisallowSlashOperator();
@@ -189,6 +191,7 @@ public static Builder builderWithDefaults() {
.incompatibleDisallowDictPlus(false)
.incompatibleDisallowFileType(false)
.incompatibleDisallowLegacyJavaInfo(false)
+ .incompatibleDisallowLoadLabelsToCrossPackageBoundaries(false)
.incompatibleDisallowOldStyleArgsAdd(false)
.incompatibleDisallowSlashOperator(false)
.incompatibleExpandDirectories(false)
@@ -245,6 +248,8 @@ public abstract static class Builder {
public abstract Builder incompatibleDisallowLegacyJavaInfo(boolean value);
+ public abstract Builder incompatibleDisallowLoadLabelsToCrossPackageBoundaries(boolean value);
+
public abstract Builder incompatibleDisallowOldStyleArgsAdd(boolean value);
public abstract Builder incompatibleDisallowSlashOperator(boolean value);
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
index fdfb30bdb8ce66..6ea333fa0fff3d 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
@@ -469,4 +469,16 @@ public void testGetWorkspaceRoot() throws Exception {
label = Label.parseAbsolute("@repo//bar/baz", ImmutableMap.of());
assertThat(label.getWorkspaceRoot()).isEqualTo("external/repo");
}
+
+ @Test
+ public void testGetContainingDirectory() {
+ assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a:b")))
+ .isEqualTo(PathFragment.create("a"));
+ assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a/b:c")))
+ .isEqualTo(PathFragment.create("a/b"));
+ assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a:b/c")))
+ .isEqualTo(PathFragment.create("a/b"));
+ assertThat(Label.getContainingDirectory(Label.parseAbsoluteUnchecked("//a/b/c")))
+ .isEqualTo(PathFragment.create("a/b/c"));
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
index 8246bf7f745608..661ab406739837 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -137,6 +137,7 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex
"--incompatible_disallow_dict_plus=" + rand.nextBoolean(),
"--incompatible_disallow_filetype=" + rand.nextBoolean(),
"--incompatible_disallow_legacy_javainfo=" + rand.nextBoolean(),
+ "--incompatible_disallow_load_labels_to_cross_package_boundaries=" + rand.nextBoolean(),
"--incompatible_disallow_old_style_args_add=" + rand.nextBoolean(),
"--incompatible_disallow_slash_operator=" + rand.nextBoolean(),
"--incompatible_expand_directories=" + rand.nextBoolean(),
@@ -180,6 +181,7 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) {
.incompatibleDisallowDictPlus(rand.nextBoolean())
.incompatibleDisallowFileType(rand.nextBoolean())
.incompatibleDisallowLegacyJavaInfo(rand.nextBoolean())
+ .incompatibleDisallowLoadLabelsToCrossPackageBoundaries(rand.nextBoolean())
.incompatibleDisallowOldStyleArgsAdd(rand.nextBoolean())
.incompatibleDisallowSlashOperator(rand.nextBoolean())
.incompatibleExpandDirectories(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
index dc61c61df4d3e8..93cf9e8324359f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunctionTest.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.skyframe;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult;
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
@@ -25,6 +26,7 @@
import com.google.devtools.build.lib.packages.SkylarkSemanticsOptions;
import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
+import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.Path;
@@ -262,4 +264,160 @@ public void testLoadFromExternalRepoInWorkspaceFileAllowed() throws Exception {
assertThat(result.hasError()).isFalse();
}
+
+ @Test
+ public void testLoadUsingLabelThatDoesntCrossesBoundaryOfPackage()
+ throws Exception {
+ scratch.file("a/BUILD");
+ scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')");
+ scratch.file("a/b/b.bzl", "b = 42");
+
+ checkSuccessfulLookup("//a:a.bzl");
+ }
+
+ @Test
+ public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Allow_OfSamePkg()
+ throws Exception {
+ scratch.file("a/BUILD");
+ scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')");
+ scratch.file("a/b/BUILD", "");
+ scratch.file("a/b/b.bzl", "b = 42");
+
+ checkSuccessfulLookup("//a:a.bzl");
+ }
+
+ @Test
+ public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfSamePkg()
+ throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_load_labels_to_cross_package_boundaries");
+
+ scratch.file("a/BUILD");
+ scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')");
+ scratch.file("a/b/BUILD", "");
+ scratch.file("a/b/b.bzl", "b = 42");
+
+ SkyKey skylarkImportLookupKey =
+ SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a:a.bzl"), false);
+ EvaluationResult<SkylarkImportLookupValue> result =
+ SkyframeExecutorTestUtils.evaluate(
+ getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter);
+ assertThat(result.hasError()).isTrue();
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .isInstanceOf(SkylarkImportFailedException.class);
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .hasMessageThat()
+ .contains(
+ "Label '//a:b/b.bzl' crosses boundary of subpackage 'a/b' (perhaps you meant to put "
+ + "the colon here: '//a/b:b.bzl'?)");
+ }
+
+ @Test
+ public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Allow_OfDifferentPkgUnder()
+ throws Exception {
+ scratch.file("a/BUILD");
+ scratch.file("a/a.bzl", "load('//a/b:c/c.bzl', 'c')");
+ scratch.file("a/b/BUILD", "");
+ scratch.file("a/b/c/BUILD", "");
+ scratch.file("a/b/c/c.bzl", "c = 42");
+
+ checkSuccessfulLookup("//a:a.bzl");
+ }
+
+ @Test
+ public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfDifferentPkgUnder()
+ throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_load_labels_to_cross_package_boundaries");
+
+ scratch.file("a/BUILD");
+ scratch.file("a/a.bzl", "load('//a/b:c/c.bzl', 'c')");
+ scratch.file("a/b/BUILD", "");
+ scratch.file("a/b/c/BUILD", "");
+ scratch.file("a/b/c/c.bzl", "c = 42");
+
+ SkyKey skylarkImportLookupKey =
+ SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a:a.bzl"), false);
+ EvaluationResult<SkylarkImportLookupValue> result =
+ SkyframeExecutorTestUtils.evaluate(
+ getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter);
+ assertThat(result.hasError()).isTrue();
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .isInstanceOf(SkylarkImportFailedException.class);
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .hasMessageThat()
+ .contains(
+ "Label '//a/b:c/c.bzl' crosses boundary of subpackage 'a/b/c' (perhaps you meant to "
+ + "put the colon here: '//a/b/c:c.bzl'?)");
+ }
+
+ @Test
+ public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Allow_OfDifferentPkgAbove()
+ throws Exception {
+ scratch.file("a/b/BUILD");
+ scratch.file("a/b/b.bzl", "load('//a/c:c/c.bzl', 'c')");
+ scratch.file("a/BUILD");
+ scratch.file("a/c/c/c.bzl", "c = 42");
+
+ // With the default of
+ // --incompatible_disallow_load_labels_to_cross_subpackage_boundaries=false,
+ // SkylarkImportLookupValue(//a/b:b.bzl) has an error because ASTFileLookupValue(//a/c:c/c.bzl)
+ // because package //a/c doesn't exist. The behavior with
+ // --incompatible_disallow_load_labels_to_cross_subpackage_boundaries=true is stricter, but we
+ // still have an explicit test for this case so that way we don't forget to think about it when
+ // we address the TODO in ASTFuleLookupFunction.
+
+ SkyKey skylarkImportLookupKey =
+ SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a/b:b.bzl"), false);
+ EvaluationResult<SkylarkImportLookupValue> result =
+ SkyframeExecutorTestUtils.evaluate(
+ getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter);
+ assertThat(result.hasError()).isTrue();
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .isInstanceOf(SkylarkImportFailedException.class);
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .hasMessageThat()
+ .contains(
+ "Extension file not found. Unable to load package for '//a/c:c/c.bzl': BUILD file not "
+ + "found on package path");
+ }
+
+ @Test
+ public void testLoadUsingLabelThatCrossesBoundaryOfPackage_Disallow_OfDifferentPkgAbove()
+ throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_disallow_load_labels_to_cross_package_boundaries");
+
+ scratch.file("a/b/BUILD");
+ scratch.file("a/b/b.bzl", "load('//a/c:c/c.bzl', 'c')");
+ scratch.file("a/BUILD");
+ scratch.file("a/c/c/c.bzl", "c = 42");
+
+ SkyKey skylarkImportLookupKey =
+ SkylarkImportLookupValue.key(Label.parseAbsoluteUnchecked("//a/b:b.bzl"), false);
+ EvaluationResult<SkylarkImportLookupValue> result =
+ SkyframeExecutorTestUtils.evaluate(
+ getSkyframeExecutor(), skylarkImportLookupKey, /*keepGoing=*/ false, reporter);
+ assertThat(result.hasError()).isTrue();
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .isInstanceOf(SkylarkImportFailedException.class);
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(skylarkImportLookupKey)
+ .hasExceptionThat()
+ .hasMessageThat()
+ .contains(
+ "Label '//a/c:c/c.bzl' crosses boundary of package 'a' (perhaps you meant to put the "
+ + "colon here: '//a:c/c/c.bzl'?)");
+ }
}
diff --git a/src/test/shell/integration/loading_phase_test.sh b/src/test/shell/integration/loading_phase_test.sh
index 5e1d76642c2f41..99bb0c2be5ea90 100755
--- a/src/test/shell/integration/loading_phase_test.sh
+++ b/src/test/shell/integration/loading_phase_test.sh
@@ -348,4 +348,27 @@ function test_no_package_loading_on_benign_workspace_file_changes() {
expect_log "//$pkg/foo:shname2"
}
+function test_incompatible_disallow_load_labels_to_cross_package_boundaries() {
+ local -r pkg="${FUNCNAME}"
+ mkdir -p "$pkg" || fail "could not create \"$pkg\""
+
+ mkdir "$pkg"/foo
+ echo "load(\"//$pkg/foo/a:b/b.bzl\", \"b\")" > "$pkg"/foo/BUILD
+ mkdir -p "$pkg"/foo/a/b
+ touch "$pkg"/foo/a/BUILD
+ touch "$pkg"/foo/a/b/BUILD
+ echo "b = 42" > "$pkg"/foo/a/b/b.bzl
+
+ bazel query "$pkg/foo:BUILD" >& "$TEST_log" || fail "Expected success"
+ expect_log "//$pkg/foo:BUILD"
+
+ bazel query \
+ --incompatible_disallow_load_labels_to_cross_package_boundaries=true \
+ "$pkg/foo:BUILD" >& "$TEST_log" && fail "Expected failure"
+ expect_log "Label '//$pkg/foo/a:b/b.bzl' crosses boundary of subpackage '$pkg/foo/a/b'"
+
+ bazel query "$pkg/foo:BUILD" >& "$TEST_log" || fail "Expected success"
+ expect_log "//$pkg/foo:BUILD"
+}
+
run_suite "Integration tests of ${PRODUCT_NAME} using loading/analysis phases."