Skip to content

Commit

Permalink
Make Package.Builder compose instead of inherit from TargetRegistrati…
Browse files Browse the repository at this point in the history 
…onEnvironment

This gives the Builder more explicit control over its own API, at the cost of a bit of verbosity in accessing the environment as a field, and in enumerating the methods it chooses to re-export to its own clients.

It also means that in the future, we can repeat the trick of factoring out a bunch of builder methods to a new base class, without having to worry about which base class supersedes the other in the hierarchy.

- `Pakcage.Builder` now inherits directly from `StarlarkThreadContext`.

- None of `TargetRegistrationEnvironment`'s members are protected, and only a few are package-private. The choice of which methods are public now (mostly) comes down to what API makes sense for `TargetRegistrationEnvironment`, without regard for what makes sense for `Package.Builder`'s clients.

- In the builder, add `currentMacro()` convenience method.

Work toward bazelbuild#19922.

PiperOrigin-RevId: 678411561
Change-Id: I5e550fe5b582e1039fa304d42d37e882a856e742
  • Loading branch information
@brandjon @copybara-github
brandjon authored and copybara-github committed Sep 24, 2024
1 parent 33a2bb7 commit 11c090f
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 76 deletions.
149 changes: 104 additions & 45 deletions src/main/java/com/google/devtools/build/lib/packages/Package.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1183,7 +1189,7 @@ private Builder(
PackageOverheadEstimator packageOverheadEstimator,
@Nullable ImmutableMap<Location, String> generatorMap,
@Nullable Globber globber) {
super(mainRepositoryMapping);
super(() -> mainRepositoryMapping);
this.metadata = metadata;
this.pkg = new Package(metadata);
this.symbolGenerator = symbolGenerator;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
* <p>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;
Expand Down Expand Up @@ -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}.
Expand Down Expand Up @@ -1641,19 +1670,40 @@ Rule createRule(
*/
MacroInstance createMacro(
MacroClass macroClass, Map<String, Object> 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<Target> 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);
}

/**
Expand All @@ -1667,9 +1717,9 @@ void replaceTarget(Target newTarget) {
Map<String, Rule> getRulesSnapshotView() {
if (rulesSnapshotViewForFinalizers != null) {
return rulesSnapshotViewForFinalizers;
} else if (getTargetMap() instanceof SnapshottableBiMap<?, ?>) {
} else if (regEnv.getTargetMap() instanceof SnapshottableBiMap<?, ?>) {
return Maps.transformValues(
((SnapshottableBiMap<String, Target>) getTargetMap()).getTrackedSnapshot(),
((SnapshottableBiMap<String, Target>) regEnv.getTargetMap()).getTrackedSnapshot(),
target -> (Rule) target);
} else {
throw new IllegalStateException(
Expand All @@ -1691,7 +1741,7 @@ Map<String, Rule> 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
Expand All @@ -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;
}

Expand All @@ -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 "
Expand All @@ -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));
}
Expand Down Expand Up @@ -1768,7 +1818,7 @@ void addPackageGroup(
repoRootMeansCurrentRepo,
eventHandler,
location);
addTarget(group);
regEnv.addTarget(group);

if (group.containsErrors()) {
setContainsErrors();
Expand Down Expand Up @@ -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)) {
Expand All @@ -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
Expand Down Expand Up @@ -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<String, String> macroNamespaceViolatingTargets) {
regEnv.putAllMacroNamespaceViolatingTargets(macroNamespaceViolatingTargets);
}

/**
* Marks a symbolic macro as having finished evaluating.
*
Expand All @@ -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__");

Expand Down Expand Up @@ -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");

Expand All @@ -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);
}
}
Expand Down Expand Up @@ -2002,16 +2059,17 @@ 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;
}

// We create an InputFile corresponding to the BUILD file in Builder's constructor. However,
// 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
Expand All @@ -2024,32 +2082,33 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
testSuiteImplicitTestsAccumulator.clearAccumulatedTests();

Map<String, InputFile> 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
// these InputFiles now. But don't do this for rules created within a symbolic macro,
// 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<Label> labels = getRuleLabels(rule);
List<Label> labels = regEnv.getRuleLabels(rule);
for (Label label : labels) {
String name = label.getName();
if (label.getPackageIdentifier().equals(metadata.packageIdentifier())
&& !getTargetMap().containsKey(name)
&& !regEnv.getTargetMap().containsKey(name)
&& !newInputFiles.containsKey(name)) {
// Check for collision with a macro namespace. Currently this is a linear loop over
// all symbolic macros in the package.
// TODO(#19922): This is quadratic complexity, optimize with a trie or similar if
// needed.
boolean macroConflictsFound = false;
for (MacroInstance macro : getMacroMap().values()) {
macroConflictsFound |= nameIsWithinMacroNamespace(name, macro.getName());
for (MacroInstance macro : regEnv.getMacroMap().values()) {
macroConflictsFound |=
TargetRegistrationEnvironment.nameIsWithinMacroNamespace(name, macro.getName());
}
if (!macroConflictsFound) {
Location loc = rule.getLocation();
Expand All @@ -2072,7 +2131,7 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
testSuiteImplicitTestsAccumulator.sortTests();

for (InputFile file : newInputFiles.values()) {
addInputFileUnchecked(file);
regEnv.addInputFileUnchecked(file);
}

return this;
Expand All @@ -2098,13 +2157,13 @@ public Package finishBuild() {
}

// Freeze rules, compacting their attributes' representations.
for (Rule rule : getRules()) {
for (Rule rule : regEnv.getRules()) {
rule.freeze();
}

// Now all targets have been loaded, so we validate the group's member environments.
for (EnvironmentGroup envGroup : ImmutableSet.copyOf(environmentGroups.values())) {
List<Event> errors = envGroup.processMemberEnvironments(getTargetMap());
List<Event> errors = envGroup.processMemberEnvironments(regEnv.getTargetMap());
if (!errors.isEmpty()) {
Event.replayEventsOn(localEventHandler, errors);
// TODO(bazel-team): Can't we automatically infer containsError from the presence of
Expand Down
Loading

0 comments on commit 11c090f

Please sign in to comment.