Allow/require callers of AbstractPackageLoader to set Skylark semantics explicitly
Previously the default semantics were used unconditionally. Allowing non-default semantics is a feature. Requiring semantics to be specified explicitly helps to avoid unintentional divergence from the caller's intended semantics. We recently did the same thing for Skylark's Environment.Builder (https://github.com/bazelbuild/bazel/commit/b368b39f8ba1e8e8a67af50e5ade9127b2b149d7).
Also pass Skylark semantics through Package.Builder.Helper, so that the extra verification done for shell tests uses the same semantics as the build.
RELNOTES: None
PiperOrigin-RevId: 173544885
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index c6c5d03..4019e87 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -34,6 +34,7 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.AttributeMap.AcceptsLabelAttribute;
import com.google.devtools.build.lib.packages.License.DistributionType;
+import com.google.devtools.build.lib.syntax.SkylarkSemantics;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.SpellChecker;
import com.google.devtools.build.lib.vfs.Canonicalizer;
@@ -713,9 +714,10 @@
/**
* Called after {@link com.google.devtools.build.lib.skyframe.PackageFunction} is completely
- * done loading the given {@link Package}.
+ * done loading the given {@link Package}. {@code skylarkSemantics} are the semantics used to
+ * evaluate the build.
*/
- void onLoadingComplete(Package pkg);
+ void onLoadingComplete(Package pkg, SkylarkSemantics skylarkSemantics);
}
/** {@link Helper} that simply calls the {@link Package} constructor. */
@@ -731,7 +733,7 @@
}
@Override
- public void onLoadingComplete(Package pkg) {
+ public void onLoadingComplete(Package pkg, SkylarkSemantics skylarkSemantics) {
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 015ac01..e144aa8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -1607,8 +1607,8 @@
* Called by a caller of {@link #createPackageFromPreprocessingAst} after this caller has fully
* loaded the package.
*/
- public void afterDoneLoadingPackage(Package pkg) {
- packageBuilderHelper.onLoadingComplete(pkg);
+ public void afterDoneLoadingPackage(Package pkg, SkylarkSemantics skylarkSemantics) {
+ packageBuilderHelper.onLoadingComplete(pkg, skylarkSemantics);
}
/**
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 c5d7232..4893ac0 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
@@ -423,6 +423,10 @@
*/
private SkyValue getExternalPackage(Environment env, Path packageLookupPath)
throws PackageFunctionException, InterruptedException {
+ SkylarkSemantics skylarkSemantics = PrecomputedValue.SKYLARK_SEMANTICS.get(env);
+ if (skylarkSemantics == null) {
+ return null;
+ }
RootedPath workspacePath = RootedPath.toRootedPath(
packageLookupPath, Label.EXTERNAL_PACKAGE_FILE_NAME);
SkyKey workspaceKey = ExternalPackageFunction.key(workspacePath);
@@ -458,7 +462,7 @@
}
if (packageFactory != null) {
- packageFactory.afterDoneLoadingPackage(pkg);
+ packageFactory.afterDoneLoadingPackage(pkg, skylarkSemantics);
}
return new PackageValue(pkg);
}
@@ -621,7 +625,7 @@
// We know this SkyFunction will not be called again, so we can remove the cache entry.
packageFunctionCache.invalidate(packageId);
- packageFactory.afterDoneLoadingPackage(pkg);
+ packageFactory.afterDoneLoadingPackage(pkg, skylarkSemantics);
return new PackageValue(pkg);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 9ea8e98..3462284 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -111,6 +111,7 @@
};
private final Reporter reporter;
protected final RuleClassProvider ruleClassProvider;
+ protected SkylarkSemantics skylarkSemantics;
protected final ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions;
protected final AtomicReference<PathPackageLocator> pkgLocatorRef;
protected final ExternalFilesHelper externalFilesHelper;
@@ -124,6 +125,7 @@
protected final Path workspaceDir;
protected final BlazeDirectories directories;
protected RuleClassProvider ruleClassProvider = getDefaultRuleClassProvider();
+ protected SkylarkSemantics skylarkSemantics;
protected Reporter reporter = new Reporter(new EventBus());
protected Map<SkyFunctionName, SkyFunction> extraSkyFunctions = new HashMap<>();
protected List<PrecomputedValue.Injected> extraPrecomputedValues = new ArrayList<>();
@@ -146,6 +148,16 @@
return this;
}
+ public Builder setSkylarkSemantics(SkylarkSemantics semantics) {
+ this.skylarkSemantics = semantics;
+ return this;
+ }
+
+ public Builder useDefaultSkylarkSemantics() {
+ this.skylarkSemantics = SkylarkSemantics.DEFAULT_SEMANTICS;
+ return this;
+ }
+
public Builder setReporter(Reporter reporter) {
this.reporter = reporter;
return this;
@@ -177,7 +189,20 @@
return this;
}
- public abstract PackageLoader build();
+ /** Throws {@link IllegalArgumentException} if builder args are incomplete/inconsistent. */
+ protected void validate() {
+ if (skylarkSemantics == null) {
+ throw new IllegalArgumentException(
+ "must call either setSkylarkSemantics or useDefaultSkylarkSemantics");
+ }
+ }
+
+ public final PackageLoader build() {
+ validate();
+ return buildImpl();
+ }
+
+ protected abstract PackageLoader buildImpl();
protected abstract RuleClassProvider getDefaultRuleClassProvider();
@@ -189,6 +214,7 @@
PathPackageLocator pkgLocator =
new PathPackageLocator(null, ImmutableList.of(workspaceDir));
this.ruleClassProvider = builder.ruleClassProvider;
+ this.skylarkSemantics = builder.skylarkSemantics;
this.reporter = builder.reporter;
this.extraSkyFunctions = ImmutableMap.copyOf(builder.extraSkyFunctions);
this.pkgLocatorRef = new AtomicReference<>(pkgLocator);
@@ -210,12 +236,14 @@
};
this.preinjectedDiff =
makePreinjectedDiff(
+ skylarkSemantics,
pkgLocator,
builder.defaultsPackageContents,
ImmutableList.copyOf(builder.extraPrecomputedValues));
}
private static ImmutableDiff makePreinjectedDiff(
+ SkylarkSemantics skylarkSemantics,
PathPackageLocator pkgLocator,
String defaultsPackageContents,
ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues) {
@@ -237,7 +265,7 @@
}
PrecomputedValue.PATH_PACKAGE_LOCATOR.set(injectable, pkgLocator);
PrecomputedValue.DEFAULT_VISIBILITY.set(injectable, ConstantRuleVisibility.PRIVATE);
- PrecomputedValue.SKYLARK_SEMANTICS.set(injectable, SkylarkSemantics.DEFAULT_SEMANTICS);
+ PrecomputedValue.SKYLARK_SEMANTICS.set(injectable, skylarkSemantics);
PrecomputedValue.DEFAULTS_PACKAGE_CONTENTS.set(injectable, defaultsPackageContents);
PrecomputedValue.BLACKLISTED_PACKAGE_PREFIXES_FILE.set(injectable, PathFragment.EMPTY_FRAGMENT);
return new ImmutableDiff(ImmutableList.<SkyKey>of(), valuesToInject);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
index 159b69b..e32a18a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
@@ -81,7 +81,7 @@
}
@Override
- public BazelPackageLoader build() {
+ public BazelPackageLoader buildImpl() {
return new BazelPackageLoader(this);
}
