Embed builtins bzls as a Java resource Currently the builtins .bzl files are packaged into, and read from, the install base. However, there is no install base in our Java unit tests, as well as in BazelPackageLoader. To support these use cases, we now instead embed the builtins_bzl.zip as a Java resource, and extract it to a fresh InMemoryFileSystem upon ConfiguredRuleClassProvider construction. The flag value --experimental_builtins_bzl_path=%install_base% has been renamed to "%bundled%". A previous prototype attempted to make ServerDirectories responsible for knowing how to access the bundled bzls. This was problematic because it required updating all instantiators of ServerDirectories (mostly tests), and complicated the target graph. The new design puts this knowledge in BazelRuleClassProvider, alongside the other rule logic registration. This avoids an undesirable situation whereby any un-updated test could block proper use of builtins injection in production. The only loss of not having builtins bzls live in the install base is that their sources are not immediately available to a debugger; you have to use --experimental_builtins_bzl_path=<some staging location> instead. This matches the intended workflow when developing builtins bzl code anyway. Our typical way of locating resource files (ResourceFileLoader) is to resolve them relative to a given Java class. But the builtins zip does not live underneath the java tree. If we tried to add it directly as a resource, then the java_binary rule would create the jar entry under its absolute package path instead of its relative path within the java root. So instead we use a genrule to copy it into the java tree where needed. Misc: - Expose a method in ResourceFileLoader to compute a resource path without retrieving it yet. This avoids the inconvenience of having to pass two args (a Class and a relative resource name) around. - Drive-by removal of a bunch of BUILD deps made obsolete by a recent configuration machinery cleanup. Likewise, drive-by auto-formatting of some files affected by the prior version of this CL. - Added the builtins zip to the distfile, for bootstrapping. This didn't belong in either the "srcs" tree or the "derived_java_srcs" target, so I added it directly as a distfile dep. - Fixed a hashCode/equals bug in ServerDirectories. Work toward #11437. PiperOrigin-RevId: 347697763
diff --git a/src/BUILD b/src/BUILD index a655dbd..6c55a33 100644 --- a/src/BUILD +++ b/src/BUILD
@@ -13,7 +13,6 @@ name = "install_base_key-file" + suffix, srcs = [ "//src/main/java/com/google/devtools/build/lib/bazel:BazelServer_deploy.jar", - "//src/main/starlark/builtins_bzl:builtins_bzl.zip", "//src/main/java/net/starlark/java/eval:cpu_profiler", # TODO(brandjon): ensure we haven't forgotten any package-zip items, # otherwise bazel won't correctly reextract modified files. @@ -324,13 +323,15 @@ [genrule( name = "package-zip" + suffix, - # This script assumes the following arg order: 1) embedded tools zip (if it - # exists), 2) the deploy jar, 3) the install base key, 4) the builtins bzl - # zip, 5) the platforms archive, 6) everything else to be bundled. + # This script assumes the following arg order: + # 1) embedded tools zip (if it exists) + # 2) the deploy jar + # 3) the install base key + # 4) the platforms archive + # 5) everything else to be bundled srcs = ([":embedded_tools" + suffix + ".zip"] if embed else []) + [ "//src/main/java/com/google/devtools/build/lib/bazel:BazelServer_deploy.jar", "install_base_key" + suffix, - "//src/main/starlark/builtins_bzl:builtins_bzl.zip", ":platforms_archive", # Non-ordered items follow: "//src/main/java/net/starlark/java/eval:cpu_profiler",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 37091e9..d468ebd0a8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -434,6 +434,7 @@ "//src/main/java/com/google/devtools/build/lib/util/io:out-err", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/com/google/devtools/common/options",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index b869ace..24221c9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.io.ByteStreams; import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.analysis.RuleContext.PrerequisiteValidator; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; @@ -52,9 +53,15 @@ import com.google.devtools.build.lib.packages.RuleClass.Builder.ThirdPartyLicenseExistencePolicy; import com.google.devtools.build.lib.packages.SymbolGenerator; import com.google.devtools.build.lib.starlarkbuildapi.core.Bootstrap; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDefinition; import com.google.devtools.common.options.OptionsProvider; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; @@ -66,6 +73,8 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; import javax.annotation.Nullable; import net.starlark.java.annot.StarlarkAnnotations; import net.starlark.java.annot.StarlarkBuiltin; @@ -99,7 +108,8 @@ private Label preludeLabel; private String runfilesPrefix; private String toolsRepository; - private String builtinsPackagePathInSource; + @Nullable private String builtinsBzlZipResource; + private String builtinsBzlPackagePathInSource; private final List<Class<? extends Fragment>> configurationFragmentClasses = new ArrayList<>(); private final List<BuildInfoFactory> buildInfoFactories = new ArrayList<>(); private final Set<Class<? extends FragmentOptions>> configurationOptions = @@ -107,11 +117,9 @@ private final Map<String, RuleClass> ruleClassMap = new HashMap<>(); private final Map<String, RuleDefinition> ruleDefinitionMap = new HashMap<>(); - private final Map<String, NativeAspectClass> nativeAspectClassMap = - new HashMap<>(); + private final Map<String, NativeAspectClass> nativeAspectClassMap = new HashMap<>(); private final Map<Class<? extends RuleDefinition>, RuleClass> ruleMap = new HashMap<>(); - private final Digraph<Class<? extends RuleDefinition>> dependencyGraph = - new Digraph<>(); + private final Digraph<Class<? extends RuleDefinition>> dependencyGraph = new Digraph<>(); private final List<Class<? extends Fragment>> universalFragments = new ArrayList<>(); @Nullable private TransitionFactory<Rule> trimmingTransitionFactory = null; @Nullable private PatchTransition toolchainTaggedTrimmingTransition = null; @@ -170,10 +178,22 @@ return this; } + /** + * Sets the resource path to the builtins_bzl.zip resource. + * + * <p>This value is required for production uses. For uses in tests, this may be left null, but + * the resulting rule class provider will not work with {@code + * --experimental_builtins_bzl_path=%bundled%}. + */ + public Builder setBuiltinsBzlZipResource(String name) { + this.builtinsBzlZipResource = name; + return this; + } + // This is required if the rule class provider will be used with // "--experimental_builtins_bzl_path=%workspace%", but can be skipped in unit tests. - public Builder setBuiltinsPackagePathInSource(String path) { - this.builtinsPackagePathInSource = path; + public Builder setBuiltinsBzlPackagePathInSource(String path) { + this.builtinsBzlPackagePathInSource = path; return this; } @@ -356,15 +376,20 @@ try { Constructor<? extends RuleConfiguredTargetFactory> ctor = factoryClass.getConstructor(); return ctor.newInstance(); - } catch (NoSuchMethodException | IllegalAccessException | InstantiationException + } catch (NoSuchMethodException + | IllegalAccessException + | InstantiationException | InvocationTargetException e) { throw new IllegalStateException(e); } } private RuleClass commitRuleDefinition(Class<? extends RuleDefinition> definitionClass) { - RuleDefinition instance = checkNotNull(ruleDefinitionMap.get(definitionClass.getName()), - "addRuleDefinition(new %s()) should be called before build()", definitionClass.getName()); + RuleDefinition instance = + checkNotNull( + ruleDefinitionMap.get(definitionClass.getName()), + "addRuleDefinition(new %s()) should be called before build()", + definitionClass.getName()); RuleDefinition.Metadata metadata = instance.getMetadata(); checkArgument( @@ -374,19 +399,18 @@ List<Class<? extends RuleDefinition>> ancestors = metadata.ancestors(); checkArgument( - metadata.type() == ABSTRACT ^ metadata.factoryClass() - != RuleConfiguredTargetFactory.class); + metadata.type() == ABSTRACT + ^ metadata.factoryClass() != RuleConfiguredTargetFactory.class); checkArgument( - (metadata.type() != TEST) - || ancestors.contains(BaseRuleClasses.TestBaseRule.class)); + (metadata.type() != TEST) || ancestors.contains(BaseRuleClasses.TestBaseRule.class)); RuleClass[] ancestorClasses = new RuleClass[ancestors.size()]; for (int i = 0; i < ancestorClasses.length; i++) { ancestorClasses[i] = ruleMap.get(ancestors.get(i)); if (ancestorClasses[i] == null) { // Ancestors should have been initialized by now - throw new IllegalStateException("Ancestor " + ancestors.get(i) + " of " - + metadata.name() + " is not initialized"); + throw new IllegalStateException( + "Ancestor " + ancestors.get(i) + " of " + metadata.name() + " is not initialized"); } } @@ -395,8 +419,8 @@ factory = createFactory(metadata.factoryClass()); } - RuleClass.Builder builder = new RuleClass.Builder( - metadata.name(), metadata.type(), false, ancestorClasses); + RuleClass.Builder builder = + new RuleClass.Builder(metadata.name(), metadata.type(), false, ancestorClasses); builder.factory(factory); builder.setThirdPartyLicenseExistencePolicy(thirdPartyLicenseExistencePolicy); RuleClass ruleClass = instance.build(builder, this); @@ -407,17 +431,58 @@ return ruleClass; } + /** + * Unpacks the builtins zip file into an InMemoryFileSystem. The zip file is located as a Java + * resource file. + * + * <p>The files underneath the zip's {@code builtins_bzl/} directory are moved to a top-level + * {@code /virtual_builtins_bzl} directory. The Path to that directory is returned. + */ + private static Path unpackBuiltinsBzlZipResource(String builtinsResourceName) { + ClassLoader loader = ConfiguredRuleClassProvider.class.getClassLoader(); + try (InputStream builtinsZip = loader.getResourceAsStream(builtinsResourceName)) { + Preconditions.checkArgument( + builtinsZip != null, "No resource with name %s", builtinsResourceName); + + InMemoryFileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256); + Path root = fs.getPath("/virtual_builtins_bzl"); + + try (ZipInputStream zip = new ZipInputStream(builtinsZip)) { + for (ZipEntry entry = zip.getNextEntry(); entry != null; entry = zip.getNextEntry()) { + String entryName = entry.getName(); + Preconditions.checkArgument(entryName.startsWith("builtins_bzl/")); + Path dest = root.getRelative(entryName.substring("builtins_bzl/".length())); + + dest.getParentDirectory().createDirectoryAndParents(); + try (OutputStream os = dest.getOutputStream()) { + ByteStreams.copy(zip, os); + } + } + } + return root; + } catch (IOException ex) { + throw new IllegalArgumentException( + "Error while unpacking builtins_bzl zip resource file", ex); + } + } + public ConfiguredRuleClassProvider build() { for (Node<Class<? extends RuleDefinition>> ruleDefinition : dependencyGraph.getTopologicalOrder()) { commitRuleDefinition(ruleDefinition.getLabel()); } + Path builtinsBzlRoot = + builtinsBzlZipResource != null + ? unpackBuiltinsBzlZipResource(builtinsBzlZipResource) + : null; + return new ConfiguredRuleClassProvider( preludeLabel, runfilesPrefix, toolsRepository, - builtinsPackagePathInSource, + builtinsBzlRoot, + builtinsBzlPackagePathInSource, ImmutableMap.copyOf(ruleClassMap), ImmutableMap.copyOf(ruleDefinitionMap), ImmutableMap.copyOf(nativeAspectClassMap), @@ -451,53 +516,43 @@ } } - /** - * Default content that should be added at the beginning of the WORKSPACE file. - */ + /** Default content that should be added at the beginning of the WORKSPACE file. */ private final String defaultWorkspaceFilePrefix; - /** - * Default content that should be added at the end of the WORKSPACE file. - */ + /** Default content that should be added at the end of the WORKSPACE file. */ private final String defaultWorkspaceFileSuffix; - - /** - * Label for the prelude file. - */ + /** Label for the prelude file. */ private final Label preludeLabel; - /** - * The default runfiles prefix. - */ + /** The default runfiles prefix. */ private final String runfilesPrefix; - /** - * The path to the tools repository. - */ + /** The path to the tools repository. */ private final String toolsRepository; - /** The relative location of the builtins_bzl directory within a Bazel source tree. */ - private final String builtinsPackagePathInSource; - /** - * Maps rule class name to the metaclass instance for that rule. + * Where the builtins bzl files are located (if not overridden by + * --experimental_builtins_bzl_path). Note that this lives in a separate InMemoryFileSystem. + * + * <p>May be null in tests, in which case --experimental_builtins_bzl_path must point to a + * builtins root. */ + @Nullable private final Path builtinsBzlRoot; + + /** The relative location of the builtins_bzl directory within a Bazel source tree. */ + private final String builtinsBzlPackagePathInSource; + + /** Maps rule class name to the metaclass instance for that rule. */ private final ImmutableMap<String, RuleClass> ruleClassMap; - /** - * Maps rule class name to the rule definition objects. - */ + /** Maps rule class name to the rule definition objects. */ private final ImmutableMap<String, RuleDefinition> ruleDefinitionMap; - /** - * Maps aspect name to the aspect factory meta class. - */ + /** Maps aspect name to the aspect factory meta class. */ private final ImmutableMap<String, NativeAspectClass> nativeAspectClassMap; - /** - * The configuration options that affect the behavior of the rules. - */ + /** The configuration options that affect the behavior of the rules. */ private final ImmutableList<Class<? extends FragmentOptions>> configurationOptions; /** The set of configuration fragment factories. */ @@ -549,7 +604,8 @@ Label preludeLabel, String runfilesPrefix, String toolsRepository, - String builtinsPackagePathInSource, + @Nullable Path builtinsBzlRoot, + String builtinsBzlPackagePathInSource, ImmutableMap<String, RuleClass> ruleClassMap, ImmutableMap<String, RuleDefinition> ruleDefinitionMap, ImmutableMap<String, NativeAspectClass> nativeAspectClassMap, @@ -573,7 +629,8 @@ this.preludeLabel = preludeLabel; this.runfilesPrefix = runfilesPrefix; this.toolsRepository = toolsRepository; - this.builtinsPackagePathInSource = builtinsPackagePathInSource; + this.builtinsBzlRoot = builtinsBzlRoot; + this.builtinsBzlPackagePathInSource = builtinsBzlPackagePathInSource; this.ruleClassMap = ruleClassMap; this.ruleDefinitionMap = ruleDefinitionMap; this.nativeAspectClassMap = nativeAspectClassMap; @@ -650,8 +707,14 @@ } @Override - public String getBuiltinsPackagePathInSource() { - return builtinsPackagePathInSource; + @Nullable + public Path getBuiltinsBzlRoot() { + return builtinsBzlRoot; + } + + @Override + public String getBuiltinsBzlPackagePathInSource() { + return builtinsBzlPackagePathInSource; } @Override @@ -716,16 +779,12 @@ return shouldInvalidateCacheForOptionDiff.apply(newOptions, changedOption, oldValue, newValue); } - /** - * Returns the set of configuration options that are supported in this module. - */ + /** Returns the set of configuration options that are supported in this module. */ public ImmutableList<Class<? extends FragmentOptions>> getConfigurationOptions() { return configurationOptions; } - /** - * Returns the definition of the rule class definition with the specified name. - */ + /** Returns the definition of the rule class definition with the specified name. */ public RuleDefinition getRuleClassDefinition(String ruleClassName) { return ruleDefinitionMap.get(ruleClassName); } @@ -738,9 +797,7 @@ return universalFragments; } - /** - * Creates a BuildOptions class for the given options taken from an optionsProvider. - */ + /** Creates a BuildOptions class for the given options taken from an optionsProvider. */ public BuildOptions createBuildOptions(OptionsProvider optionsProvider) { return BuildOptions.of(configurationOptions, optionsProvider); }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ServerDirectories.java b/src/main/java/com/google/devtools/build/lib/analysis/ServerDirectories.java index 5a8bb70..4b1ada1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ServerDirectories.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ServerDirectories.java
@@ -25,8 +25,8 @@ import javax.annotation.Nullable; /** - * Represents the relevant directories for the server: the location of the embedded binaries - * and the output directories. + * Represents the relevant directories for the server: the location of the embedded binaries and the + * output directories. */ @Immutable public final class ServerDirectories { @@ -34,15 +34,21 @@ /** Top-level user output directory; used, e.g., as default location for caches. */ private final Path outputUserRoot; + /** Where Blaze gets unpacked. */ private final Path installBase; /** The content hash of everything in installBase. */ @Nullable private final HashCode installMD5; + /** The root of the temp and output trees. */ private final Path outputBase; private final Path execRootBase; + // TODO(bazel-team): Use a builder to simplify/unify these constructors. This makes it easier to + // have sensible defaults, e.g. execRootBase = outputBase + "/execroot". Then reorder the fields + // to be consistent throughout this class. + public ServerDirectories( Path installBase, Path outputBase, @@ -58,7 +64,11 @@ } public ServerDirectories(Path installBase, Path outputBase, Path outputUserRoot) { - this(installBase, outputBase, outputUserRoot, outputBase.getRelative(EXECROOT), null); + this( + // Some tests set installBase to null. + // TODO(bazel-team): Be more consistent about whether nulls are permitted (e.g. equals() + // presently doesn't tolerate them). We should probably just disallow them. + installBase, outputBase, outputUserRoot, outputBase.getRelative(EXECROOT), null); } private static HashCode checkMD5(HashCode hash) { @@ -116,7 +126,7 @@ @Override public int hashCode() { - return Objects.hash(installBase, installMD5, outputBase, outputUserRoot); + return Objects.hash(installBase, installMD5, outputBase, execRootBase, outputUserRoot); } @Override @@ -131,6 +141,7 @@ return this.installBase.equals(that.installBase) && Objects.equals(this.installMD5, that.installMD5) && this.outputBase.equals(that.outputBase) + && this.execRootBase.equals(that.execRootBase) && this.outputUserRoot.equals(that.outputUserRoot); } }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD index 89cdaa4..0f674f4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD
@@ -22,7 +22,7 @@ java_library( name = "rules", srcs = glob(["*.java"]), - resources = glob(["*.WORKSPACE"]), + resources = glob(["*.WORKSPACE"]) + [":builtins_bzl_zip"], deps = [ "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions", @@ -98,3 +98,14 @@ "//third_party:jsr305", ], ) + +# We need this redirect target so that the builtins zip can be packaged as a +# resource next to the BazelBuiltins class (com/google/devtools/...). Otherwise +# it gets placed under the package path of the original zip target. +genrule( + name = "builtins_bzl_zip", + srcs = ["//src/main/starlark/builtins_bzl:builtins_bzl.zip"], + outs = ["builtins_bzl.zip"], + cmd = "cp $(SRCS) $@", + visibility = ["//:__pkg__"], +)
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 532590c..a2b6858 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
@@ -240,7 +240,9 @@ /** Adds this class's definitions to a builder. */ public static void setup(ConfiguredRuleClassProvider.Builder builder) { builder.setToolsRepository(TOOLS_REPOSITORY); - builder.setBuiltinsPackagePathInSource("src/main/starlark/builtins_bzl"); + builder.setBuiltinsBzlZipResource( + ResourceFileLoader.resolveResource(BazelRuleClassProvider.class, "builtins_bzl.zip")); + builder.setBuiltinsBzlPackagePathInSource("src/main/starlark/builtins_bzl"); builder.setThirdPartyLicenseExistencePolicy(ThirdPartyLicenseExistencePolicy.NEVER_CHECK); for (RuleSet ruleSet : RULE_SETS) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java index 1f7765b..8791fd4 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java
@@ -19,6 +19,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.RuleClass.Builder.ThirdPartyLicenseExistencePolicy; +import com.google.devtools.build.lib.vfs.Path; import java.util.Map; import net.starlark.java.eval.StarlarkThread; @@ -38,8 +39,17 @@ */ String getRunfilesPrefix(); + /** + * Where the builtins bzl files are located (if not overridden by + * --experimental_builtins_bzl_path). Note that this lives in a separate InMemoryFileSystem. + * + * <p>May be null in tests, in which case --experimental_builtins_bzl_path must point to a + * builtins root. + */ + Path getBuiltinsBzlRoot(); + /** The relative location of the builtins_bzl directory within a Bazel source tree. */ - String getBuiltinsPackagePathInSource(); + String getBuiltinsBzlPackagePathInSource(); /** * Returns a map from rule names to rule class objects.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 4a5ff48..4925142 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
@@ -84,8 +84,8 @@ "This flag tells Bazel how to find the \"@_builtins\" .bzl files that govern how " + "predeclared symbols for BUILD and .bzl files are defined. This flag is only " + "intended for Bazel developers, to help when writing @_builtins .bzl code. " - + "Ordinarily this value is set to \"%install_base%\", which means to use the " - + "builtins_bzl/ directory located in the install base. However, it can be set to " + + "Ordinarily this value is set to \"%bundled%\", which means to use the " + + "builtins_bzl/ directory packaged in the Bazel binary. However, it can be set to " + "the path (relative to the root of the current workspace) of an alternate " + "builtins_bzl/ directory, such as one in a Bazel source tree workspace. A literal " + "value of \"%workspace%\" is equivalent to the relative package path of "
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index 7d2e2df..079b375 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.skyframe.RecordingSkyFunctionEnvironment; @@ -102,7 +103,7 @@ // computeInline() entry point. private final PackageFactory packageFactory; - // Used for determining paths to builtins bzls. + // Used for determining paths to builtins bzls that live in the workspace. private final BlazeDirectories directories; // Handles retrieving BzlCompileValues, either by calling Skyframe or by inlining @@ -585,20 +586,29 @@ } private Root getBuiltinsRoot(StarlarkSemantics starlarkSemantics) throws BzlLoadFailedException { - String path = starlarkSemantics.get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH); - if (path.isEmpty()) { + String flag = starlarkSemantics.get(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH); + if (flag.isEmpty()) { throw new IllegalStateException("Requested builtins root, but injection is disabled"); - } else if (path.equals("%install_base%")) { - return Root.fromPath(directories.getInstallBase().getRelative("builtins_bzl")); - } else if (path.equals("%workspace%")) { - return Root.fromPath( - directories - .getWorkspace() - .getRelative(packageFactory.getRuleClassProvider().getBuiltinsPackagePathInSource())); - } else { - // TODO(#11437): Should we consider interning these roots? - return Root.fromPath(directories.getWorkspace().getRelative(path)); } + + Path path; + if (flag.equals("%bundled%")) { + // May be null in tests, but in that case the flag shouldn't be set to %bundled%. + path = + Preconditions.checkNotNull( + packageFactory.getRuleClassProvider().getBuiltinsBzlRoot(), + "rule class provider does not specify a builtins root; either call" + + " setBuiltinsBzlZipResource() or else set --experimental_builtins_bzl_path to" + + " a root"); + } else if (flag.equals("%workspace%")) { + String packagePath = + packageFactory.getRuleClassProvider().getBuiltinsBzlPackagePathInSource(); + path = directories.getWorkspace().getRelative(packagePath); + } else { + path = directories.getWorkspace().getRelative(flag); + } + // TODO(#11437): Should we consider interning these roots? + return Root.fromPath(path); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/util/ResourceFileLoader.java b/src/main/java/com/google/devtools/build/lib/util/ResourceFileLoader.java index a773f4a..3f32fc0 100644 --- a/src/main/java/com/google/devtools/build/lib/util/ResourceFileLoader.java +++ b/src/main/java/com/google/devtools/build/lib/util/ResourceFileLoader.java
@@ -20,9 +20,9 @@ import java.io.InputStream; /** - * A little utility to load resources (property files) from jars or - * the classpath. Recommended for longer texts that do not fit nicely into - * a piece of Java code - e.g. a template for a lengthy email. + * A little utility to load resources (property files) from jars or the classpath. Recommended for + * longer texts that do not fit nicely into a piece of Java code - e.g. a template for a lengthy + * email. */ public final class ResourceFileLoader { @@ -37,11 +37,10 @@ } /** - * Loads a text resource that is located in a directory on the Java classpath that - * corresponds to the package of <code>relativeToClass</code> using UTF8 encoding. - * E.g. - * <code>loadResource(Class.forName("com.google.foo.Foo", "bar.txt"))</code> - * will look for <code>com/google/foo/bar.txt</code> in the classpath. + * Loads a text resource that is located in a directory on the Java classpath that corresponds to + * the package of <code>relativeToClass</code> using UTF8 encoding. E.g. <code> + * loadResource(Class.forName("com.google.foo.Foo", "bar.txt"))</code> will look for <code> + * com/google/foo/bar.txt</code> in the classpath. */ public static String loadResource(Class<?> relativeToClass, String resourceName) throws IOException { @@ -55,11 +54,19 @@ private static InputStream getResourceAsStream(Class<?> relativeToClass, String resourceName) { ClassLoader loader = relativeToClass.getClassLoader(); + String resource = resolveResource(relativeToClass, resourceName); + return loader.getResourceAsStream(resource); + } + + /** + * Converts a relative resource name and Java class to a full resource path, using the same logic + * as {@link #loadResource}. + */ + public static String resolveResource(Class<?> relativeToClass, String resourceName) { // TODO(bazel-team): use relativeToClass.getPackage().getName(). String className = relativeToClass.getName(); String packageName = className.substring(0, className.lastIndexOf('.')); String path = packageName.replace('.', '/'); - String resource = path + '/' + resourceName; - return loader.getResourceAsStream(resource); + return path + '/' + resourceName; } }
diff --git a/src/main/starlark/builtins_bzl/BUILD b/src/main/starlark/builtins_bzl/BUILD index ee13edc..43efa02 100644 --- a/src/main/starlark/builtins_bzl/BUILD +++ b/src/main/starlark/builtins_bzl/BUILD
@@ -10,11 +10,11 @@ visibility = ["//src:__pkg__"], ) -# A zipfile containing the builtins_bzl/ directory as it should appear in the -# package zip / install base. +# A zipfile containing the builtins_bzl/ directory, to be bundled as a Java +# resource with BazelRuleClassProvider. genrule( name = "builtins_bzl_zip", - srcs = [f for f in glob(["**"]) if f.endswith(".bzl")] + ["BUILD.builtins"], + srcs = glob(["**/*.bzl"]) + ["BUILD.builtins"], outs = ["builtins_bzl.zip"], # builtins_zip.sh zip output builtins_root files... cmd = "$(location //src:zip_builtins)" + @@ -22,5 +22,7 @@ " $@ src/main/starlark/builtins_bzl $(SRCS)", output_to_bindir = 1, tools = ["//src:zip_builtins"], - visibility = ["//src:__pkg__"], + visibility = [ + "//src/main/java/com/google/devtools/build/lib/bazel/rules:__pkg__", + ], )
diff --git a/src/main/starlark/builtins_bzl/BUILD.builtins b/src/main/starlark/builtins_bzl/BUILD.builtins index bdb8e52..fa0e7e2 100644 --- a/src/main/starlark/builtins_bzl/BUILD.builtins +++ b/src/main/starlark/builtins_bzl/BUILD.builtins
@@ -3,6 +3,6 @@ # interface, they use a modified dialect of Bazel's Build Language and cannot be # loaded as regular .bzl files. # -# Bazel developers: This directory is used whenever "%install_base%" is passed -# to --experimental_builtins_bzl_path. See StarlarkBuiltinsFunction.java and +# Bazel developers: This directory is used whenever "%bundled%" is passed to +# --experimental_builtins_bzl_path. See StarlarkBuiltinsFunction.java and # BzlLoadFunction.java for more information.
diff --git a/src/package-bazel.sh b/src/package-bazel.sh index 9a00625..606d3e5 100755 --- a/src/package-bazel.sh +++ b/src/package-bazel.sh
@@ -24,7 +24,6 @@ EMBEDDED_TOOLS=$1; shift DEPLOY_JAR=$1; shift INSTALL_BASE_KEY=$1; shift -BUILTINS_ZIP=$1; shift PLATFORMS_ARCHIVE=$1; shift if [[ "$OUT" == *jdk_allmodules.zip ]]; then @@ -71,12 +70,6 @@ (cd ${PACKAGE_DIR}/embedded_tools && unzip -q "${WORKDIR}/${EMBEDDED_TOOLS}") fi -# Add the builtins bzls. -( - cd $PACKAGE_DIR - unzip -q $WORKDIR/$BUILTINS_ZIP -) - # Unzip platforms.zip into platforms/, move files up from external/platforms # subdirectory, and create WORKSPACE if it doesn't exist. (
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java index ed333f2..439cf38 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java
@@ -86,14 +86,12 @@ public abstract void setupMockClient( MockToolsConfig mockToolsConfig, List<String> getWorkspaceContents) throws IOException; - /** - * Returns the contents of WORKSPACE. - */ + /** Returns the contents of WORKSPACE. */ public abstract List<String> getWorkspaceContents(MockToolsConfig config); /** - * This is called from test setup to create any necessary mock workspace files in the - * <code>_embedded_binaries</code> directory. + * This is called from test setup to create any necessary mock workspace files in the <code> + * _embedded_binaries</code> directory. */ public abstract void setupMockWorkspaceFiles(Path embeddedBinariesRoot) throws IOException;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsIntegrationTest.java index 7fdd0f4..09911dd 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsIntegrationTest.java
@@ -44,4 +44,17 @@ assertThat(result).isNull(); assertContainsEvent("_builtins_dummy is experimental"); } + + @Test + public void builtinsInjectionWorksInBuildViewTestCase() throws Exception { + scratch.file("pkg/BUILD", "load(':foo.bzl', 'foo')"); + scratch.file("pkg/foo.bzl", "foo = 1; print(\"dummy :: \" + str(_builtins_dummy))"); + setBuildLanguageOptions( + "--experimental_builtins_bzl_path=%bundled%", "--experimental_builtins_dummy=true"); + + getConfiguredTarget("//pkg:BUILD"); + // The production builtins bzl code overwrites the dummy from "original value" to "overridden + // value". + assertContainsEvent("dummy :: overridden value"); + } }
diff --git a/src/test/shell/integration/builtins_injection_test.sh b/src/test/shell/integration/builtins_injection_test.sh index 76ca8fc..c36ec14 100755 --- a/src/test/shell/integration/builtins_injection_test.sh +++ b/src/test/shell/integration/builtins_injection_test.sh
@@ -107,16 +107,12 @@ &>"$TEST_log" || fail "bazel build failed" expect_log "dummy :: original value" - # TODO(#11437): This doesn't work yet because PackageLoader doesn't have an - # install base to read from. The solution will be to provide the builtins as - # a Java resource in that case. - # - # # Using the builtins root bundled with bazel in the install base. - # bazel build //pkg:BUILD --experimental_builtins_dummy=true \ - # --experimental_builtins_bzl_path=%install_base% \ - # &>"$TEST_log" || fail "bazel build failed" - # # "overridden value" comes from the exports.bzl in production Bazel. - # expect_log "dummy :: overridden value" + # Using the builtins root that's bundled with bazel. + bazel build //pkg:BUILD --experimental_builtins_dummy=true \ + --experimental_builtins_bzl_path=%bundled% \ + &>"$TEST_log" || fail "bazel build failed" + # "overridden value" comes from the exports.bzl in production Bazel. + expect_log "dummy :: overridden value" # Using the builtins root located within the client workspace, as if we're # running Bazel in its own source tree.