Skip to content

Commit

Permalink
Reorder the top-level declarations in the Package class
Browse files Browse the repository at this point in the history
The declaration order of the elements in `Package.java` makes it even harder to figure out what is going on in this already monstrous (3k LOC) file. That order evolved over a very long time. This CL is an attempt to wrangle this complexity and unblock further cleanups, toward the goal of adding support for evaluation of individual symbolic macros.

(The drawback of this change is that it has a very large blast radius in terms of code line history / blame. I imagine I'll be sent many CL reviews by default for Package.java in the future...)

The new order for `Package`'s members is:

1. enums and static fields
2. instance fields -- metadata, then WORKSPACE-like fields, then targets/macros maps
3. accessors, in the same order as their corresponding fields (or where those fields will be after the follow-up CL). This includes a number of convenience accessors around subfields of the `metadata` field.
4. initialization logic - constructor, `finishInit()`, and a mutator only used in construction (I hope)
5. stringification
6. public static utility methods
7. non-trivial subclasses

with private helper methods sprinkled where appropriate. The order for `Package.Builder` and `Package.Metadata` elements is unchanged.

Other non-ordering changes:
- inlined `setPackageOverhead()`
- `getRepositoryMapping(RepositoryName)` was an overload of `getRepositoryMapping()`, but the former is intended for use only in `//external` while the latter is for normal `BUILD` packages. Renamed `getRepositoryMapping(RepositoryName)` -> `getExternalPackageRepositoryMapping(RepositoryName)`, which almost overloads an analogous method `getExternalPackageRepositoryMappings()` (note the missing 's').
- elaborated TODO for `setContainsErrors()`
- made `contextualizeFailureDetailForTarget` a static method, eliminated unneeded preconditions
- renamed static overload of `getSourceRoot` to `computeSourceRoot`; it belongs with `finishInit`, not the accessor
- inlined `createDetailedCode`

Work toward #19922.

PiperOrigin-RevId: 676094017
Change-Id: If2f779a6461b45c9c586653ee31f2f9dda7b71eb
  • Loading branch information
brandjon authored and copybara-github committed Sep 18, 2024
1 parent 9e91c0f commit e34043c
Show file tree
Hide file tree
Showing 4 changed files with 381 additions and 351 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ private StateMachine unwrapTarget(Tasks tasks) {
}

if (pkg.containsErrors()) {
FailureDetail failureDetail = pkg.contextualizeFailureDetailForTarget(target);
FailureDetail failureDetail =
Package.contextualizeFailureDetailForTarget(pkg.getFailureDetail(), target);
// The target can be loaded but may have associated errors, for example, a missing required
// attribute. In these cases, instead of failing fast, it's possible to perform dependency
// resolution using the target-in-error to uncover any other errors that could be present in
Expand Down
Loading

0 comments on commit e34043c

Please sign in to comment.