Clean up Package and Package.Builder interface
A lot of stuff was a lot more exposed than it needed to be. This reduces the
number of ways we can create, mutate, and generally interact with packages.
This only grabs the low hanging fruit, there's definitely more.
PiperOrigin-RevId: 214871508
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 18ea46d..fff662c 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
@@ -74,7 +74,7 @@
* Common superclass for all name-conflict exceptions.
*/
public static class NameConflictException extends Exception {
- protected NameConflictException(String message) {
+ private NameConflictException(String message) {
super(message);
}
}
@@ -87,7 +87,7 @@
/**
* The name of the package, e.g. "foo/bar".
*/
- protected final String name;
+ private final String name;
/**
* Like name, but in the form of a PathFragment.
@@ -97,7 +97,7 @@
/**
* The filename of this package's BUILD file.
*/
- protected Path filename;
+ private Path filename;
/**
* The directory in which this package's BUILD file resides. All InputFile
@@ -109,7 +109,7 @@
* The name of the workspace this package is in. Used as a prefix for the runfiles directory.
* This can be set in the WORKSPACE file. This must be a valid target name.
*/
- protected String workspaceName;
+ private String workspaceName;
/**
* The root of the source tree in which this package was found. It is an invariant that {@code
@@ -124,7 +124,7 @@
private ImmutableMap<String, String> makeEnv;
/** The collection of all targets defined in this package, indexed by name. */
- protected ImmutableSortedKeyMap<String, Target> targets;
+ private ImmutableSortedKeyMap<String, Target> targets;
/**
* Default visibility for rules that do not specify it.
@@ -246,49 +246,19 @@
*/
public ImmutableMap<RepositoryName, RepositoryName> getRepositoryMapping(
RepositoryName repository) {
- if (!isWorkspace()) {
+ if (!packageIdentifier.equals(Label.EXTERNAL_PACKAGE_IDENTIFIER)) {
throw new UnsupportedOperationException("Can only access the external package repository"
+ "mappings from the //external package");
}
return externalPackageRepositoryMappings.getOrDefault(repository, ImmutableMap.of());
}
- /**
- * Returns the repository mapping for the requested external repository.
- *
- * @throws LabelSyntaxException if repository is not a valid {@link RepositoryName}
- * @throws UnsupportedOperationException if called from any package other than the //external
- * package
- */
- public ImmutableMap<RepositoryName, RepositoryName> getRepositoryMapping(String repository)
- throws LabelSyntaxException, UnsupportedOperationException {
- RepositoryName repositoryName = RepositoryName.create(repository);
- return getRepositoryMapping(repositoryName);
- }
-
/** Get the repository mapping for this package. */
public ImmutableMap<RepositoryName, RepositoryName> getRepositoryMapping() {
return repositoryMapping;
}
/**
- * Gets the global name for a repository within an external repository.
- *
- * <p>{@code localName} is a repository name reference found in a BUILD file within a repository
- * external to the main workspace. This method returns the main workspace's global remapped name
- * for {@code localName}.
- */
- public RepositoryName getGlobalName(RepositoryName localName) {
- RepositoryName globalname = repositoryMapping.get(localName);
- return globalname != null ? globalname : localName;
- }
-
- /** Returns whether we are in the WORKSPACE file or not. */
- public boolean isWorkspace() {
- return getPackageIdentifier().equals(Label.EXTERNAL_PACKAGE_IDENTIFIER);
- }
-
- /**
* Package initialization: part 2 of 3: sets this package's default header
* strictness checking.
*
@@ -296,21 +266,21 @@
* which accesses {@link #getDefaultHdrsCheck} from the still-under-construction
* package.
*/
- protected void setDefaultHdrsCheck(String defaultHdrsCheck) {
+ private void setDefaultHdrsCheck(String defaultHdrsCheck) {
this.defaultHdrsCheck = defaultHdrsCheck;
}
/**
* Set the default 'testonly' value for this package.
*/
- protected void setDefaultTestOnly(boolean testOnly) {
+ private void setDefaultTestOnly(boolean testOnly) {
defaultTestOnly = testOnly;
}
/**
* Set the default 'deprecation' value for this package.
*/
- protected void setDefaultDeprecation(String deprecation) {
+ private void setDefaultDeprecation(String deprecation) {
defaultDeprecation = deprecation;
}
@@ -318,7 +288,7 @@
* Sets the default value to use for a rule's {@link RuleClass#COMPATIBLE_ENVIRONMENT_ATTR}
* attribute when not explicitly specified by the rule.
*/
- protected void setDefaultCompatibleWith(Set<Label> environments) {
+ private void setDefaultCompatibleWith(Set<Label> environments) {
defaultCompatibleWith = environments;
}
@@ -326,7 +296,7 @@
* Sets the default value to use for a rule's {@link RuleClass#RESTRICTED_ENVIRONMENT_ATTR}
* attribute when not explicitly specified by the rule.
*/
- protected void setDefaultRestrictedTo(Set<Label> environments) {
+ private void setDefaultRestrictedTo(Set<Label> environments) {
defaultRestrictedTo = environments;
}
@@ -360,7 +330,7 @@
* <p>Only after this method is called can this package be considered "complete"
* and be shared publicly.
*/
- protected void finishInit(Builder builder) {
+ private void finishInit(Builder builder) {
// If any error occurred during evaluation of this package, consider all
// rules in the package to be "in error" also (even if they were evaluated
// prior to the error). This behaviour is arguably stricter than need be,
@@ -603,7 +573,7 @@
protected NoSuchTargetException makeNoSuchTargetException(String targetName, String suffix) {
Label label;
try {
- label = createLabel(targetName);
+ label = Label.create(packageIdentifier, targetName);
} catch (LabelSyntaxException e) {
throw new IllegalArgumentException(targetName);
}
@@ -617,15 +587,6 @@
}
/**
- * Creates a label for a target inside this package.
- *
- * @throws LabelSyntaxException if the {@code targetName} is invalid
- */
- public Label createLabel(String targetName) throws LabelSyntaxException {
- return Label.create(packageIdentifier, targetName);
- }
-
- /**
* Returns the default visibility for this package.
*/
public RuleVisibility getDefaultVisibility() {
@@ -807,7 +768,7 @@
* are applied during {@link #build}. See {@link Package#Package}
* and {@link Package#finishInit} for details.
*/
- protected Package pkg;
+ private final Package pkg;
// 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.
@@ -829,20 +790,20 @@
private List<String> features = new ArrayList<>();
private List<Event> events = Lists.newArrayList();
private List<Postable> posts = Lists.newArrayList();
- @Nullable String ioExceptionMessage = null;
+ @Nullable private String ioExceptionMessage = null;
@Nullable private IOException ioException = null;
private boolean containsErrors = false;
private License defaultLicense = License.NO_LICENSE;
private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
- protected Map<String, Target> targets = new HashMap<>();
- protected Map<Label, EnvironmentGroup> environmentGroups = new HashMap<>();
+ private Map<String, Target> targets = new HashMap<>();
+ private final Map<Label, EnvironmentGroup> environmentGroups = new HashMap<>();
- protected ImmutableList<Label> skylarkFileDependencies = ImmutableList.of();
+ private ImmutableList<Label> skylarkFileDependencies = ImmutableList.of();
- protected List<String> registeredExecutionPlatforms = new ArrayList<>();
- protected List<String> registeredToolchains = new ArrayList<>();
+ private final List<String> registeredExecutionPlatforms = new ArrayList<>();
+ private final List<String> registeredToolchains = new ArrayList<>();
/**
* True iff the "package" function has already been called in this package.
@@ -869,26 +830,30 @@
}
};
- protected Builder(Package pkg) {
+ Builder(Package pkg) {
this.pkg = pkg;
if (pkg.getName().startsWith("javatests/")) {
setDefaultTestonly(true);
}
}
- public Builder(Helper helper, PackageIdentifier id, String runfilesPrefix) {
+ Builder(Helper helper, PackageIdentifier id, String runfilesPrefix) {
this(helper.createFreshPackage(id, runfilesPrefix));
}
- protected PackageIdentifier getPackageIdentifier() {
+ PackageIdentifier getPackageIdentifier() {
return pkg.getPackageIdentifier();
}
/** Determine if we are in the WORKSPACE file or not */
- public boolean isWorkspace() {
+ boolean isWorkspace() {
return pkg.getPackageIdentifier().equals(Label.EXTERNAL_PACKAGE_IDENTIFIER);
}
+ String getPackageWorkspaceName() {
+ return pkg.getWorkspaceName();
+ }
+
/**
* Updates the externalPackageRepositoryMappings entry for {@code repoWithin}. Adds new
* entry from {@code localName} to {@code mappedName} in {@code repoWithin}'s map.
@@ -898,7 +863,7 @@
* in the {@code repoWithin} repository
* @param mappedName the RepositoryName by which localName should be referenced
*/
- public Builder addRepositoryMappingEntry(
+ Builder addRepositoryMappingEntry(
RepositoryName repoWithin, RepositoryName localName, RepositoryName mappedName) {
HashMap<RepositoryName, RepositoryName> mapping =
externalPackageRepositoryMappings
@@ -908,7 +873,7 @@
}
/** Adds all the mappings from a given {@link Package}. */
- public Builder addRepositoryMappings(Package aPackage) {
+ Builder addRepositoryMappings(Package aPackage) {
ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
repositoryMappings = aPackage.externalPackageRepositoryMappings;
for (Map.Entry<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>> repositoryName :
@@ -953,7 +918,7 @@
return this;
}
- public Label getBuildFileLabel() {
+ Label getBuildFileLabel() {
return buildFileLabel;
}
@@ -1018,11 +983,11 @@
/**
* Returns whether the "package" function has been called yet
*/
- public boolean isPackageFunctionUsed() {
+ boolean isPackageFunctionUsed() {
return packageFunctionUsed;
}
- public void setPackageFunctionUsed() {
+ void setPackageFunctionUsed() {
packageFunctionUsed = true;
}
@@ -1043,7 +1008,7 @@
return this;
}
- public Builder addFeatures(Iterable<String> features) {
+ Builder addFeatures(Iterable<String> features) {
Iterables.addAll(this.features, features);
return this;
}
@@ -1066,19 +1031,14 @@
return containsErrors;
}
- public Builder addPosts(Iterable<Postable> posts) {
+ Builder addPosts(Iterable<Postable> posts) {
for (Postable post : posts) {
- addPost(post);
+ this.posts.add(post);
}
return this;
}
- public Builder addPost(Postable post) {
- this.posts.add(post);
- return this;
- }
-
- public Builder addEvents(Iterable<Event> events) {
+ Builder addEvents(Iterable<Event> events) {
for (Event event : events) {
addEvent(event);
}
@@ -1188,10 +1148,21 @@
}
@Nullable
- public Target getTarget(String name) {
+ Target getTarget(String name) {
return targets.get(name);
}
+ /**
+ * Removes a target from the {@link Package} under construction. Intended to be used only by
+ * {@link com.google.devtools.build.lib.skyframe.PackageFunction} to remove targets whose
+ * labels cross subpackage boundaries.
+ */
+ public void removeTarget(Target target) {
+ if (target.getPackage() == pkg) {
+ this.targets.remove(target.getName());
+ }
+ }
+
public Collection<Target> getTargets() {
return Package.getTargets(targets);
}
@@ -1200,7 +1171,7 @@
* Returns an (immutable, unordered) view of all the targets belonging to
* this package which are instances of the specified class.
*/
- <T extends Target> Iterable<T> getTargets(Class<T> targetClass) {
+ private <T extends Target> Iterable<T> getTargets(Class<T> targetClass) {
return Package.getTargets(targets, targetClass);
}
@@ -1381,7 +1352,7 @@
addRuleUnchecked(rule);
}
- public void addRegisteredExecutionPlatforms(List<String> platforms) {
+ void addRegisteredExecutionPlatforms(List<String> platforms) {
this.registeredExecutionPlatforms.addAll(platforms);
}
@@ -1389,8 +1360,7 @@
this.registeredToolchains.addAll(toolchains);
}
- private Builder beforeBuild(boolean discoverAssumedInputFiles)
- throws InterruptedException, NoSuchPackageException {
+ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPackageException {
Preconditions.checkNotNull(pkg);
Preconditions.checkNotNull(filename);
Preconditions.checkNotNull(buildFileLabel);
@@ -1441,24 +1411,13 @@
}
/** Intended for use by {@link com.google.devtools.build.lib.skyframe.PackageFunction} only. */
- public Builder buildPartial() throws InterruptedException, NoSuchPackageException {
+ public Builder buildPartial() throws NoSuchPackageException {
if (alreadyBuilt) {
return this;
}
return beforeBuild(/*discoverAssumedInputFiles=*/ true);
}
- /**
- * Removes a target from the {@link Package} under construction. Intended to be used only by
- * {@link com.google.devtools.build.lib.skyframe.PackageFunction} to remove targets whose
- * labels cross subpackage boundaries.
- */
- public void removeTarget(Target target) {
- if (target.getPackage() == pkg) {
- this.targets.remove(target.getName());
- }
- }
-
/** Intended for use by {@link com.google.devtools.build.lib.skyframe.PackageFunction} only. */
public Package finishBuild() {
if (alreadyBuilt) {
@@ -1485,7 +1444,7 @@
return pkg;
}
- public Package build() throws InterruptedException, NoSuchPackageException {
+ public Package build() throws NoSuchPackageException {
return build(/*discoverAssumedInputFiles=*/ true);
}
@@ -1493,8 +1452,7 @@
* Build the package, optionally adding any labels in the package not already associated with a
* target as an input file.
*/
- public Package build(boolean discoverAssumedInputFiles)
- throws InterruptedException, NoSuchPackageException {
+ Package build(boolean discoverAssumedInputFiles) throws NoSuchPackageException {
if (alreadyBuilt) {
return pkg;
}
@@ -1506,7 +1464,7 @@
* If "label" refers to a non-existent target in the current package, create
* an InputFile target.
*/
- void createInputFileMaybe(Label label, Location location) {
+ private void createInputFileMaybe(Label label, Location location) {
if (label != null && label.getPackageIdentifier().equals(pkg.getPackageIdentifier())) {
if (!targets.containsKey(label.getName())) {
addInputFile(label, location);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index e0e6f09..4a7ebec 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -478,9 +478,10 @@
throws EvalException, InterruptedException {
try {
Package.Builder builder = PackageFactory.getContext(env, ast.getLocation()).pkgBuilder;
+ String externalRepoName = (String) kwargs.get("name");
if (!allowOverride
- && kwargs.containsKey("name")
- && builder.targets.containsKey(kwargs.get("name"))) {
+ && externalRepoName != null
+ && builder.getTarget(externalRepoName) != null) {
throw new EvalException(
ast.getLocation(),
"Cannot redefine repository after any load statement in the WORKSPACE file"
@@ -488,15 +489,14 @@
+ kwargs.get("name")
+ "')");
}
- String externalRepoName = (String) kwargs.get("name");
// Add an entry in every repository from @<mainRepoName> to "@" to avoid treating
// @<mainRepoName> as a separate repository. This will be overridden if the main
// repository has a repo_mapping entry from <mainRepoName> to something.
if (env.getSemantics().experimentalRemapMainRepo()) {
- if (!Strings.isNullOrEmpty(builder.pkg.getWorkspaceName())) {
+ if (!Strings.isNullOrEmpty(builder.getPackageWorkspaceName())) {
builder.addRepositoryMappingEntry(
RepositoryName.createFromValidStrippedName(externalRepoName),
- RepositoryName.createFromValidStrippedName(builder.pkg.getWorkspaceName()),
+ RepositoryName.createFromValidStrippedName(builder.getPackageWorkspaceName()),
RepositoryName.MAIN);
}
}
@@ -514,8 +514,8 @@
for (Map.Entry<String, String> e : map.entrySet()) {
builder.addRepositoryMappingEntry(
RepositoryName.createFromValidStrippedName(externalRepoName),
- RepositoryName.create((String) e.getKey()),
- RepositoryName.create((String) e.getValue()));
+ RepositoryName.create(e.getKey()),
+ RepositoryName.create(e.getValue()));
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
index 2a264e4..566f560 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
@@ -103,13 +103,13 @@
private static void overwriteRule(Package.Builder pkg, Rule rule)
throws Package.NameConflictException, InterruptedException {
Preconditions.checkArgument(rule.getOutputFiles().isEmpty());
- Target old = pkg.targets.get(rule.getName());
+ Target old = pkg.getTarget(rule.getName());
if (old != null) {
if (old instanceof Rule) {
Verify.verify(((Rule) old).getOutputFiles().isEmpty());
}
- pkg.targets.remove(rule.getName());
+ pkg.removeTarget(rule);
}
pkg.addRule(rule);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 405ef5e..d72c676 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -53,6 +53,7 @@
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.skyframe.GlobValue.InvalidGlobPatternException;
+import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.syntax.BuildFileAST;
import com.google.devtools.build.lib.syntax.Environment.Extension;
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
index c806d0f..bfd84c9 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
@@ -249,7 +249,7 @@
Rule rule =
new Rule(
pkg,
- pkg.createLabel("myrule"),
+ Label.create(pkg.getPackageIdentifier(), "myrule"),
ruleClass,
Location.fromFile(myPkgPath),
new AttributeContainer(ruleClass));
diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
index c964539..017ec02 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTest.java
@@ -130,7 +130,7 @@
")");
assertMapping(helper, "@foo", "@x", "@y");
}
-
+
@Test
public void testMultipleRepositoriesWithMappings() throws Exception {
helper.setSkylarkSemantics("--experimental_enable_repo_mapping");
@@ -172,7 +172,7 @@
" path = '/foo',",
" repo_mapping = {},",
")");
- assertThat(helper.getPackage().getRepositoryMapping("@foo")).isEmpty();
+ assertThat(helper.getPackage().getRepositoryMapping(RepositoryName.create("@foo"))).isEmpty();
}
@Test
@@ -208,14 +208,14 @@
public void testNoImplicitMainRepoRenameWithoutFlag() throws Exception {
helper.parse("workspace(name = 'foo')");
RepositoryName foo = RepositoryName.create("@foo");
- assertThat(helper.getPackage().getRepositoryMapping("@"))
+ assertThat(helper.getPackage().getRepositoryMapping(RepositoryName.create("@")))
.doesNotContainEntry(foo, RepositoryName.MAIN);
}
@Test
public void testEmptyRepositoryHasEmptyMap() throws Exception {
helper.parse("");
- assertThat(helper.getPackage().getRepositoryMapping("@")).isEmpty();
+ assertThat(helper.getPackage().getRepositoryMapping(RepositoryName.create("@"))).isEmpty();
}
@Test
@@ -237,7 +237,7 @@
throws Exception {
RepositoryName localRepoName = RepositoryName.create(local);
RepositoryName globalRepoName = RepositoryName.create(global);
- assertThat(helper.getPackage().getRepositoryMapping(repo))
+ assertThat(helper.getPackage().getRepositoryMapping(RepositoryName.create(repo)))
.containsEntry(localRepoName, globalRepoName);
}
