From beee1bfc48749d081bcf338b99c2e8ed52fe6bfb Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Oct 2024 07:15:07 -0700 Subject: [PATCH] Factor out much of Package.Builder into TargetDefinitionContext This CL creates a new base class of `Package.Builder`, `TargetDefinitionContext`, to hold all the API needed during Starlark evaluation of a package. It does not include operations that are only needed during setup / tear down of the package builder. For instance, `addRule()` belongs on this base class, whereas `finishBuild()` remains on the `Builder`. Factoring in this manner makes it clearer what operations need to be supported to evaluate a piece of a package corresponding to a single symbolic macro (needed for lazy macro evaluation). In any case, anything that splits the API in a reasonably systematic way and allows us to siphon off lines of code from Package.java is a win. A previous base class by the name of `TargetDefinitionContext` was deleted in https://github.com/bazelbuild/bazel/commit/33a2bb79c6d1b03adb3b9c46c658fe18438cc5d1. I made it a point to first remove that file before working on this CL so that we could treat the new `TargetDefinitionContext.java` as a branch of Package.java for better code review and history preservation. All the stuff that's branched over is left in the same declaration order. Other changes: - Migrated fields are made `protected` if they are still directly referenced in `Package.java`. - `setMakeVariable`, `mergePackageArgsFrom` (2 overloads), and `setWorkspaceName` are made void (avoids the `Builder` return type problem). - `getRulesSnapshotView`, `getNonFinalizerInstantiatedRule`, and `addMacro` are moved but also overridden by the builder. Work toward #19922. PiperOrigin-RevId: 681006375 Change-Id: I2df816dc35e7ceea9e8704ae9df2a2ea6daeceda --- .../devtools/build/lib/packages/Package.java | 570 +--------------- .../lib/packages/TargetDefinitionContext.java | 635 ++++++++++++++++++ .../build/lib/packages/WorkspaceGlobals.java | 7 +- .../build/lib/skyframe/PackageFunction.java | 6 +- 4 files changed, 665 insertions(+), 553 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/packages/TargetDefinitionContext.java 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 d9f2ec5c765be0..c0ff8a35a25923 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 @@ -14,17 +14,13 @@ package com.google.devtools.build.lib.packages; -import static com.google.common.base.MoreObjects.firstNonNull; - import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.Interner; import com.google.common.collect.Iterables; -import com.google.common.collect.Maps; import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.cmdline.BazelModuleContext; import com.google.devtools.build.lib.cmdline.BazelModuleContext.LoadGraphVisitor; @@ -36,14 +32,10 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.cmdline.StarlarkThreadContext; import com.google.devtools.build.lib.cmdline.TargetPattern; -import com.google.devtools.build.lib.collect.CollectionUtils; -import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; 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.TargetRecorder.MacroFrame; import com.google.devtools.build.lib.packages.TargetRecorder.MacroNamespaceViolationException; import com.google.devtools.build.lib.packages.TargetRecorder.NameConflictException; import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey; @@ -67,18 +59,15 @@ import java.io.IOException; import java.io.PrintStream; import java.util.ArrayList; -import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.OptionalInt; import java.util.OptionalLong; import java.util.Set; -import java.util.TreeMap; import java.util.concurrent.Semaphore; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -994,7 +983,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 StarlarkThreadContext { + public static class Builder extends TargetDefinitionContext { /** * A bundle of options affecting package construction, that is not specific to any particular @@ -1033,62 +1022,17 @@ default boolean precomputeTransitiveLoads() { PackageSettings DEFAULTS = new PackageSettings() {}; } - private final SymbolGenerator symbolGenerator; - - // Same as pkg.metadata. - private final Metadata metadata; - - /** - * The output instance for this builder. Needs to be instantiated and available with name info - * throughout initialization. All other settings are applied during {@link #build}. See {@link - * Package#Package} and {@link Package#finishInit} for details. - */ - private final Package pkg; - - // The container object on which targets and macro instances are added and conflicts are - // detected. - private final TargetRecorder recorder = new TargetRecorder(); - - // Initialized from outside but also potentially set by `workspace()` function in WORKSPACE - // file. - private String workspaceName; - - private final Label buildFileLabel; - private final boolean precomputeTransitiveLoads; private final boolean noImplicitFileExport; - private final boolean simplifyUnconditionalSelectsInRuleAttrs; // The map from each repository to that repository's remappings map. // This is only used in the //external package, it is an empty map for all other packages. private final HashMap> externalPackageRepositoryMappings = new HashMap<>(); - /** Converts label literals to Label objects within this package. */ - private final LabelConverter labelConverter; - /** Estimates the package overhead of this package. */ private final PackageOverheadEstimator packageOverheadEstimator; - /** - * Semaphore held by the Skyframe thread when performing CPU work. - * - *

This should be released when performing I/O. - */ - @Nullable // Only non-null when inside PackageFunction.compute and the semaphore is enabled. - private final Semaphore cpuBoundSemaphore; - - // TreeMap so that the iteration order of variables is consistent regardless of insertion order - // (which may change due to serialization). This is useful so that the serialized representation - // is deterministic. - private final TreeMap makeEnv = new TreeMap<>(); - - private final StoredEventHandler localEventHandler = new StoredEventHandler(); - - @Nullable private String ioExceptionMessage = null; - @Nullable private IOException ioException = null; - @Nullable private DetailedExitCode ioExceptionDetailedExitCode = null; - // 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 @@ -1101,12 +1045,6 @@ default boolean precomputeTransitiveLoads() { // serialize events emitted during its construction/evaluation. @Nullable private FailureDetail failureDetailOverride = null; - // Used by glob(). Null for contexts where glob() is disallowed, including WORKSPACE files and - // some tests. - @Nullable private final Globber globber; - - private final Map environmentGroups = new HashMap<>(); - // 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). @@ -1139,48 +1077,6 @@ default boolean precomputeTransitiveLoads() { */ private OptionalInt firstWorkspaceSuffixRegisteredToolchain = OptionalInt.empty(); - /** True iff the "package" function has already been called in this package. */ - private boolean packageFunctionUsed; - - private final Interner> listInterner = new ThreadCompatibleInterner<>(); - - private final ImmutableMap generatorMap; - - private final TestSuiteImplicitTestsAccumulator testSuiteImplicitTestsAccumulator = - new TestSuiteImplicitTestsAccumulator(); - - /** Returns the "generator_name" to use for a given call site location in a BUILD file. */ - @Nullable - String getGeneratorNameByLocation(Location loc) { - return generatorMap.get(loc); - } - - /** - * Returns the value to use for {@code test_suite}s' {@code $implicit_tests} attribute, as-is, - * when the {@code test_suite} doesn't specify an explicit, non-empty {@code tests} value. The - * returned list is mutated by the package-building process - it may be observed to be empty or - * incomplete before package loading is complete. When package loading is complete it will - * contain the label of each non-manual test matching the provided tags in the package, in label - * order. - * - *

This method MUST be called before the package is built - otherwise the requested - * implicit tests won't be accumulated. - */ - List

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() { - recorder.setContainsErrors(); - } - - void setIOException(IOException e, String message, DetailedExitCode detailedExitCode) { - this.ioException = e; - this.ioExceptionMessage = message; - this.ioExceptionDetailedExitCode = detailedExitCode; - setContainsErrors(); - } - void setFailureDetailOverride(FailureDetail failureDetail) { failureDetailOverride = failureDetail; } @@ -1628,87 +1390,6 @@ private void checkLoadsNotSet() { pkg.transitiveLoads == null, "Transitive loads already set: %s", pkg.transitiveLoads); } - /** - * Returns the {@link Globber} used to implement {@code glob()} functionality during BUILD - * evaluation. Null for contexts where globbing is not possible, including WORKSPACE files and - * some tests. - */ - @Nullable - public Globber getGlobber() { - return globber; - } - - /** - * Returns true if values of conditional rule attributes which only contain unconditional - * selects should be simplified and stored as a non-select value. - */ - public boolean simplifyUnconditionalSelectsInRuleAttrs() { - return this.simplifyUnconditionalSelectsInRuleAttrs; - } - - /** - * Returns the innermost currently executing symbolic macro, or null if not in a symbolic macro. - */ - @Nullable - public MacroInstance currentMacro() { - MacroFrame frame = recorder.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}. - * - *

The created {@link Rule} will have no output files and therefore will be in an invalid - * state. - */ - Rule createRule( - Label label, RuleClass ruleClass, List callstack) { - return createRule( - label, - ruleClass, - callstack.isEmpty() ? Location.BUILTIN : callstack.get(0).location, - CallStack.compactInterior(callstack)); - } - - Rule createRule( - Label label, - RuleClass ruleClass, - Location location, - @Nullable CallStack.Node interiorCallStack) { - return new Rule(pkg, label, ruleClass, location, interiorCallStack); - } - - /** - * Creates a new {@link MacroInstance} {@code m} where {@code m.getPackage()} is the {@link - * Package} associated with this {@link Builder}. - */ - MacroInstance createMacro( - MacroClass macroClass, Map attrValues, int sameNameDepth) - throws EvalException { - MacroInstance parent = currentMacro(); - return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth); - } - - @Nullable - public MacroFrame getCurrentMacroFrame() { - return recorder.getCurrentMacroFrame(); - } - - @Nullable - public MacroFrame setCurrentMacroFrame(@Nullable MacroFrame frame) { - return recorder.setCurrentMacroFrame(frame); - } - - public boolean currentlyInNonFinalizerMacro() { - return recorder.currentlyInNonFinalizerMacro(); - } - - @Nullable - public Target getTarget(String name) { - return recorder.getTarget(name); - } - public Set getTargets() { return recorder.getTargets(); } @@ -1722,216 +1403,22 @@ void replaceTarget(Target newTarget) { recorder.replaceTarget(newTarget); } - /** - * Returns a lightweight snapshot view of the names of all rule targets belonging to this - * package at the time of this call; in finalizer expansion stage, returns a lightweight - * snapshot view of only the non-finalizer-instantiated rule targets. - * - * @throws IllegalStateException if this method is called after {@link #beforeBuild} has been - * called. - */ + @Override Map getRulesSnapshotView() { if (rulesSnapshotViewForFinalizers != null) { return rulesSnapshotViewForFinalizers; - } else if (recorder.getTargetMap() instanceof SnapshottableBiMap) { - return Maps.transformValues( - ((SnapshottableBiMap) recorder.getTargetMap()).getTrackedSnapshot(), - target -> (Rule) target); } else { - throw new IllegalStateException( - "getRulesSnapshotView() cannot be used after beforeBuild() has been called"); + return super.getRulesSnapshotView(); } } - /** - * Returns a non-finalizer-instantiated rule target with the provided name belonging to this - * package at the time of this call. If such a rule target cannot be returned, returns null. - */ - // TODO(https://github.com/bazelbuild/bazel/issues/23765): when we restrict - // native.existing_rule() to be usable only in finalizer context, we can replace this method - // with {@code getRulesSnapshotView().get(name)}; we don't do so at present because we do not - // want to make unnecessary snapshots. + @Override @Nullable Rule getNonFinalizerInstantiatedRule(String name) { if (rulesSnapshotViewForFinalizers != null) { return rulesSnapshotViewForFinalizers.get(name); } else { - Target target = recorder.getTargetMap().get(name); - return target instanceof Rule ? (Rule) target : null; - } - } - - /** - * Creates an input file target in this package with the specified name, if it does not yet - * exist. - * - *

This operation is idempotent. - * - * @param targetName name of the input file. This must be a valid target name as defined by - * {@link com.google.devtools.build.lib.cmdline.LabelValidator#validateTargetName}. - * @return the newly-created {@code InputFile}, or the old one if it already existed. - * @throws NameConflictException if the name was already taken by another target that is not an - * input file - * @throws IllegalArgumentException if the name is not a valid label - */ - InputFile createInputFile(String targetName, Location location) throws NameConflictException { - Target existing = recorder.getTargetMap().get(targetName); - - if (existing instanceof InputFile) { - return (InputFile) existing; // idempotent - } - - InputFile inputFile; - try { - inputFile = new InputFile(pkg, createLabel(targetName), location); - } catch (LabelSyntaxException e) { - throw new IllegalArgumentException( - "FileTarget in package " + metadata.getName() + " has illegal name: " + targetName, e); - } - - recorder.addTarget(inputFile); - return inputFile; - } - - /** - * Sets the visibility and license for an input file. The input file must already exist as a - * member of this package. - * - * @throws IllegalArgumentException if the input file doesn't exist in this package's target - * map. - */ - // TODO: #19922 - Don't allow exports_files() to modify visibility of targets that the current - // symbolic macro did not create. Fun pathological example: exports_files() modifying the - // visibility of :BUILD inside a symbolic macro. - void setVisibilityAndLicense(InputFile inputFile, RuleVisibility visibility, License license) { - String filename = inputFile.getName(); - Target cacheInstance = recorder.getTargetMap().get(filename); - if (!(cacheInstance instanceof InputFile)) { - throw new IllegalArgumentException( - "Can't set visibility for nonexistent FileTarget " - + filename - + " in package " - + metadata.getName() - + "."); - } - if (!((InputFile) cacheInstance).isVisibilitySpecified() - || cacheInstance.getVisibility() != visibility - || !Objects.equals(cacheInstance.getLicense(), license)) { - recorder.replaceInputFileUnchecked( - new VisibilityLicenseSpecifiedInputFile( - pkg, cacheInstance.getLabel(), cacheInstance.getLocation(), visibility, license)); - } - } - - /** - * Creates a label for a target inside this package. - * - * @throws LabelSyntaxException if the {@code targetName} is invalid - */ - Label createLabel(String targetName) throws LabelSyntaxException { - return Label.create(metadata.packageIdentifier(), targetName); - } - - /** Adds a package group to the package. */ - void addPackageGroup( - String name, - Collection packages, - Collection

TODO(bazel-team): apply this to all build functions (maybe automatically?), possibly - * integrate with RuleClass.checkForDuplicateLabels. - */ - private static boolean hasDuplicateLabels( - List

In other words, if a {@code Package.Builder} method needs to be called as a result of Starlark + * evaluation of the BUILD file or its macros, the operation belongs in this base class. + * + *

The motivation for this split is two-fold: 1) It keeps the size of Package.java smaller. 2) It + * will make it easier to factor out common code for evaluating a whole package vs an individual + * symbolic macro of that package (lazy macro evaluation). + */ +public abstract class TargetDefinitionContext extends StarlarkThreadContext { + + // TODO: #19922 - Avoid protected fields, encapsulate with getters/setters. Temporary state on way + // to separating this class from Package.Builder. + + private final SymbolGenerator symbolGenerator; + + // Same as pkg.metadata. + protected final Metadata metadata; + + /** + * The {@link Package} to be constructed with the help of this context. + * + *

Since the package has not yet been constructed, it is in an intermediate state and some + * operations may fail unexpectedly. {@code TargetDefinitionContext} only uses this field to help + * create the cyclic links between packages and their targets. + */ + protected final Package pkg; + + // The container object on which targets and macro instances are added and conflicts are + // detected. + protected final TargetRecorder recorder = new TargetRecorder(); + + // Initialized from outside but also potentially set by `workspace()` function in WORKSPACE + // file. + protected String workspaceName; + + protected final Label buildFileLabel; + + private final boolean simplifyUnconditionalSelectsInRuleAttrs; + + /** Converts label literals to Label objects within this package. */ + private final LabelConverter labelConverter; + + /** + * Semaphore held by the Skyframe thread when performing CPU work. + * + *

This should be released when performing I/O. + */ + @Nullable // Only non-null when inside PackageFunction.compute and the semaphore is enabled. + private final Semaphore cpuBoundSemaphore; + + // TreeMap so that the iteration order of variables is consistent regardless of insertion order + // (which may change due to serialization). This is useful so that the serialized representation + // is deterministic. + protected final TreeMap makeEnv = new TreeMap<>(); + + protected final StoredEventHandler localEventHandler = new StoredEventHandler(); + + @Nullable protected String ioExceptionMessage = null; + @Nullable protected IOException ioException = null; + @Nullable protected DetailedExitCode ioExceptionDetailedExitCode = null; + + // Used by glob(). Null for contexts where glob() is disallowed, including WORKSPACE files and + // some tests. + @Nullable private final Globber globber; + + protected final Map environmentGroups = new HashMap<>(); + + /** True iff the "package" function has already been called in this package. */ + private boolean packageFunctionUsed; + + private final Interner> listInterner = new ThreadCompatibleInterner<>(); + + private final ImmutableMap generatorMap; + + protected final TestSuiteImplicitTestsAccumulator testSuiteImplicitTestsAccumulator = + new TestSuiteImplicitTestsAccumulator(); + + /** Returns the "generator_name" to use for a given call site location in a BUILD file. */ + @Nullable + String getGeneratorNameByLocation(Location loc) { + return generatorMap.get(loc); + } + + /** + * Returns the value to use for {@code test_suite}s' {@code $implicit_tests} attribute, as-is, + * when the {@code test_suite} doesn't specify an explicit, non-empty {@code tests} value. The + * returned list is mutated by the package-building process - it may be observed to be empty or + * incomplete before package loading is complete. When package loading is complete it will contain + * the label of each non-manual test matching the provided tags in the package, in label order. + * + *

This method MUST be called before the package is built - otherwise the requested + * implicit tests won't be accumulated. + */ + List

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() { + recorder.setContainsErrors(); + } + + void setIOException(IOException e, String message, DetailedExitCode detailedExitCode) { + this.ioException = e; + this.ioExceptionMessage = message; + this.ioExceptionDetailedExitCode = detailedExitCode; + setContainsErrors(); + } + + /** + * Returns the {@link Globber} used to implement {@code glob()} functionality during BUILD + * evaluation. Null for contexts where globbing is not possible, including WORKSPACE files and + * some tests. + */ + @Nullable + public Globber getGlobber() { + return globber; + } + + /** + * Returns true if values of conditional rule attributes which only contain unconditional selects + * should be simplified and stored as a non-select value. + */ + public boolean simplifyUnconditionalSelectsInRuleAttrs() { + return this.simplifyUnconditionalSelectsInRuleAttrs; + } + + /** + * Returns the innermost currently executing symbolic macro, or null if not in a symbolic macro. + */ + @Nullable + public MacroInstance currentMacro() { + MacroFrame frame = recorder.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}. + * + *

The created {@link Rule} will have no output files and therefore will be in an invalid + * state. + */ + Rule createRule(Label label, RuleClass ruleClass, List callstack) { + return createRule( + label, + ruleClass, + callstack.isEmpty() ? Location.BUILTIN : callstack.get(0).location, + CallStack.compactInterior(callstack)); + } + + Rule createRule( + Label label, + RuleClass ruleClass, + Location location, + @Nullable CallStack.Node interiorCallStack) { + return new Rule(pkg, label, ruleClass, location, interiorCallStack); + } + + /** + * Creates a new {@link MacroInstance} {@code m} where {@code m.getPackage()} is the {@link + * Package} associated with this {@link Builder}. + */ + MacroInstance createMacro( + MacroClass macroClass, Map attrValues, int sameNameDepth) + throws EvalException { + MacroInstance parent = currentMacro(); + return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth); + } + + @Nullable + public MacroFrame getCurrentMacroFrame() { + return recorder.getCurrentMacroFrame(); + } + + @Nullable + public MacroFrame setCurrentMacroFrame(@Nullable MacroFrame frame) { + return recorder.setCurrentMacroFrame(frame); + } + + public boolean currentlyInNonFinalizerMacro() { + return recorder.currentlyInNonFinalizerMacro(); + } + + @Nullable + public Target getTarget(String name) { + return recorder.getTarget(name); + } + + // TODO: #19922 - Refactor finalizer expansion such that TargetDefinitionContext can handle + // working with finalizer macros. At that point, getRulesSnapshotView() and + // getNonFinalizerInstantiatedRule() must account for the snapshot view here rather than in the + // override in Package.Builder. + + /** + * Returns a lightweight snapshot view of the names of all rule targets belonging to this package + * at the time of this call; in finalizer expansion stage, returns a lightweight snapshot view of + * only the non-finalizer-instantiated rule targets. + * + * @throws IllegalStateException if this method is called after {@link + * Package.Builder#beforeBuild} has been called. + */ + Map getRulesSnapshotView() { + if (recorder.getTargetMap() instanceof SnapshottableBiMap) { + return Maps.transformValues( + ((SnapshottableBiMap) recorder.getTargetMap()).getTrackedSnapshot(), + target -> (Rule) target); + } else { + throw new IllegalStateException( + "getRulesSnapshotView() cannot be used after beforeBuild() has been called"); + } + } + + /** + * Returns a non-finalizer-instantiated rule target with the provided name belonging to this + * package at the time of this call. If such a rule target cannot be returned, returns null. + */ + // TODO(https://github.com/bazelbuild/bazel/issues/23765): when we restrict + // native.existing_rule() to be usable only in finalizer context, we can replace this method + // with {@code getRulesSnapshotView().get(name)}; we don't do so at present because we do not + // want to make unnecessary snapshots. + @Nullable + Rule getNonFinalizerInstantiatedRule(String name) { + Target target = recorder.getTargetMap().get(name); + return target instanceof Rule ? (Rule) target : null; + } + + /** + * Creates an input file target in this package with the specified name, if it does not yet exist. + * + *

This operation is idempotent. + * + * @param targetName name of the input file. This must be a valid target name as defined by {@link + * com.google.devtools.build.lib.cmdline.LabelValidator#validateTargetName}. + * @return the newly-created {@code InputFile}, or the old one if it already existed. + * @throws NameConflictException if the name was already taken by another target that is not an + * input file + * @throws IllegalArgumentException if the name is not a valid label + */ + InputFile createInputFile(String targetName, Location location) throws NameConflictException { + Target existing = recorder.getTargetMap().get(targetName); + + if (existing instanceof InputFile) { + return (InputFile) existing; // idempotent + } + + InputFile inputFile; + try { + inputFile = new InputFile(pkg, createLabel(targetName), location); + } catch (LabelSyntaxException e) { + throw new IllegalArgumentException( + "FileTarget in package " + metadata.getName() + " has illegal name: " + targetName, e); + } + + recorder.addTarget(inputFile); + return inputFile; + } + + /** + * Sets the visibility and license for an input file. The input file must already exist as a + * member of this package. + * + * @throws IllegalArgumentException if the input file doesn't exist in this package's target map. + */ + // TODO: #19922 - Don't allow exports_files() to modify visibility of targets that the current + // symbolic macro did not create. Fun pathological example: exports_files() modifying the + // visibility of :BUILD inside a symbolic macro. + void setVisibilityAndLicense(InputFile inputFile, RuleVisibility visibility, License license) { + String filename = inputFile.getName(); + Target cacheInstance = recorder.getTargetMap().get(filename); + if (!(cacheInstance instanceof InputFile)) { + throw new IllegalArgumentException( + "Can't set visibility for nonexistent FileTarget " + + filename + + " in package " + + metadata.getName() + + "."); + } + if (!((InputFile) cacheInstance).isVisibilitySpecified() + || cacheInstance.getVisibility() != visibility + || !Objects.equals(cacheInstance.getLicense(), license)) { + recorder.replaceInputFileUnchecked( + new VisibilityLicenseSpecifiedInputFile( + pkg, cacheInstance.getLabel(), cacheInstance.getLocation(), visibility, license)); + } + } + + /** + * Creates a label for a target inside this package. + * + * @throws LabelSyntaxException if the {@code targetName} is invalid + */ + Label createLabel(String targetName) throws LabelSyntaxException { + return Label.create(metadata.packageIdentifier(), targetName); + } + + /** Adds a package group to the package. */ + void addPackageGroup( + String name, + Collection packages, + Collection

TODO(bazel-team): apply this to all build functions (maybe automatically?), possibly + * integrate with RuleClass.checkForDuplicateLabels. + */ + private static boolean hasDuplicateLabels( + List