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 1877897d36894a..b8d7e8f26d461c 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 @@ -43,7 +43,9 @@ import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings; +import com.google.devtools.build.lib.packages.TargetRegistrationEnvironment.MacroFrame; import com.google.devtools.build.lib.packages.TargetRegistrationEnvironment.MacroNamespaceViolationException; +import com.google.devtools.build.lib.packages.TargetRegistrationEnvironment.NameConflictException; import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; @@ -740,12 +742,12 @@ private void finishInit(Builder builder) { this.workspaceName = builder.workspaceName; this.makeEnv = ImmutableMap.copyOf(builder.makeEnv); - this.targets = ImmutableSortedMap.copyOf(builder.getTargetMap()); - this.macros = ImmutableSortedMap.copyOf(builder.getMacroMap()); + this.targets = ImmutableSortedMap.copyOf(builder.regEnv.getTargetMap()); + this.macros = ImmutableSortedMap.copyOf(builder.regEnv.getMacroMap()); this.macroNamespaceViolatingTargets = - ImmutableMap.copyOf(builder.getMacroNamespaceViolatingTargets()); + ImmutableMap.copyOf(builder.regEnv.getMacroNamespaceViolatingTargets()); this.targetsToDeclaringMacros = - ImmutableSortedMap.copyOf(builder.getTargetsToDeclaringMacros()); + ImmutableSortedMap.copyOf(builder.regEnv.getTargetsToDeclaringMacros()); this.failureDetail = builder.getFailureDetail(); this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms); this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains); @@ -988,7 +990,7 @@ public static Builder newExternalPackageBuilderForBzlmod( * A builder for {@link Package} objects. Only intended to be used by {@link PackageFactory} and * {@link com.google.devtools.build.lib.skyframe.PackageFunction}. */ - public static class Builder extends TargetRegistrationEnvironment { + public static class Builder extends StarlarkThreadContext { /** * A bundle of options affecting package construction, that is not specific to any particular @@ -1039,6 +1041,10 @@ default boolean precomputeTransitiveLoads() { */ private final Package pkg; + // The container object on which targets and macro instances are added and conflicts are + // detected. + private final TargetRegistrationEnvironment regEnv = new TargetRegistrationEnvironment(); + // Initialized from outside but also potentially set by `workspace()` function in WORKSPACE // file. private String workspaceName; @@ -1183,7 +1189,7 @@ private Builder( PackageOverheadEstimator packageOverheadEstimator, @Nullable ImmutableMap generatorMap, @Nullable Globber globber) { - super(mainRepositoryMapping); + super(() -> mainRepositoryMapping); this.metadata = metadata; this.pkg = new Package(metadata); this.symbolGenerator = symbolGenerator; @@ -1214,7 +1220,7 @@ private Builder( // Add target for the BUILD file itself. // (This may be overridden by an exports_file declaration.) - addInputFileUnchecked( + regEnv.addInputFileUnchecked( new InputFile( pkg, buildFileLabel, @@ -1269,8 +1275,8 @@ public static Builder fromOrFail( boolean bad = false; if (ctx instanceof Builder builder) { bad |= !allowBuild && !builder.isRepoRulePackage(); - bad |= !allowFinalizers && builder.currentlyInFinalizer(); - bad |= !allowNonFinalizerSymbolicMacros && builder.currentlyInNonFinalizerMacro(); + bad |= !allowFinalizers && builder.regEnv.currentlyInFinalizer(); + bad |= !allowNonFinalizerSymbolicMacros && builder.regEnv.currentlyInNonFinalizerMacro(); bad |= !allowWorkspace && builder.isRepoRulePackage(); if (!bad) { return builder; @@ -1535,6 +1541,20 @@ void setComputationSteps(long n) { pkg.computationSteps = n; } + public boolean containsErrors() { + return regEnv.containsErrors(); + } + + /** + * Declares that errors were encountering while loading this package. + * + *

If this method is called, then there should also be an ERROR event added to the handler on + * the {@link Package.Builder}. The event should include a {@link FailureDetail}. + */ + public void setContainsErrors() { + regEnv.setContainsErrors(); + } + void setIOException(IOException e, String message, DetailedExitCode detailedExitCode) { this.ioException = e; this.ioExceptionMessage = message; @@ -1611,6 +1631,15 @@ public Globber getGlobber() { return globber; } + /** + * Returns the innermost currently executing symbolic macro, or null if not in a symbolic macro. + */ + @Nullable + private MacroInstance currentMacro() { + MacroFrame frame = regEnv.getCurrentMacroFrame(); + return frame == null ? null : frame.macroInstance; + } + /** * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package} * associated with this {@link Builder}. @@ -1641,19 +1670,40 @@ Rule createRule( */ MacroInstance createMacro( MacroClass macroClass, Map attrValues, int sameNameDepth) { - MacroInstance parent = - getCurrentMacroFrame() == null ? null : getCurrentMacroFrame().macroInstance; + MacroInstance parent = currentMacro(); return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth); } - @Override + @Nullable + public MacroFrame getCurrentMacroFrame() { + return regEnv.getCurrentMacroFrame(); + } + + @Nullable + public MacroFrame setCurrentMacroFrame(@Nullable MacroFrame frame) { + return regEnv.setCurrentMacroFrame(frame); + } + + public boolean currentlyInNonFinalizerMacro() { + return regEnv.currentlyInNonFinalizerMacro(); + } + + @Nullable + public Target getTarget(String name) { + return regEnv.getTarget(name); + } + + public Set getTargets() { + return regEnv.getTargets(); + } + void replaceTarget(Target newTarget) { Preconditions.checkArgument( newTarget.getPackage() == pkg, // pointer comparison since we're constructing `pkg` "Replacement target belongs to package '%s', expected '%s'", newTarget.getPackage(), pkg); - super.replaceTarget(newTarget); + regEnv.replaceTarget(newTarget); } /** @@ -1667,9 +1717,9 @@ void replaceTarget(Target newTarget) { Map getRulesSnapshotView() { if (rulesSnapshotViewForFinalizers != null) { return rulesSnapshotViewForFinalizers; - } else if (getTargetMap() instanceof SnapshottableBiMap) { + } else if (regEnv.getTargetMap() instanceof SnapshottableBiMap) { return Maps.transformValues( - ((SnapshottableBiMap) getTargetMap()).getTrackedSnapshot(), + ((SnapshottableBiMap) regEnv.getTargetMap()).getTrackedSnapshot(), target -> (Rule) target); } else { throw new IllegalStateException( @@ -1691,7 +1741,7 @@ Map getRulesSnapshotView() { * @throws IllegalArgumentException if the name is not a valid label */ InputFile createInputFile(String targetName, Location location) throws NameConflictException { - Target existing = getTargetMap().get(targetName); + Target existing = regEnv.getTargetMap().get(targetName); if (existing instanceof InputFile) { return (InputFile) existing; // idempotent @@ -1705,7 +1755,7 @@ InputFile createInputFile(String targetName, Location location) throws NameConfl "FileTarget in package " + metadata.getName() + " has illegal name: " + targetName, e); } - addTarget(inputFile); + regEnv.addTarget(inputFile); return inputFile; } @@ -1721,7 +1771,7 @@ InputFile createInputFile(String targetName, Location location) throws NameConfl // visibility of :BUILD inside a symbolic macro. void setVisibilityAndLicense(InputFile inputFile, RuleVisibility visibility, License license) { String filename = inputFile.getName(); - Target cacheInstance = getTargetMap().get(filename); + Target cacheInstance = regEnv.getTargetMap().get(filename); if (!(cacheInstance instanceof InputFile)) { throw new IllegalArgumentException( "Can't set visibility for nonexistent FileTarget " @@ -1733,7 +1783,7 @@ void setVisibilityAndLicense(InputFile inputFile, RuleVisibility visibility, Lic if (!((InputFile) cacheInstance).isVisibilitySpecified() || cacheInstance.getVisibility() != visibility || !Objects.equals(cacheInstance.getLicense(), license)) { - replaceInputFileUnchecked( + regEnv.replaceInputFileUnchecked( new VisibilityLicenseSpecifiedInputFile( pkg, cacheInstance.getLabel(), cacheInstance.getLocation(), visibility, license)); } @@ -1768,7 +1818,7 @@ void addPackageGroup( repoRootMeansCurrentRepo, eventHandler, location); - addTarget(group); + regEnv.addTarget(group); if (group.containsErrors()) { setContainsErrors(); @@ -1808,7 +1858,7 @@ void addEnvironmentGroup( EventHandler eventHandler, Location location) throws NameConflictException, LabelSyntaxException { - Preconditions.checkState(getCurrentMacroFrame() == null); + Preconditions.checkState(currentMacro() == null); if (hasDuplicateLabels(environments, name, "environments", location, eventHandler) || hasDuplicateLabels(defaults, name, "defaults", location, eventHandler)) { @@ -1818,7 +1868,7 @@ void addEnvironmentGroup( EnvironmentGroup group = new EnvironmentGroup(createLabel(name), pkg, environments, defaults, location); - addTarget(group); + regEnv.addTarget(group); // Invariant: once group is inserted into targets, it must also: // (a) be inserted into environmentGroups, or @@ -1851,26 +1901,33 @@ void addEnvironmentGroup( } } - @Override + // For Package deserialization. + void disableNameConflictChecking() { + regEnv.disableNameConflictChecking(); + } + public void addRuleUnchecked(Rule rule) { Preconditions.checkArgument(rule.getPackage() == pkg); - super.addRuleUnchecked(rule); + regEnv.addRuleUnchecked(rule); } - @Override public void addRule(Rule rule) throws NameConflictException { Preconditions.checkArgument(rule.getPackage() == pkg); - super.addRule(rule); + regEnv.addRule(rule); } - @Override public void addMacro(MacroInstance macro) throws NameConflictException { Preconditions.checkState( !isRepoRulePackage(), "Cannot instantiate symbolic macros in this context"); - super.addMacro(macro); + regEnv.addMacro(macro); unexpandedMacros.add(macro.getId()); } + // For Package deserialization. + void putAllMacroNamespaceViolatingTargets(Map macroNamespaceViolatingTargets) { + regEnv.putAllMacroNamespaceViolatingTargets(macroNamespaceViolatingTargets); + } + /** * Marks a symbolic macro as having finished evaluating. * @@ -1895,10 +1952,10 @@ public void markMacroComplete(MacroInstance macro) { * from which the macro's {@code MacroClass} was exported. */ RuleVisibility copyAppendingCurrentMacroLocation(RuleVisibility visibility) { - if (getCurrentMacroFrame() == null) { + if (currentMacro() == null) { return visibility; } - MacroClass macroClass = getCurrentMacroFrame().macroInstance.getMacroClass(); + MacroClass macroClass = currentMacro().getMacroClass(); PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier(); Label newVisibilityItem = Label.createUnvalidated(macroLocation, "__pkg__"); @@ -1953,7 +2010,7 @@ public void expandAllRemainingMacros(StarlarkSemantics semantics) throws Interru if (!unexpandedMacros.isEmpty()) { Preconditions.checkState( unexpandedMacros.stream() - .allMatch(id -> getMacroMap().get(id).getMacroClass().isFinalizer()), + .allMatch(id -> regEnv.getMacroMap().get(id).getMacroClass().isFinalizer()), "At the beginning of finalizer expansion, unexpandedMacros must contain only" + " finalizers"); @@ -1962,14 +2019,14 @@ public void expandAllRemainingMacros(StarlarkSemantics semantics) throws Interru // include any rule instantiated by a finalizer or macro called from a finalizer. if (rulesSnapshotViewForFinalizers == null) { Preconditions.checkState( - getTargetMap() instanceof SnapshottableBiMap, + regEnv.getTargetMap() instanceof SnapshottableBiMap, "Cannot call expandAllRemainingMacros() after beforeBuild() has been called"); rulesSnapshotViewForFinalizers = getRulesSnapshotView(); } while (!unexpandedMacros.isEmpty()) { // NB: collection mutated by body String id = unexpandedMacros.iterator().next(); - MacroInstance macro = getMacroMap().get(id); + MacroInstance macro = regEnv.getMacroMap().get(id); MacroClass.executeMacroImplementation(macro, this, semantics); } } @@ -2002,8 +2059,8 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack // map to the SnapshottableBiMap's underlying bimap and thus stop tracking insertion order. // After this point, snapshots of targets should no longer be used, and any further // getRulesSnapshotView calls will throw. - if (getTargetMap() instanceof SnapshottableBiMap) { - unwrapSnapshottableBiMap(); + if (regEnv.getTargetMap() instanceof SnapshottableBiMap) { + regEnv.unwrapSnapshottableBiMap(); rulesSnapshotViewForFinalizers = null; } @@ -2011,7 +2068,8 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack // the visibility of this target may be overridden with an exports_files directive, so we wait // until now to obtain the current instance from the targets map. pkg.buildFile = - (InputFile) Preconditions.checkNotNull(getTargetMap().get(buildFileLabel.getName())); + Preconditions.checkNotNull( + (InputFile) regEnv.getTargetMap().get(buildFileLabel.getName())); // TODO(bazel-team): We run testSuiteImplicitTestsAccumulator here in beforeBuild(), but what // if one of the accumulated tests is later removed in PackageFunction, between the call to @@ -2024,7 +2082,7 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack testSuiteImplicitTestsAccumulator.clearAccumulatedTests(); Map newInputFiles = new HashMap<>(); - for (Rule rule : getRules()) { + for (Rule rule : regEnv.getRules()) { if (discoverAssumedInputFiles) { // Labels mentioned by a rule that refer to an unknown target in the current package are // assumed to be InputFiles, unless they overlap a namespace owned by a macro. Create @@ -2032,24 +2090,25 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack // since we don't want the evaluation of the macro to affect the semantics of whether or // not this target was created (i.e. all implicitly created files are knowable without // necessarily evaluating symbolic macros). - if (isRuleCreatedInMacro(rule)) { + if (regEnv.isRuleCreatedInMacro(rule)) { continue; } // We use a temporary map, newInputFiles, to avoid concurrent modification to this.targets // while iterating (via getRules() above). - List

The target must have a valid name (for the current macro) and cannot have already been * added. */ - protected void addTarget(Target target) throws NameConflictException { + public void addTarget(Target target) throws NameConflictException { if (target instanceof Rule rule) { // Use addRule() to ensure all rule-related maps and caches are consulted. // checkTargetName() and putTargetInternal() are both reached through addRule(). @@ -274,7 +265,7 @@ protected void addTarget(Target target) throws NameConflictException { *

The target must not have already been added, and there cannot be any existing target by the * same name. */ - protected void addInputFileUnchecked(InputFile file) { + public void addInputFileUnchecked(InputFile file) { Target prev = putTargetInternal(file); Preconditions.checkState(prev == null); } @@ -284,17 +275,17 @@ protected void addInputFileUnchecked(InputFile file) { * *

It is an error if no input file by that name already exists. */ - protected void replaceInputFileUnchecked(InputFile file) { + public void replaceInputFileUnchecked(InputFile file) { Target prev = putTargetInternal(file); Preconditions.checkState(prev instanceof InputFile, prev); } @Nullable - Target getTarget(String name) { + public Target getTarget(String name) { return targetMap.get(name); } - protected void unwrapSnapshottableBiMap() { + public void unwrapSnapshottableBiMap() { Preconditions.checkState(targetMap instanceof SnapshottableBiMap); this.targetMap = ((SnapshottableBiMap) targetMap).getUnderlyingBiMap(); } @@ -309,7 +300,7 @@ protected void unwrapSnapshottableBiMap() { * *

A hack needed for {@link WorkspaceFactoryHelper}. */ - void replaceTarget(Target newTarget) { + public void replaceTarget(Target newTarget) { ensureNameConflictChecking(); Preconditions.checkArgument( @@ -328,6 +319,9 @@ void replaceTarget(Target newTarget) { } } + // TODO(bazel-team): This method allows target deletion via the returned view, which is used in + // PackageFunction#handleLabelsCrossingSubpackagesAndPropagateInconsistentFilesystemExceptions. + // Let's disallow that and make removal go through a dedicated method. public Set getTargets() { return targetMap.values(); } @@ -338,7 +332,7 @@ public Set getTargets() { *

The returned {@link Iterable} will be deterministically ordered, in the order the rule * instance targets were instantiated. */ - protected Iterable getRules() { + public Iterable getRules() { return Iterables.filter(targetMap.values(), Rule.class); } @@ -353,7 +347,7 @@ protected Iterable getRules() { *

This method must be called prior to {@link #addRuleUnchecked}. It may not be called, neither * before nor after, a call to {@link #addRule} or {@link #replaceTarget}. */ - void disableNameConflictChecking() { + public void disableNameConflictChecking() { Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN); this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.NOT_GUARANTEED; this.ruleLabels = null; @@ -362,7 +356,7 @@ void disableNameConflictChecking() { this.outputFilePrefixes = null; } - protected void ensureNameConflictChecking() { + public void ensureNameConflictChecking() { Preconditions.checkState( nameConflictCheckingPolicy != NameConflictCheckingPolicy.NOT_GUARANTEED); this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.ENABLED; @@ -386,7 +380,7 @@ private void addRuleInternal(Rule rule) { * Adds a rule without certain validation checks. Requires that {@link * #disableNameConflictChecking} was already called. */ - void addRuleUnchecked(Rule rule) { + public void addRuleUnchecked(Rule rule) { Preconditions.checkState( nameConflictCheckingPolicy == NameConflictCheckingPolicy.NOT_GUARANTEED); addRuleInternal(rule); @@ -396,7 +390,7 @@ void addRuleUnchecked(Rule rule) { * Adds a rule, subject to the usual validation checks. Requires that {@link * #disableNameConflictChecking} was not called. */ - void addRule(Rule rule) throws NameConflictException { + public void addRule(Rule rule) throws NameConflictException { ensureNameConflictChecking(); List

Either the new or old frame may be null, indicating no currently running symbolic macro. */ @Nullable - MacroFrame setCurrentMacroFrame(@Nullable MacroFrame frame) { + public MacroFrame setCurrentMacroFrame(@Nullable MacroFrame frame) { MacroFrame prev = currentMacroFrame; currentMacroFrame = frame; return prev; @@ -554,7 +548,7 @@ private void checkRuleAndOutputs(Rule rule, List

Note that just because a name is within a macro's namespace does not necessarily mean the * corresponding target or macro was declared within this macro. */ - protected static boolean nameIsWithinMacroNamespace(String name, String macroName) { + public static boolean nameIsWithinMacroNamespace(String name, String macroName) { if (name.equals(macroName)) { return true; } else if (name.startsWith(macroName)) { @@ -605,7 +599,8 @@ private void checkTargetName(Target target) throws NameConflictException { * *

Intended to be used for package deserialization. */ - void putAllMacroNamespaceViolatingTargets(Map macroNamespaceViolatingTargets) { + public void putAllMacroNamespaceViolatingTargets( + Map macroNamespaceViolatingTargets) { if (this.macroNamespaceViolatingTargets == null) { this.macroNamespaceViolatingTargets = new LinkedHashMap<>(); }