Skip to content

Commit

Permalink
Expose TargetsBelowDirectory as a visible subclass of TargetPattern: …
Browse files Browse the repository at this point in the history 
…it has too many dedicated methods for it to remain hidden.

PiperOrigin-RevId: 349560782
  • Loading branch information
@janakdr @copybara-github
janakdr authored and copybara-github committed Dec 30, 2020
1 parent e4535a0 commit 7b76683
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 254 deletions.
185 changes: 86 additions & 99 deletions src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ public abstract class TargetPattern implements Serializable {

private static final Parser DEFAULT_PARSER = new Parser(PathFragment.EMPTY_FRAGMENT);

private final Type type;
private final String originalPattern;
private final PathFragment offset;

Expand Down Expand Up @@ -117,9 +116,8 @@ static String normalize(String path) {
return SLASH_JOINER.join(pieces);
}

private TargetPattern(Type type, String originalPattern, PathFragment offset) {
private TargetPattern(String originalPattern, PathFragment offset) {
// Don't allow inheritance outside this class.
this.type = type;
this.originalPattern = Preconditions.checkNotNull(originalPattern);
this.offset = Preconditions.checkNotNull(offset);
}
Expand All @@ -128,9 +126,7 @@ private TargetPattern(Type type, String originalPattern, PathFragment offset) {
* Return the type of the pattern. Examples include "below directory" like "foo/..." and "single
* target" like "//x:y".
*/
public Type getType() {
return type;
}
public abstract Type getType();

/**
* Return the string that was parsed into this pattern.
Expand Down Expand Up @@ -208,72 +204,6 @@ public <T, E extends Exception> ListenableFuture<Void> evalAsync(
resolver, ignoredSubdirectories, excludedSubdirectories, callback, exceptionClass);
}

/**
* Returns {@code true} iff this pattern has type {@code Type.TARGETS_BELOW_DIRECTORY} and
* {@code directory} is contained by or equals this pattern's directory.
*
* <p>For example, returns {@code true} for {@code this = TargetPattern ("//...")} and
* {@code directory = "foo")}.
*/
public abstract boolean containsAllTransitiveSubdirectoriesForTBD(PackageIdentifier directory);

/** A tristate return value for {@link #containsTBDForTBD}. */
public enum ContainsTBDForTBDResult {
/**
* Evaluating this TBD pattern with a directory exclusion of the other TBD pattern's directory
* would result in exactly the same set of targets as evaluating the subtraction of the other
* TBD pattern from this one.
*/
DIRECTORY_EXCLUSION_WOULD_BE_EXACT,
/**
* A directory exclusion of the other TBD pattern's directory would be too broad because this
* TBD pattern is not "rules only" and the other one is, meaning that this TBD pattern
* potentially matches more targets underneath the directory in question than the other one
* does. Thus, a directory exclusion would incorrectly exclude non-rule targets.
*/
DIRECTORY_EXCLUSION_WOULD_BE_TOO_BROAD,
/**
* None of the above. Perhaps the other pattern isn't a TBD pattern or perhaps it's not
* contained by this pattern.
*/
OTHER,
}

/**
* Determines how, if it all, the evaluation of this TBD pattern with a directory exclusion of the
* given TBD {@code containedPattern}'s directory relates to the evaluation of the subtraction of
* the given {@code containedPattern} from this one.
*/
public ContainsTBDForTBDResult containsTBDForTBD(TargetPattern containedPattern) {
if (containedPattern.getType() != Type.TARGETS_BELOW_DIRECTORY) {
return ContainsTBDForTBDResult.OTHER;
} else if (containsAllTransitiveSubdirectoriesForTBD(
containedPattern.getDirectoryForTargetsUnderDirectory())) {
return !getRulesOnly() && containedPattern.getRulesOnly()
? ContainsTBDForTBDResult.DIRECTORY_EXCLUSION_WOULD_BE_TOO_BROAD
: ContainsTBDForTBDResult.DIRECTORY_EXCLUSION_WOULD_BE_EXACT;
} else {
return ContainsTBDForTBDResult.OTHER;
}

}

/**
* For patterns of type {@link Type#TARGETS_BELOW_DIRECTORY}, returns a {@link PackageIdentifier}
* identifying the most specific containing directory of the patterns that could be matched by
* this pattern.
*
* <p>Note that we are using the {@link PackageIdentifier} type as a convenience; there may not
* actually be a package corresponding to this directory!
*
* <p>This returns a {@link PackageIdentifier} that identifies the referred-to directory. For
* example, for a {@link Type#TARGETS_BELOW_DIRECTORY} corresponding to "//foo/bar/...", this
* method returns a {@link PackageIdentifier} for "foo/bar".
*/
public PackageIdentifier getDirectoryForTargetsUnderDirectory() {
throw new IllegalStateException();
}

/**
* For patterns of type {@link Type#PATH_AS_TARGET}, returns the path in question.
*
Expand Down Expand Up @@ -318,7 +248,7 @@ private SingleTarget(
PackageIdentifier directory,
String originalPattern,
PathFragment offset) {
super(Type.SINGLE_TARGET, originalPattern, offset);
super(originalPattern, offset);
this.targetName = Preconditions.checkNotNull(targetName);
this.directory = Preconditions.checkNotNull(directory);
}
Expand All @@ -334,11 +264,6 @@ public <T, E extends Exception> void eval(
callback.process(resolver.getExplicitTarget(label(targetName)).getTargets());
}

@Override
public boolean containsAllTransitiveSubdirectoriesForTBD(PackageIdentifier directory) {
return false;
}

@Override
public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() {
return directory;
Expand All @@ -359,6 +284,11 @@ public String getSingleTargetPath() {
return targetName;
}

@Override
public Type getType() {
return Type.SINGLE_TARGET;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -378,11 +308,10 @@ public int hashCode() {
}

private static final class InterpretPathAsTarget extends TargetPattern {

private final String path;

private InterpretPathAsTarget(String path, String originalPattern, PathFragment offset) {
super(Type.PATH_AS_TARGET, originalPattern, offset);
super(originalPattern, offset);
this.path = normalize(Preconditions.checkNotNull(path));
}

Expand Down Expand Up @@ -421,11 +350,6 @@ public <T, E extends Exception> void eval(
}
}

@Override
public boolean containsAllTransitiveSubdirectoriesForTBD(PackageIdentifier directory) {
return false;
}

@Override
public String getPathForPathAsTarget() {
return path;
Expand All @@ -443,6 +367,11 @@ public boolean getRulesOnly() {
return false;
}

@Override
public Type getType() {
return Type.PATH_AS_TARGET;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -462,7 +391,6 @@ public int hashCode() {
}

private static final class TargetsInPackage extends TargetPattern {

private final PackageIdentifier packageIdentifier;
private final String suffix;
private final boolean wasOriginallyAbsolute;
Expand All @@ -477,7 +405,7 @@ private TargetsInPackage(
boolean wasOriginallyAbsolute,
boolean rulesOnly,
boolean checkWildcardConflict) {
super(Type.TARGETS_IN_PACKAGE, originalPattern, offset);
super(originalPattern, offset);
Preconditions.checkArgument(!packageIdentifier.getRepository().isDefault());
this.packageIdentifier = packageIdentifier;
this.suffix = Preconditions.checkNotNull(suffix);
Expand Down Expand Up @@ -506,11 +434,6 @@ public <T, E extends Exception> void eval(
resolver.getTargetsInPackage(getOriginalPattern(), packageIdentifier, rulesOnly));
}

@Override
public boolean containsAllTransitiveSubdirectoriesForTBD(PackageIdentifier directory) {
return false;
}

@Override
public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() {
return packageIdentifier;
Expand All @@ -526,6 +449,11 @@ public boolean getRulesOnly() {
return rulesOnly;
}

@Override
public Type getType() {
return Type.TARGETS_IN_PACKAGE;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down Expand Up @@ -586,8 +514,13 @@ private <T> ResolvedTargets<T> getWildcardConflict(TargetPatternResolver<T> reso
}
}

private static final class TargetsBelowDirectory extends TargetPattern {

/**
* Specialization of {@link TargetPattern} for {@link Type#TARGETS_BELOW_DIRECTORY}. Exposed
* because it has a considerable number of specific methods. If {@link TargetPattern#getType}
* returns {@link Type#TARGETS_BELOW_DIRECTORY} the instance can safely be cast to {@code
* TargetsBelowDirectory}.
*/
public static final class TargetsBelowDirectory extends TargetPattern {
private final PackageIdentifier directory;
private final boolean rulesOnly;

Expand All @@ -596,7 +529,7 @@ private TargetsBelowDirectory(
PathFragment offset,
PackageIdentifier directory,
boolean rulesOnly) {
super(Type.TARGETS_BELOW_DIRECTORY, originalPattern, offset);
super(originalPattern, offset);
Preconditions.checkArgument(!directory.getRepository().isDefault());
this.directory = Preconditions.checkNotNull(directory);
this.rulesOnly = rulesOnly;
Expand Down Expand Up @@ -641,17 +574,66 @@ public <T, E extends Exception> ListenableFuture<Void> evalAsync(
executor);
}

@Override
public boolean containsAllTransitiveSubdirectoriesForTBD(PackageIdentifier containedDirectory) {
/**
* Returns true if {@code containedDirectory} is contained by or equals this pattern's
* directory.
*
* <p>For example, returns {@code true} for {@code this = TargetPattern ("//...")} and {@code
* directory = "foo")}.
*/
public boolean containsAllTransitiveSubdirectories(PackageIdentifier containedDirectory) {
// Note that merely checking to see if the directory startsWith the TargetsBelowDirectory's
// directory is insufficient. "food" begins with "foo", but "//foo/..." does not contain
// "//food/...".
return containedDirectory.getRepository().equals(directory.getRepository())
&& containedDirectory.getPackageFragment().startsWith(directory.getPackageFragment());
}

@Override
public PackageIdentifier getDirectoryForTargetsUnderDirectory() {
/**
* Determines how, if it all, the evaluation of this pattern with a directory exclusion of the
* given {@code containedPattern}'s directory relates to the evaluation of the subtraction of
* the given {@code containedPattern} from this one.
*/
public ContainsResult contains(TargetsBelowDirectory containedPattern) {
if (containsAllTransitiveSubdirectories(containedPattern.getDirectory())) {
return !getRulesOnly() && containedPattern.getRulesOnly()
? ContainsResult.DIRECTORY_EXCLUSION_WOULD_BE_TOO_BROAD
: ContainsResult.DIRECTORY_EXCLUSION_WOULD_BE_EXACT;
} else {
return ContainsResult.NOT_CONTAINED;
}
}

/** A tristate return value for {@link #contains}. */
public enum ContainsResult {
/**
* Evaluating this pattern with a directory exclusion of the other pattern's directory would
* result in exactly the same set of targets as evaluating the subtraction of the other
* pattern from this one.
*/
DIRECTORY_EXCLUSION_WOULD_BE_EXACT,
/**
* A directory exclusion of the other pattern's directory would be too broad because this
* pattern is not "rules only" and the other one is, meaning that this pattern potentially
* matches more targets underneath the directory in question than the other one does. Thus, a
* directory exclusion would incorrectly exclude non-rule targets.
*/
DIRECTORY_EXCLUSION_WOULD_BE_TOO_BROAD,
/** None of the above. The other pattern isn't contained by this pattern. */
NOT_CONTAINED,
}

/**
* Returns a {@link PackageIdentifier} identifying the most specific containing directory of the
* patterns that could be matched by this pattern.
*
* <p>Note that we are using the {@link PackageIdentifier} type as a convenience; there may not
* actually be a package corresponding to this directory!
*
* <p>This returns a {@link PackageIdentifier} that identifies the referred-to directory. For
* example, for "//foo/bar/...", this method returns a {@link PackageIdentifier} for "foo/bar".
*/
public PackageIdentifier getDirectory() {
return directory;
}

Expand All @@ -665,6 +647,11 @@ public boolean getRulesOnly() {
return rulesOnly;
}

@Override
public Type getType() {
return Type.TARGETS_BELOW_DIRECTORY;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
20 changes: 10 additions & 10 deletions ...main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.cmdline.TargetPattern.TargetsBelowDirectory;
import com.google.devtools.build.lib.concurrent.BatchCallback;
import com.google.devtools.build.lib.concurrent.ParallelVisitor.UnusedException;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
Expand All @@ -42,8 +43,6 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.WalkableGraph;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Set;

Expand Down Expand Up @@ -158,7 +157,7 @@ public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier pa
return packageLookupValue.packageExists();
}

private List<Root> checkValidDirectoryAndGetRoots(
private ImmutableList<Root> checkValidDirectoryAndGetRoots(
RepositoryName repository,
PathFragment directory,
ImmutableSet<PathFragment> ignoredSubdirectories,
Expand All @@ -171,9 +170,12 @@ private List<Root> checkValidDirectoryAndGetRoots(
// Check that this package is covered by at least one of our universe patterns.
boolean inUniverse = false;
for (TargetPattern pattern : universeTargetPatterns) {
boolean isTBD = pattern.getType().equals(TargetPattern.Type.TARGETS_BELOW_DIRECTORY);
if (!pattern.getType().equals(TargetPattern.Type.TARGETS_BELOW_DIRECTORY)) {
continue;
}
PackageIdentifier packageIdentifier = PackageIdentifier.create(repository, directory);
if (isTBD && pattern.containsAllTransitiveSubdirectoriesForTBD(packageIdentifier)) {
if (((TargetsBelowDirectory) pattern)
.containsAllTransitiveSubdirectories(packageIdentifier)) {
inUniverse = true;
break;
}
Expand All @@ -183,9 +185,8 @@ private List<Root> checkValidDirectoryAndGetRoots(
return ImmutableList.of();
}

List<Root> roots = new ArrayList<>();
if (repository.isMain()) {
roots.addAll(pkgRoots);
return pkgRoots;
} else {
RepositoryDirectoryValue repositoryValue =
(RepositoryDirectoryValue) graph.getValue(RepositoryDirectoryValue.key(repository));
Expand All @@ -194,9 +195,8 @@ private List<Root> checkValidDirectoryAndGetRoots(
// "nothing".
return ImmutableList.of();
}
roots.add(Root.fromPath(repositoryValue.getPath()));
return ImmutableList.of(Root.fromPath(repositoryValue.getPath()));
}
return roots;
}

@Override
Expand All @@ -208,7 +208,7 @@ public void streamPackagesUnderDirectory(
ImmutableSet<PathFragment> ignoredSubdirectories,
ImmutableSet<PathFragment> excludedSubdirectories)
throws InterruptedException {
List<Root> roots =
ImmutableList<Root> roots =
checkValidDirectoryAndGetRoots(
repository, directory, ignoredSubdirectories, excludedSubdirectories);

Expand Down
Loading

0 comments on commit 7b76683

Please sign in to comment.