From b58036e8d12c770a3ce14f0c709a5f6def3eea2d Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 24 Sep 2024 14:33:12 -0700 Subject: [PATCH] Factor out TargetRegistrationEnvironment from Package.Builder `Package.java` is too large and its builder class tries to do too many things. This and the following CLs are an effort to reduce its LOC and better separate concerns. The main strategy for accomplishing this is to move out the logic for registering targets/macros, validating their names, and indexing them, into a separate dedicated container class. Future work could look at minimizing `Package.Builder` down to just the things needed to construct a `Package` in the most straightforward context, deserialization. All the stuff that's needed to evaluate BUILD Starlark threads doesn't have to be there -- for instance, `glob()` and `generator_name` machinery. This CL is focused on just branching `Package.Builder` into a new superclass, `TargetRegistrationEnvironment`. To keep the diff simple I avoided other refactoring in this CL, such as reordering members and restricting field access by adding new getters. The net result is about 600 LOC moved out of a 2800 line file. In Package.java: - Inlined static methods `getTargets(BiMap) and `getTargets(Target, Class)`. - Moved the map fields that track registration of targets and macros. Also moved `currentMacroFrame`, `nameConflictCheckingPolicy`, the `ruleLabels` cache, and `containsErrors`. I did *not* move `unexpandedMacros` and `rulesSnapshotViewForFinalizers` because those are part of the evaluation model for symbolic macros, and aren't needed for their registration / conflict checking. - Moved corresponding accessors/setters for these fields. - The new base class doesn't track `pkg`, so it can't do the precondition checks that the targets it's manipulating belong to the package being constructed. For now, I kept these checks by overriding the applicable methods in the Builder class and dispatching to `super`. If these checks are worthwhile, then the better solution is to add a `pkg` field to `TargetRegistrationEnvironment`. - Methods that *create* targets (rather than merely adding them) are *not* pushed up to `TargetRegistrationEnvironment`. In TargetRegistrationEnvironment: - Fields access broadened (for now) from `private` to `protected`. (Technically, package-private would've also worked, but `protected` is a clear signal that it's intended for use in a subclass.) Same for a few methods. - Update javadoc/comment in `setContainsErrors()` to not mention `addEvent[s]()`, which hasn't existed on `Package.Builder` since https://github.com/bazelbuild/bazel/commit/d8d9078ba5dadc575e12f52424313b3a07bf3eaf. - `disableNameConflictChecking()` now returns void. Work toward #19922. PiperOrigin-RevId: 678399563 Change-Id: I32cfd2c1d1aab142f271235abd59fd9d46cabf9d --- .../build/lib/packages/MacroClass.java | 2 +- .../devtools/build/lib/packages/Package.java | 625 +---------------- .../TargetRegistrationEnvironment.java | 654 ++++++++++++++++++ 3 files changed, 668 insertions(+), 613 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/packages/TargetRegistrationEnvironment.java diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java index 1c96271190a256..d61ce28086af41 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java @@ -23,8 +23,8 @@ import com.google.common.collect.Lists; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.packages.Package.Builder.MacroFrame; import com.google.devtools.build.lib.packages.TargetDefinitionContext.NameConflictException; +import com.google.devtools.build.lib.packages.TargetRegistrationEnvironment.MacroFrame; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.ArrayList; 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 434712ed2fe38a..473153c00c1544 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 @@ -18,7 +18,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.collect.BiMap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -69,7 +68,6 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -150,11 +148,6 @@ public enum ConfigSettingVisibilityPolicy { /** Sentinel value for package overhead being empty. */ private static final long PACKAGE_OVERHEAD_UNSET = -1; - /** Used for constructing macro namespace violation error messages. */ - private static final String MACRO_NAMING_RULES = - "Name must be the same as the macro's name, or the macro's name followed by '_'" - + " (recommended), '-', or '.', and a non-empty string."; - // ==== General package metadata fields ==== private final Metadata metadata; @@ -550,26 +543,12 @@ public ImmutableSortedMap getTargets() { return targets; } - /** Common getTargets implementation, accessible by {@link Package.Builder}. */ - private static Set getTargets(BiMap targetMap) { - return targetMap.values(); - } - /** * Returns a (read-only, ordered) iterable of all the targets belonging to this package which are * instances of the specified class. */ public Iterable getTargets(Class targetClass) { - return getTargets(targets, targetClass); - } - - /** - * Common getTargets implementation, accessible by both {@link Package} and {@link - * Package.Builder}. - */ - private static Iterable getTargets( - Map targetMap, Class targetClass) { - return Iterables.filter(targetMap.values(), targetClass); + return Iterables.filter(targets.values(), targetClass); } /** @@ -606,7 +585,9 @@ public void checkMacroNamespaceCompliance(Target target) throws MacroNamespaceVi String.format( "Target %s declared in symbolic macro '%s' violates macro naming rules and cannot be" + " built. %s", - target.getLabel(), macroNamespaceViolated, MACRO_NAMING_RULES)); + target.getLabel(), + macroNamespaceViolated, + TargetRegistrationEnvironment.MACRO_NAMING_RULES)); } } @@ -1008,7 +989,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 TargetDefinitionContext { + public static class Builder extends TargetRegistrationEnvironment { /** * A bundle of options affecting package construction, that is not specific to any particular @@ -1097,7 +1078,7 @@ default boolean precomputeTransitiveLoads() { @Nullable private String ioExceptionMessage = null; @Nullable private IOException ioException = null; @Nullable private DetailedExitCode ioExceptionDetailedExitCode = null; - private boolean containsErrors = false; + // A package's FailureDetail field derives from the events on its Builder's event handler. // During package deserialization, those events are unavailable, because those events aren't // serialized [*]. Its FailureDetail value is serialized, however. During deserialization, that @@ -1116,15 +1097,6 @@ default boolean precomputeTransitiveLoads() { private final Map environmentGroups = new HashMap<>(); - // All targets added to the package. - // - // We use SnapshottableBiMap to help track insertion order of Rule targets, for use by - // native.existing_rules(). - // - // Use addOrReplaceTarget() to add new entries. - private BiMap targets = - new SnapshottableBiMap<>(target -> target instanceof Rule); - // The snapshot of {@link #targets} for use in rule finalizer macros. Contains all // non-finalizer-instantiated rule targets (i.e. all rule targets except for those instantiated // in a finalizer or in a macro called from a finalizer). @@ -1132,10 +1104,6 @@ default boolean precomputeTransitiveLoads() { // Initialized by expandAllRemainingMacros() and reset to null by beforeBuild(). @Nullable private Map rulesSnapshotViewForFinalizers; - // All instances of symbolic macros created during package construction, indexed by id (not - // name). - private final Map macros = new LinkedHashMap<>(); - /** * Ids of all symbolic macros that have been declared but not yet evaluated. * @@ -1148,109 +1116,6 @@ default boolean precomputeTransitiveLoads() { */ private final Set unexpandedMacros = new LinkedHashSet<>(); - /** - * Represents the innermost currently executing symbolic macro, or null if none are running. - * - *

Logically, this is the top entry of a stack of frames where each frame corresponds to a - * nested symbolic macro invocation. In actuality, symbolic macros do not necessarily run - * eagerly when they are invoked, so this is not really a call stack per se. We leave it to the - * pkgbuilder client to set the current frame, so that the choice of whether to push and pop, or - * process a worklist of queued evaluations, is up to them. - * - *

The state of this field is used to determine what Starlark APIs are available (see user - * documentation on {@code macro()} at {@link StarlarkRuleFunctionsApi#macro}), and to help - * enforce naming requirements on targets and macros. - */ - private MacroFrame currentMacroFrame = null; - - /** - * Represents the state of a running symbolic macro (see {@link #currentMacroFrame}). - * Semi-opaque. - */ - static class MacroFrame { - final MacroInstance macroInstance; - // Most name conflicts are caught by checking the keys of the `targets` and `macros` maps. - // It is not a conflict for a target or macro to have the same name as the macro it is - // declared in, yet such a target or macro may still conflict with siblings in the same macro. - // We use this bool to track whether or not a newly introduced macro, M, having the same name - // as its parent (the current macro), would clash with an already defined sibling of M. - private boolean mainSubmacroHasBeenDefined = false; - - MacroFrame(MacroInstance macroInstance) { - this.macroInstance = macroInstance; - } - } - - private enum NameConflictCheckingPolicy { - UNKNOWN, - NOT_GUARANTEED, - ENABLED; - } - - /** - * Whether to do all validation checks for name clashes among targets, macros, and output file - * prefixes. - * - *

The {@code NOT_GUARANTEED} value should only be used when the package data has already - * been validated, e.g. in package deserialization. - * - *

Setting it to {@code NOT_GUARANTEED} does not necessarily turn off *all* checking, just - * some of the more expensive ones. Do not rely on being able to violate these checks. - */ - private NameConflictCheckingPolicy nameConflictCheckingPolicy = - NameConflictCheckingPolicy.UNKNOWN; - - /** - * Stores labels for each rule so that we don't have to call the costly {@link Rule#getLabels} - * twice (once for {@link #checkForInputOutputConflicts} and once for {@link #beforeBuild}). - * - *

This field is null if name conflict checking is disabled. It is also null after the - * package is built. - */ - // TODO(#19922): Technically we don't need to store entries for rules that were created by - // macros; see rulesCreatedInMacros, below. - @Nullable private Map> ruleLabels = new HashMap<>(); - - /** - * Stores labels of rule targets that were created in symbolic macros. We don't implicitly - * create input files on behalf of such targets (though they may still be created on behalf of - * other targets not in macros). - * - *

This field is null if name conflict checking is disabled. It is also null after the - * package is built. - */ - // TODO(#19922): This can be eliminated once we have Targets directly store a reference to the - // MacroInstance that instantiated them. (This is a little nontrivial because we'd like to avoid - // simply adding a new field to Target subclasses, and instead want to combine it with the - // existing Package-typed field.) - @Nullable private Set rulesCreatedInMacros = new HashSet<>(); - - /** - * A map from names of targets declared in a symbolic macro which violate macro naming rules, - * such as "lib%{name}-src.jar" implicit outputs in java rules, to the name of the macro - * instance where they were declared. - * - *

This field is null if name conflict checking is disabled. The content of the map is - * manipulated only in {@link #checkRuleAndOutputs}. - */ - @Nullable - private LinkedHashMap macroNamespaceViolatingTargets = new LinkedHashMap<>(); - - /** - * A map from target name to the (innermost) macro instance that declared it. See {@link - * Package#targetsToDeclaringMacros}. - */ - private LinkedHashMap targetsToDeclaringMacros = new LinkedHashMap<>(); - - /** - * The collection of the prefixes of every output file. Maps each prefix to an arbitrary output - * file having that prefix. Used for error reporting. - * - *

This field is null if name conflict checking is disabled. It is also null after the - * package is built. The content of the map is manipulated only in {@link #checkRuleAndOutputs}. - */ - @Nullable private Map outputFilePrefixes = new HashMap<>(); - private final List registeredExecutionPlatforms = new ArrayList<>(); private final List registeredToolchains = new ArrayList<>(); @@ -1678,25 +1543,6 @@ void setIOException(IOException e, String message, DetailedExitCode detailedExit setContainsErrors(); } - /** - * Declares that errors were encountering while loading this package. If called, {@link - * #addEvent} or {@link #addEvents} should already have been called with an {@link Event} of - * type {@link EventKind#ERROR} that includes a {@link FailureDetail}. - */ - // TODO(bazel-team): For simplicity it would be nice to replace this with - // getLocalEventHandler().hasErrors(), since that would prevent the kind of inconsistency where - // we have reported an ERROR event but not called setContainsErrors(), or vice versa. - public void setContainsErrors() { - // TODO(bazel-team): Maybe do Preconditions.checkState(localEventHandler.hasErrors()). - // Maybe even assert that it has a FailureDetail, though that's a linear scan unless we - // customize the event handler. - this.containsErrors = true; - } - - public boolean containsErrors() { - return containsErrors; - } - void setFailureDetailOverride(FailureDetail failureDetail) { failureDetailOverride = failureDetail; } @@ -1800,59 +1646,14 @@ MacroInstance createMacro( return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth); } - /** - * Inserts a target into the targets map. Returns the previous target if one was present, or - * null. - */ - @CanIgnoreReturnValue - @Nullable - private Target addOrReplaceTarget(Target target) { - Target existing = targets.put(target.getName(), target); - if (currentMacroFrame != null) { - targetsToDeclaringMacros.put(target.getName(), currentMacroFrame.macroInstance); - } - return existing; - } - - @Nullable - Target getTarget(String name) { - return targets.get(name); - } - - /** - * Replaces a target in the {@link Package} under construction with a new target with the same - * name and belonging to the same package. - * - *

Requires that {@link #disableNameConflictChecking} was not called. - * - *

A hack needed for {@link WorkspaceFactoryHelper}. - */ + @Override void replaceTarget(Target newTarget) { - ensureNameConflictChecking(); - - Preconditions.checkArgument( - targets.containsKey(newTarget.getName()), - "No existing target with name '%s' in the targets map", - newTarget.getName()); Preconditions.checkArgument( newTarget.getPackage() == pkg, // pointer comparison since we're constructing `pkg` "Replacement target belongs to package '%s', expected '%s'", newTarget.getPackage(), pkg); - Target oldTarget = addOrReplaceTarget(newTarget); - if (newTarget instanceof Rule) { - List

The returned {@link Iterable} will be deterministically ordered, in the order the rule - * instance targets were instantiated. - */ - private Iterable getRules() { - return Package.getTargets(targets, Rule.class); - } - /** * Creates an input file target in this package with the specified name, if it does not yet * exist. @@ -2063,92 +1854,18 @@ void addEnvironmentGroup( } } - /** - * Turns off (some) conflict checking for name clashes between targets, macros, and output file - * prefixes. (It is not guaranteed to disable all checks, since it is intended as an - * optimization and not for semantic effect.) - * - *

This should only be done for data that has already been validated, e.g. during package - * deserialization. Do not call this unless you know what you're doing. - * - *

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}. - */ - @CanIgnoreReturnValue - Builder disableNameConflictChecking() { - Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN); - this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.NOT_GUARANTEED; - this.ruleLabels = null; - this.rulesCreatedInMacros = null; - this.macroNamespaceViolatingTargets = null; - this.outputFilePrefixes = null; - return this; - } - - private void ensureNameConflictChecking() { - Preconditions.checkState( - nameConflictCheckingPolicy != NameConflictCheckingPolicy.NOT_GUARANTEED); - this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.ENABLED; - } - - /** - * Adds a rule and its outputs to the targets map, and propagates the error bit from the rule to - * the package. - */ - private void addRuleInternal(Rule rule) { + @Override + protected void addRuleInternal(Rule rule) { Preconditions.checkArgument(rule.getPackage() == pkg); - for (OutputFile outputFile : rule.getOutputFiles()) { - addOrReplaceTarget(outputFile); - } - addOrReplaceTarget(rule); - if (rule.containsErrors()) { - setContainsErrors(); - } + super.addRuleInternal(rule); } - /** - * Adds a rule without certain validation checks. Requires that {@link - * #disableNameConflictChecking} was already called. - */ - void addRuleUnchecked(Rule rule) { - Preconditions.checkState( - nameConflictCheckingPolicy == NameConflictCheckingPolicy.NOT_GUARANTEED); - addRuleInternal(rule); - } - - /** - * Adds a rule, subject to the usual validation checks. Requires that {@link - * #disableNameConflictChecking} was not called. - */ - void addRule(Rule rule) throws NameConflictException { - ensureNameConflictChecking(); - - List

Note that this function examines only the current macro frame, not any parent frames; and - * thus returns true even if the current non-finalizer macro was called within a finalizer - * macro. - */ - public boolean currentlyInNonFinalizerMacro() { - return currentMacroFrame != null - ? !currentMacroFrame.macroInstance.getMacroClass().isFinalizer() - : false; - } - - /** - * Returns true if a symbolic macro is running and the current macro frame is a rule finalizer. - */ - public boolean currentlyInFinalizer() { - return currentMacroFrame != null - ? currentMacroFrame.macroInstance.getMacroClass().isFinalizer() - : false; - } - - /** - * Sets the current macro frame and returns the old one. - * - *

Either the new or old frame may be null, indicating no currently running symbolic macro. - */ - @Nullable - MacroFrame setCurrentMacroFrame(@Nullable MacroFrame frame) { - MacroFrame prev = currentMacroFrame; - currentMacroFrame = frame; - return prev; - } - /** * If we are currently executing a symbolic macro, returns the result of unioning the given * visibility with the location of the innermost macro's code. Otherwise, returns the given @@ -2463,281 +2139,6 @@ Package build(boolean discoverAssumedInputFiles) throws NoSuchPackageException { return finishBuild(); } - /** - * Adds an input file to this package. - * - *

There must not already be a target with the same name (i.e., this is not idempotent). - */ - private void addInputFile(InputFile inputFile) { - Target prev = addOrReplaceTarget(inputFile); - Preconditions.checkState(prev == null); - } - - /** - * Precondition check for {@link #addRule} (to be called before the rule and its outputs are in - * the targets map). Verifies that: - * - *

    - *
  • The added rule's name, and the names of its output files, are not the same as the name - * of any target already declared in the package. - *
  • The added rule's output files list does not contain the same name twice. - *
  • The added rule does not have an input file and an output file that share the same name. - *
  • For each of the added rule's output files, no directory prefix of that file matches the - * name of another output file in the package; and conversely, the file is not itself a - * prefix for another output file. (This check statefully mutates the {@code - * outputFilePrefixes} field.) - *
- */ - // TODO(bazel-team): We verify that all prefixes of output files are distinct from other output - // file names, but not that they're distinct from other target names in the package. What - // happens if you define an input file "abc" and output file "abc/xyz"? - private void checkRuleAndOutputs(Rule rule, List