Skip to content

Commit

Permalink
Inline confusing OutputArtifactWithoutDigest.getKey()
Browse files Browse the repository at this point in the history
and replace with its default and only implementation.

PiperOrigin-RevId: 568180984
  • Loading branch information
Googler authored and copybara-github committed Sep 26, 2023
1 parent 3298278 commit c285c84
Show file tree
Hide file tree
Showing 15 changed files with 47 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public final class UnpackedAarUtils {
/* Converts {@link BlazeArtifact} to the key which is used to create aar directory name */
public static String getArtifactKey(BlazeArtifact artifact) {
if (artifact instanceof OutputArtifactWithoutDigest) {
return ((OutputArtifactWithoutDigest) artifact).getKey();
return ((OutputArtifactWithoutDigest) artifact).getRelativePath();
}
if (artifact instanceof SourceArtifact) {
return ((SourceArtifact) artifact).getFile().getPath();
Expand Down Expand Up @@ -67,8 +67,7 @@ public static String getSrcJarName(BlazeArtifact srcJar) {
* Returns aar directory name in the format of <name>_<hashcode>. This provides flexibility to
* caller to generate aar directory name to deal with different cases. But this method cannot
* guarantee the uniqueness.
*
*
* @param name the file name of aar file without extension.
* @param hashCode the file name of aar is not guaranteed to be unique, so a hash code is used to
* make sure the directory name is unique. Caller needs to make sure the hashcode provided is
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public AndroidDeployInfo readDeployInfoProtoForTarget(
log.warn("Local execroot: " + bepOutput.getLocalExecRoot());
log.warn("All output artifacts:");
for (OutputArtifact outputArtifact : bepOutput.getAllOutputArtifacts(path -> true)) {
log.warn(outputArtifact.getKey() + " -> " + outputArtifact.getRelativePath());
log.warn(outputArtifact.getRelativePath() + " -> " + outputArtifact.getRelativePath());
}
log.warn("All local artifacts for " + target + ":");
List<OutputArtifact> allBuildArtifacts =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@
public class BepArtifactData {

public final OutputArtifact artifact;

/** The output groups this artifact belongs to. */
public final ImmutableSet<String> outputGroups;

/** The top-level targets this artifact is transitively associated with. */
public final ImmutableSet<String> topLevelTargets;

Expand All @@ -43,7 +45,7 @@ public class BepArtifactData {

@Override
public int hashCode() {
return artifact.getKey().hashCode();
return artifact.getRelativePath().hashCode();
}

@Override
Expand All @@ -61,7 +63,7 @@ public BepArtifactData removeTargetAssociation(String target) {

/** Combines this data with a newer version. */
public BepArtifactData update(BepArtifactData newer) {
Preconditions.checkState(artifact.getKey().equals(newer.artifact.getKey()));
Preconditions.checkState(artifact.getRelativePath().equals(newer.artifact.getRelativePath()));
return new BepArtifactData(
newer.artifact,
Sets.union(outputGroups, newer.outputGroups).immutableCopy(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private long getLastModifiedTime() {
@Nullable
public ArtifactState toArtifactState() {
long lastModifiedTime = getLastModifiedTime();
return lastModifiedTime == 0 ? null : new LocalFileState(getKey(), lastModifiedTime);
return lastModifiedTime == 0 ? null : new LocalFileState(getRelativePath(), lastModifiedTime);
}

@Override
Expand Down Expand Up @@ -93,5 +93,4 @@ public int hashCode() {
public String toString() {
return blazeOutRelativePath;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,6 @@ public interface OutputArtifactWithoutDigest extends BlazeArtifact, OutputArtifa
/** The path component related to the build configuration. */
String getConfigurationMnemonic();

/**
* A key uniquely identifying an artifact between builds. Different versions of an artifact
* produced from different blaze builds will have the same key.
*
* <p>TODO(brendandouglas): remove this in favor of ArtifactState#getKey
*/
default String getKey() {
return getRelativePath();
}

/**
* Returns the {@link ArtifactState} for this output, used for serialization/diffing purposes. Can
* require file system operations.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,8 @@ public ImmutableList<OutputArtifact> getOutputGroupArtifacts(String outputGroup)
public ImmutableMap<String, BepArtifactData> getFullArtifactData() {
return fileSets.values().stream()
.flatMap(FileSet::toPerArtifactData)
.collect(toImmutableMap(d -> d.artifact.getKey(), d -> d, BepArtifactData::update));
.collect(
toImmutableMap(d -> d.artifact.getRelativePath(), d -> d, BepArtifactData::update));
}

/** Returns the set of build targets that had an error. */
Expand Down
10 changes: 6 additions & 4 deletions base/src/com/google/idea/blaze/base/filecache/ArtifactsDiff.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.idea.blaze.base.command.buildresult.BlazeArtifact.LocalFileArtifact;
import com.google.idea.blaze.base.command.buildresult.OutputArtifactInfo;
import com.google.idea.blaze.base.command.buildresult.OutputArtifactWithoutDigest;
import com.google.idea.blaze.base.io.FileAttributeScanner;
import com.google.idea.blaze.base.prefetch.FetchExecutor;
Expand Down Expand Up @@ -54,7 +55,7 @@ public static ArtifactsDiff diffArtifacts(
throws InterruptedException, ExecutionException {
return diffArtifacts(
oldState,
newArtifacts.stream().collect(toImmutableMap(OutputArtifactWithoutDigest::getKey, a -> a)));
newArtifacts.stream().collect(toImmutableMap(OutputArtifactInfo::getRelativePath, a -> a)));
}

public static ArtifactsDiff diffArtifacts(
Expand Down Expand Up @@ -90,13 +91,14 @@ private static ImmutableMap<String, ArtifactState> computeState(
return artifacts.stream()
.collect(
toImmutableMap(
OutputArtifactWithoutDigest::getKey,
OutputArtifactInfo::getRelativePath,
OutputArtifactWithoutDigest::toArtifactState));
}
// for local files, diffing requires checking the timestamps, which we multi-thread
return FileAttributeScanner.readAttributes(artifacts, TO_ARTIFACT_STATE, FetchExecutor.EXECUTOR)
.entrySet().stream()
.collect(toImmutableMap(e -> e.getKey().getKey(), Map.Entry::getValue));
.entrySet()
.stream()
.collect(toImmutableMap(e -> e.getKey().getRelativePath(), Map.Entry::getValue));
}

private static FileAttributeScanner.AttributeReader<OutputArtifactWithoutDigest, ArtifactState>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ private Map<String, File> readCachedFiles() {
*/
@VisibleForTesting
static String getCacheKey(RemoteOutputArtifact output) {
String key = output.getKey();
String key = output.getRelativePath();
String fileName = PathUtil.getFileName(key);
List<String> components = Splitter.on('.').limit(2).splitToList(fileName);
StringBuilder builder =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public RemoteOutputArtifacts appendNewOutputs(Set<OutputArtifact> outputs) {
// more recently built artifacts replace existing ones with the same path
outputs.forEach(
a -> {
String key = a.getKey();
String key = a.getRelativePath();
if (!(a instanceof RemoteOutputArtifact)) {
map.remove(key);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,14 @@ public BlazeBuildOutputs updateOutputs(BlazeBuildOutputs nextOutputs) {
continue;
}
// no longer output by this target; need to update target associations
BepArtifactData data = combined.get(old.getKey());
BepArtifactData data = combined.get(old.getRelativePath());
if (data != null) {
data = data.removeTargetAssociation(target);
}
if (data == null) {
combined.remove(old.getKey());
combined.remove(old.getRelativePath());
} else {
combined.put(old.getKey(), data);
combined.put(old.getRelativePath(), data);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ private static TargetMapAndInterfaceState updateTargetMap(
.getBuildResult()
.getOutputGroupArtifacts(group -> group.startsWith(OutputGroup.INFO.prefix))
.stream()
.filter(f -> ideInfoPredicate.test(f.getKey()))
.filter(f -> ideInfoPredicate.test(f.getRelativePath()))
.distinct()
.collect(toImmutableList());

Expand Down Expand Up @@ -388,15 +388,15 @@ private static TargetMapAndInterfaceState updateState(
configurations.add(config);
TargetKey key = targetFilePair.target.getKey();
if (targetMap.putIfAbsent(key, targetFilePair.target) == null) {
state.ideInfoToTargetKey.forcePut(file.getKey(), key);
state.ideInfoToTargetKey.forcePut(file.getRelativePath(), key);
} else {
if (!newTargets.add(key)) {
duplicateTargetLabels++;
}
// prioritize the default configuration over build order
if (Objects.equals(config, configHandler.defaultConfigurationPathComponent)) {
targetMap.put(key, targetFilePair.target);
state.ideInfoToTargetKey.forcePut(file.getKey(), key);
state.ideInfoToTargetKey.forcePut(file.getRelativePath(), key);
}
}
}
Expand Down Expand Up @@ -533,7 +533,7 @@ private static void prefetchGenfiles(
// TODO: handle prefetching for arbitrary OutputArtifacts
ImmutableList<File> files =
artifacts.stream()
.filter(a -> filter.test(a.getKey()))
.filter(a -> filter.test(a.getRelativePath()))
.filter(o -> o instanceof LocalFileArtifact)
.map(o -> ((LocalFileArtifact) o).getFile())
.collect(toImmutableList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,56 +42,56 @@ public void setUp() throws Exception {
@Test
public void testNormalExtension() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt/foo/bar/SourceFile.java");
when(artifact.getRelativePath()).thenReturn("k8-opt/foo/bar/SourceFile.java");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("SourceFile_c5ad9a45.java");
}

@Test
public void testNoExtension() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt/include/vector");
when(artifact.getRelativePath()).thenReturn("k8-opt/include/vector");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("vector_b2da3b80");
}

@Test
public void testDoubleExtension() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt/foo/bar/proto.pb.go");
when(artifact.getRelativePath()).thenReturn("k8-opt/foo/bar/proto.pb.go");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("proto_5d28a0a7.pb.go");
}

@Test
public void testExtensionOnly() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt/foo/bar/.bazelrc");
when(artifact.getRelativePath()).thenReturn("k8-opt/foo/bar/.bazelrc");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("_8f54cd80.bazelrc");
}

@Test
public void testEndingDot() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt/foo/bar/foo.");
when(artifact.getRelativePath()).thenReturn("k8-opt/foo/bar/foo.");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("foo_51eabcd4.");
}

@Test
public void testDoubleDot() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt/foo/bar/foo..bar");
when(artifact.getRelativePath()).thenReturn("k8-opt/foo/bar/foo..bar");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("foo_7025f43e..bar");
}

@Test
public void testDotInDirectory() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt/foo.bar/foo.bar");
when(artifact.getRelativePath()).thenReturn("k8-opt/foo.bar/foo.bar");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("foo_509a9fc2.bar");
}

@Test
public void testWindowsStylePath() {
RemoteOutputArtifact artifact = mock(RemoteOutputArtifact.class);
when(artifact.getKey()).thenReturn("k8-opt\\foo\\bar\\foo.bar");
when(artifact.getRelativePath()).thenReturn("k8-opt\\foo\\bar\\foo.bar");
assertThat(RemoteOutputsCache.getCacheKey(artifact)).isEqualTo("foo_e9b6ced4.bar");
}

Expand All @@ -101,10 +101,10 @@ public void testHashDuplication() {
RemoteOutputArtifact artifact2 = mock(RemoteOutputArtifact.class);
// we notice hashCode collision for the following paths. More details can be
// found in b/249402913#comment16
when(artifact1.getKey())
when(artifact1.getRelativePath())
.thenReturn(
"android-armeabi-v7a-fastbuild-ST-537378632435/bin/java/com/google/android/apps/gmm/navigation/ui/common/_migrated/_min_sdk_bumped/common/AndroidManifest.xml");
when(artifact2.getKey())
when(artifact2.getRelativePath())
.thenReturn(
"android-armeabi-v7a-fastbuild-ST-537378632435/bin/java/com/google/android/apps/gmm/place/timeline/common/_migrated/_min_sdk_bumped/ImpressionLoggingPropertyNode/AndroidManifest.xml");
assertThat(RemoteOutputsCache.getCacheKey(artifact1))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.devtools.intellij.qsync.ArtifactTrackerData.BuildArtifacts;
import com.google.devtools.intellij.qsync.ArtifactTrackerData.TargetArtifacts;
import com.google.idea.blaze.base.command.buildresult.OutputArtifact;
import com.google.idea.blaze.base.command.buildresult.OutputArtifactInfo;
import com.google.idea.blaze.base.filecache.ArtifactState;
import com.google.idea.blaze.base.qsync.ArtifactTracker.UpdateResult;
import com.google.idea.blaze.base.qsync.OutputInfo;
Expand Down Expand Up @@ -293,7 +294,7 @@ public ListenableFuture<?> copy(
ImmutableMap<? extends OutputArtifact, ArtifactDestination> artifactToDest,
Context<?> context) {
artifactToDest.keySet().stream()
.map(OutputArtifact::getKey)
.map(OutputArtifactInfo::getRelativePath)
.forEach(collectedArtifactKeyToMetadata::add);
return Futures.immediateFuture(null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ private static String cacheKeyInternal(BlazeArtifact output) {

private static String artifactKey(BlazeArtifact artifact) {
if (artifact instanceof OutputArtifactWithoutDigest) {
return ((OutputArtifactWithoutDigest) artifact).getKey();
return ((OutputArtifactWithoutDigest) artifact).getRelativePath();
}
if (artifact instanceof SourceArtifact) {
return ((SourceArtifact) artifact).getFile().getPath();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ public static PackageManifestReader getInstance() {
private Map<ArtifactState, TargetKey> fileToLabelMap = new HashMap<>();
private final Map<TargetKey, Map<ArtifactLocation, String>> manifestMap = Maps.newConcurrentMap();

/** @return A map from java source absolute file path to declared package string. */
/**
* @return A map from java source absolute file path to declared package string.
*/
public Map<TargetKey, Map<ArtifactLocation, String>> readPackageManifestFiles(
Project project,
BlazeContext context,
Expand Down Expand Up @@ -155,7 +157,12 @@ public Map<TargetKey, Map<ArtifactLocation, String>> readPackageManifestFiles(
}
this.fileToLabelMap =
fileToLabelMap.entrySet().stream()
.filter(e -> diff.getNewState().containsKey(e.getKey().getKey()))
.filter(
e -> {
OutputArtifactWithoutDigest outputArtifactWithoutDigest = e.getKey();
return diff.getNewState()
.containsKey(outputArtifactWithoutDigest.getRelativePath());
})
.collect(toImmutableMap(e -> e.getKey().toArtifactState(), Map.Entry::getValue));

try {
Expand Down

0 comments on commit c285c84

Please sign in to comment.