Skip to content

Commit

Permalink
Make Package.Metadata a record class
Browse files Browse the repository at this point in the history 
This codifies the previous CLs' efforts to make `Package.Metadata` immutable. It also saves a few LOC.

- Accessors on `Metadata` are deleted, in favor of the implicit ones generated by `record`. Updated usages to prefer these accessors over direct access to the fields. `Metadata#getPackageDirectory()` remains as an explicit accessor since it's not materialized as a field.
- Add `@Nullable` for `configSettingVisibilityPolicy`.
- Rename `Metadata#filename` -> `Metadata#buildFilename` for clarity

Work toward #19922.

PiperOrigin-RevId: 676389043
Change-Id: Id40dab239d160681be69b48d111ac8c32b475163
  • Loading branch information
@brandjon @iancha1992
brandjon authored and iancha1992 committed Sep 20, 2024
1 parent 61751b3 commit 8d0e4db
Showing 1 changed file with 70 additions and 117 deletions.
187 changes: 70 additions & 117 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 @@ -290,35 +290,39 @@ public Metadata getMetadata() {
return metadata;
}

/** Returns this package's identifier. */
/**
* Returns this package's identifier.
*
* <p>This is a suffix of {@code getFilename().getParentDirectory()}.
*/
public PackageIdentifier getPackageIdentifier() {
return metadata.packageIdentifier;
return metadata.packageIdentifier();
}

/**
* Returns the name of this package. If this build is using external repositories then this name
* may not be unique!
*/
public String getName() {
return metadata.getName();
return metadata.packageIdentifier().getPackageFragment().getPathString();
}

/** Like {@link #getName}, but has type {@code PathFragment}. */
public PathFragment getNameFragment() {
return metadata.getNameFragment();
return getPackageIdentifier().getPackageFragment();
}

/**
* Returns the filename of the BUILD file which defines this package. The parent directory of the
* BUILD file is the package directory.
*/
public RootedPath getFilename() {
return metadata.filename;
return metadata.buildFilename();
}

/** Returns the directory containing the package's BUILD file. */
public Path getPackageDirectory() {
return metadata.packageDirectory;
return metadata.getPackageDirectory();
}

/**
Expand All @@ -329,17 +333,23 @@ private boolean isRepoRulePackage() {
return metadata.isRepoRulePackage;
}

/** Get the repository mapping for this package. */
/**
* Returns the map of repository reassignments for BUILD packages. This will be empty for packages
* within the main workspace.
*/
public RepositoryMapping getRepositoryMapping() {
return metadata.repositoryMapping;
return metadata.repositoryMapping();
}

/**
* How to enforce visibility on <code>config_setting</code> See {@link
* ConfigSettingVisibilityPolicy} for details.
*
* <p>Null for repo rule packages.
*/
@Nullable
public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() {
return metadata.configSettingVisibilityPolicy;
return metadata.configSettingVisibilityPolicy();
}

/**
Expand Down Expand Up @@ -617,12 +627,12 @@ public Target getTarget(String targetName) throws NoSuchTargetException {

Label label;
try {
label = Label.create(metadata.packageIdentifier, targetName);
label = Label.create(metadata.packageIdentifier(), targetName);
} catch (LabelSyntaxException e) {
throw new IllegalArgumentException(targetName, e);
}

if (metadata.succinctTargetNotFoundErrors) {
if (metadata.succinctTargetNotFoundErrors()) {
throw new NoSuchTargetException(
label, String.format("target '%s' not declared in package '%s'", targetName, getName()));
} else {
Expand All @@ -633,7 +643,7 @@ public Target getTarget(String targetName) throws NoSuchTargetException {
"target '%s' not declared in package '%s' defined by %s%s",
targetName,
getName(),
metadata.filename.asPath().getPathString(),
metadata.buildFilename().asPath().getPathString(),
alternateTargetSuggestion));
}
}
Expand All @@ -643,7 +653,7 @@ private String getAlternateTargetSuggestion(String targetName) {
// produce a more informative error. NOTE! this code path is only executed
// on failure, which is (relatively) very rare. In the common case no
// stat(2) is executed.
Path filename = metadata.packageDirectory.getRelative(targetName);
Path filename = metadata.getPackageDirectory().getRelative(targetName);
if (!PathFragment.isNormalized(targetName) || "*".equals(targetName)) {
// Don't check for file existence if the target name is not normalized
// because the error message would be confusing and wrong. If the
Expand Down Expand Up @@ -699,7 +709,7 @@ public MacroInstance getDeclaringMacroForTarget(String target) {
* publicly.
*/
private void finishInit(Builder builder) {
String baseName = metadata.filename.getRootRelativePath().getBaseName();
String baseName = metadata.buildFilename().getRootRelativePath().getBaseName();

this.containsErrors |= builder.containsErrors;
if (directLoads == null && transitiveLoads == null) {
Expand All @@ -712,22 +722,22 @@ private void finishInit(Builder builder) {
this.sourceRoot = Optional.empty();
} else {
Root sourceRoot =
computeSourceRoot(metadata.filename, metadata.packageIdentifier.getSourceRoot());
computeSourceRoot(metadata.buildFilename(), metadata.packageIdentifier().getSourceRoot());
if (sourceRoot.asPath() == null
|| !sourceRoot
.getRelative(metadata.packageIdentifier.getSourceRoot())
.equals(metadata.packageDirectory)) {
.getRelative(metadata.packageIdentifier().getSourceRoot())
.equals(metadata.getPackageDirectory())) {
throw new IllegalArgumentException(
"Invalid BUILD file name for package '"
+ metadata.packageIdentifier
+ metadata.packageIdentifier()
+ "': "
+ metadata.filename
+ metadata.buildFilename()
+ " (in source "
+ sourceRoot
+ " with packageDirectory "
+ metadata.packageDirectory
+ metadata.getPackageDirectory()
+ " and package identifier source root "
+ metadata.packageIdentifier.getSourceRoot()
+ metadata.packageIdentifier().getSourceRoot()
+ ")");
}
this.sourceRoot = Optional.of(sourceRoot);
Expand Down Expand Up @@ -816,7 +826,7 @@ public String toString() {
* output.
*/
public void dump(PrintStream out) {
out.println(" Package " + getName() + " (" + metadata.filename.asPath() + ")");
out.println(" Package " + getName() + " (" + metadata.buildFilename().asPath() + ")");

// Rules:
out.println(" Rules");
Expand Down Expand Up @@ -1005,6 +1015,9 @@ public interface PackageSettings {
* thrown from {@link #getTarget}. Useful for toning down verbosity in situations where it can
* be less helpful.
*/
// TODO(bazel-team): Arguably, this could be replaced by a boolean param to getTarget(), or
// some separate action taken by the caller. But there's a lot of call sites that would need
// updating.
default boolean succinctTargetNotFoundErrors() {
return false;
}
Expand Down Expand Up @@ -1312,29 +1325,26 @@ private Builder(

this.workspaceName = Preconditions.checkNotNull(workspaceName);

Metadata metadata = new Metadata();
metadata.packageIdentifier = Preconditions.checkNotNull(id);

metadata.filename = filename;
metadata.packageDirectory = filename.asPath().getParentDirectory();
Metadata metadata =
new Metadata(
/* packageIdentifier= */ id,
/* buildFilename= */ filename,
/* isRepoRulePackage= */ id.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)
// For bzlmod packages, setWorkspaceName() is not called, so this check doesn't
// change during package evaluation.
|| workspaceName.equals(DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES),
/* repositoryMapping= */ repositoryMapping,
/* associatedModuleName= */ associatedModuleName,
/* associatedModuleVersion= */ associatedModuleVersion,
/* configSettingVisibilityPolicy= */ configSettingVisibilityPolicy,
/* succinctTargetNotFoundErrors= */ packageSettings.succinctTargetNotFoundErrors());
try {
buildFileLabel = Label.create(id, filename.getRootRelativePath().getBaseName());
} catch (LabelSyntaxException e) {
// This can't actually happen.
throw new AssertionError("Package BUILD file has an illegal name: " + filename, e);
}

metadata.isRepoRulePackage =
metadata.packageIdentifier.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)
// For bzlmod packages, setWorkspaceName() is not called, so this check doesn't change
// during package evaluation.
|| workspaceName.equals(DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES);
metadata.repositoryMapping = Preconditions.checkNotNull(repositoryMapping);
metadata.associatedModuleName = Preconditions.checkNotNull(associatedModuleName);
metadata.associatedModuleVersion = Preconditions.checkNotNull(associatedModuleVersion);
metadata.succinctTargetNotFoundErrors = packageSettings.succinctTargetNotFoundErrors();
metadata.configSettingVisibilityPolicy = configSettingVisibilityPolicy;

this.pkg = new Package(metadata);

this.precomputeTransitiveLoads = packageSettings.precomputeTransitiveLoads();
Expand Down Expand Up @@ -1539,7 +1549,7 @@ String getWorkspaceName() {
* this is the name of the module hosting the extension.
*/
Optional<String> getAssociatedModuleName() {
return pkg.metadata.associatedModuleName;
return pkg.metadata.associatedModuleName();
}

/**
Expand All @@ -1548,7 +1558,7 @@ Optional<String> getAssociatedModuleName() {
* this is the version of the module hosting the extension.
*/
Optional<String> getAssociatedModuleVersion() {
return pkg.metadata.associatedModuleVersion;
return pkg.metadata.associatedModuleVersion();
}

/**
Expand Down Expand Up @@ -1616,7 +1626,7 @@ RepositoryMapping getRepositoryMappingFor(RepositoryName name) {
}

RootedPath getFilename() {
return pkg.metadata.filename;
return pkg.metadata.buildFilename();
}

/** Returns the {@link StoredEventHandler} associated with this builder. */
Expand Down Expand Up @@ -2744,89 +2754,32 @@ public Semaphore getCpuBoundSemaphore() {
}

/** A collection of data that is known before BUILD file evaluation even begins. */
public static final class Metadata {

private Metadata() {}

private PackageIdentifier packageIdentifier;

/**
* Returns the package identifier for this package.
*
* <p>This is a suffix of {@code getFilename().getParentDirectory()}.
*/
public PackageIdentifier getPackageIdentifier() {
return packageIdentifier;
}

/**
* Returns the name of this package. If this build is using external repositories then this name
* may not be unique!
*/
public String getName() {
return packageIdentifier.getPackageFragment().getPathString();
}

/** Like {@link #getName}, but has type {@code PathFragment}. */
public PathFragment getNameFragment() {
return packageIdentifier.getPackageFragment();
}

private RootedPath filename;
public record Metadata(
PackageIdentifier packageIdentifier,
RootedPath buildFilename,
boolean isRepoRulePackage,
RepositoryMapping repositoryMapping,
Optional<String> associatedModuleName,
Optional<String> associatedModuleVersion,
@Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy,
boolean succinctTargetNotFoundErrors) {

/** Returns the filename of this package's BUILD file. */
public RootedPath getFilename() {
return filename;
public Metadata {
Preconditions.checkNotNull(packageIdentifier);
Preconditions.checkNotNull(buildFilename);
Preconditions.checkNotNull(repositoryMapping);
Preconditions.checkNotNull(associatedModuleName);
Preconditions.checkNotNull(associatedModuleVersion);
}

private Path packageDirectory;

/**
* Returns the directory in which this package's BUILD file resides. All InputFile members of
* the packages are located relative to this directory.
* Returns the directory in which this package's BUILD file resides.
*
* <p>All InputFile members of the packages are located relative to this directory.
*/
public Path getPackageDirectory() {
return packageDirectory;
}

private boolean isRepoRulePackage;

private RepositoryMapping repositoryMapping;

/**
* Returns the map of repository reassignments for BUILD packages. This will be empty for
* packages within the main workspace.
*/
public RepositoryMapping getRepositoryMapping() {
return repositoryMapping;
return buildFilename.asPath().getParentDirectory();
}

/**
* The name of the Bzlmod module associated with the repo this package is in. If this package is
* not from a Bzlmod repo, this is empty. For repos generated by module extensions, this is the
* name of the module hosting the extension.
*/
private Optional<String> associatedModuleName;

/**
* The version of the Bzlmod module associated with the repo this package is in. If this package
* is not from a Bzlmod repo, this is empty. For repos generated by module extensions, this is
* the version of the module hosting the extension.
*/
private Optional<String> associatedModuleVersion;

private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;

/** Returns the visibility enforcement policy for {@code config_setting}. */
public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() {
return configSettingVisibilityPolicy;
}

/** Governs the error message behavior of {@link Package#getTarget}. */
// TODO(bazel-team): Arguably, this could be replaced by a boolean param to getTarget(), or some
// separate action taken by the caller. But there's a lot of call sites that would need
// updating.
private boolean succinctTargetNotFoundErrors;
}

/** Package codec implementation. */
Expand Down

0 comments on commit 8d0e4db

Please sign in to comment.