Skip to content

Commit

Permalink
Factor out TargetRegistrationEnvironment from Package.Builder
Browse files Browse the repository at this point in the history
`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 d8d9078.

- `disableNameConflictChecking()` now returns void.

Work toward #19922.

PiperOrigin-RevId: 678399563
Change-Id: I32cfd2c1d1aab142f271235abd59fd9d46cabf9d
  • Loading branch information
brandjon authored and copybara-github committed Sep 24, 2024
1 parent abe1f66 commit b58036e
Show file tree
Hide file tree
Showing 3 changed files with 668 additions and 613 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading

0 comments on commit b58036e

Please sign in to comment.