Skip to content

Commit

Permalink
Set test sources bit correctly for generated source jars.
Browse files Browse the repository at this point in the history
Generated source jars should have their test bit set according to the target that generated them. Failure to do so results in the IDE failing to resolve references from the generated code to regular code in the same target, since the IDE thinks the generated code is not test code (so cannot access test code).

This requires changing the way that we handle generated source jars. Previously, we wrote them all to a cache directory, then used the directory contents to update the project proto. Doing it this way means we loose information about the origin of the jar file, so we cannot map it back to the target that generated it to determine if it's a test target.

Instead, take the set of generated source jars directly from the `ArtifactInfo` protos that we have from previous dependency builds, and then map the build artifact paths to cache paths using the cache layout. This allows us to look up the "is test" but using the corresponding build graph data and the project definition therein.

PiperOrigin-RevId: 586646320
  • Loading branch information
Googler authored and copybara-github committed Dec 1, 2023
1 parent da499b2 commit 4f71f12
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@
import com.google.idea.blaze.common.PrintOutput;
import com.google.idea.blaze.exception.BuildException;
import com.google.idea.blaze.qsync.GeneratedSourceProjectUpdater;
import com.google.idea.blaze.qsync.GeneratedSourceProjectUpdater.GeneratedSourceJar;
import com.google.idea.blaze.qsync.SrcJarProjectUpdater;
import com.google.idea.blaze.qsync.TestSourceGlobMatcher;
import com.google.idea.blaze.qsync.cc.CcDependenciesInfo;
import com.google.idea.blaze.qsync.project.BuildGraphData;
import com.google.idea.blaze.qsync.project.ProjectDefinition;
Expand All @@ -81,7 +83,7 @@
import com.google.idea.common.experiments.BoolExperiment;
import com.google.protobuf.ExtensionRegistry;
import com.intellij.openapi.diagnostic.Logger;
import com.intellij.openapi.util.io.FileUtil;
import com.intellij.openapi.util.io.FileUtilRt;
import com.intellij.openapi.util.text.StringUtil;
import java.io.File;
import java.io.IOException;
Expand Down Expand Up @@ -609,18 +611,18 @@ private void updateMaps(Set<Label> targets, JavaArtifacts newArtifacts) {
ImmutableSet.of("jar", "zip", "srcjar");

private static boolean hasJarOrZipExtension(Path p) {
return JAR_ZIP_EXTENSIONS.contains(FileUtil.getExtension(p.toString()));
return JAR_ZIP_EXTENSIONS.contains(FileUtilRt.getExtension(p.toString()));
}

@Override
public ProjectProto.Project updateProjectProto(
ProjectProto.Project projectProto, BuildGraphData graph, Context<?> context)
throws BuildException {
return updateProjectProtoForJavaDeps(projectProto);
return updateProjectProtoForJavaDeps(projectProto, graph);
}

private ProjectProto.Project updateProjectProtoForJavaDeps(ProjectProto.Project projectProto)
throws BuildException {
private ProjectProto.Project updateProjectProtoForJavaDeps(
ProjectProto.Project projectProto, BuildGraphData graph) throws BuildException {

Set<ProjectPath> generatedJavaSrcRoots = Sets.newHashSet();

Expand All @@ -641,26 +643,37 @@ private ProjectProto.Project updateProjectProtoForJavaDeps(ProjectProto.Project
}
}

ImmutableSet<ProjectPath> generatedProjectSrcJars;
Path srcJarDir =
generatedSrcFileCacheDirectory.resolve(JavaSourcesArchiveCacheLayout.ROOT_DIRECTORY_NAME);
try (Stream<Path> pathStream =
srcJarDir.toFile().exists() ? Files.walk(srcJarDir) : Stream.empty()) {
generatedProjectSrcJars =
pathStream
.filter(Files::isRegularFile)
.map(ideProjectBasePath::relativize)
.map(ProjectPath::projectRelative)
.collect(toImmutableSet());
} catch (IOException e) {
throw new BuildException(e);
TestSourceGlobMatcher testSourceMatcher = TestSourceGlobMatcher.create(projectDefinition);
ImmutableSet.Builder<GeneratedSourceJar> generatedProjectSrcJars = ImmutableSet.builder();
for (ArtifactInfo ai : javaArtifacts.values()) {
if (!projectDefinition.isIncluded(ai.label())) {
continue;
}
for (Path blazeOutRelativePath : ai.genSrcs()) {
// TODO(mathewi) depending on `JAVA_ARCHIVE_EXTENSIONS` here exposes a design problem, we
// shouldn't have to depend on such implementation details. Figure out a better design
// for the dance between this class and the cache.
if (!JavaSourcesArchiveCacheLayout.JAVA_ARCHIVE_EXTENSIONS.contains(
FileUtilRt.getExtension(blazeOutRelativePath.toString()))) {
continue;
}
Optional<Path> artifactPath = generatedSrcFileCache.getCacheFile(blazeOutRelativePath);
if (artifactPath.isEmpty()) {
logger.warn("No cached artifact found for source jar " + blazeOutRelativePath);
continue;
}
generatedProjectSrcJars.add(
GeneratedSourceJar.create(
ProjectPath.projectRelative(artifactPath.get()),
testSourceMatcher.matches(ai.label().getPackage())));
}
}

GeneratedSourceProjectUpdater updater =
new GeneratedSourceProjectUpdater(
projectProto,
ImmutableSet.copyOf(generatedJavaSrcRoots),
generatedProjectSrcJars,
generatedProjectSrcJars.build(),
projectPathResolver);

projectProto = updater.addGenSrcContentEntry();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
/** Places java source archives in a dedicated subdirectory. */
public class JavaSourcesArchiveCacheLayout implements CacheLayout {

private static final ImmutableSet<String> JAVA_ARCHIVE_EXTENSIONS =
public static final ImmutableSet<String> JAVA_ARCHIVE_EXTENSIONS =
ImmutableSet.of("jar", "srcjar");

/** Cache subdirectory in which all source jars are placed. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import static com.google.idea.blaze.qsync.SrcJarInnerPathFinder.AllowPackagePrefixes.ALLOW_NON_EMPTY_PACKAGE_PREFIXES;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import com.google.idea.blaze.qsync.SrcJarInnerPathFinder.JarPath;
import com.google.idea.blaze.qsync.project.ProjectPath;
Expand All @@ -26,17 +27,32 @@
/** Updates project protos with a content entry for generated sources */
public class GeneratedSourceProjectUpdater {

/**
* Information about a generated source jar, comprising its path and whether or not it's test only
* source.
*/
@AutoValue
public abstract static class GeneratedSourceJar {
public abstract ProjectPath path();

public abstract boolean isTestSource();

public static GeneratedSourceJar create(ProjectPath path, boolean isTestSource) {
return new AutoValue_GeneratedSourceProjectUpdater_GeneratedSourceJar(path, isTestSource);
}
}

private final Project project;
private final ImmutableSet<ProjectPath> genSrcRoots;
private final ImmutableSet<ProjectPath> genSrcJars;
private final ImmutableSet<GeneratedSourceJar> genSrcJars;
private final ProjectPath.Resolver resolver;

private final SrcJarInnerPathFinder srcJarInnerPathFinder;

public GeneratedSourceProjectUpdater(
Project project,
ImmutableSet<ProjectPath> genSrcFileFolders,
ImmutableSet<ProjectPath> genSrcJars,
ImmutableSet<GeneratedSourceJar> genSrcJars,
ProjectPath.Resolver resolver) {
this.project = project;
this.genSrcRoots = genSrcFileFolders;
Expand Down Expand Up @@ -70,15 +86,16 @@ public Project addGenSrcContentEntry() {
workspaceModule.addContentEntries(genSourcesContentEntry);
}

for (ProjectPath path : genSrcJars) {
for (GeneratedSourceJar jar : genSrcJars) {
ProjectProto.ContentEntry.Builder genSrcJarContentEntry =
ProjectProto.ContentEntry.newBuilder().setRoot(path.toProto());
ProjectProto.ContentEntry.newBuilder().setRoot(jar.path().toProto());
for (JarPath innerPath :
srcJarInnerPathFinder.findInnerJarPaths(resolver.resolve(path).toFile())) {
srcJarInnerPathFinder.findInnerJarPaths(resolver.resolve(jar.path()).toFile())) {
genSrcJarContentEntry.addSources(
ProjectProto.SourceFolder.newBuilder()
.setProjectPath(path.withInnerJarPath(innerPath.path).toProto())
.setProjectPath(jar.path().withInnerJarPath(innerPath.path).toProto())
.setIsGenerated(true)
.setIsTest(jar.isTestSource())
.setPackagePrefix(innerPath.packagePrefix));
}
workspaceModule.addContentEntries(genSrcJarContentEntry);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,7 @@ public ProjectProto.Project createProject(BuildGraphData graph) throws BuildExce

ListMultimap<Path, Path> excludesByRootDirectory =
projectDefinition.getExcludesByRootDirectory();
TestSourceGlobMatcher testSourceGlobMatcher =
new TestSourceGlobMatcher(projectDefinition.testSources());
TestSourceGlobMatcher testSourceGlobMatcher = TestSourceGlobMatcher.create(projectDefinition);
for (Path dir : projectDefinition.projectIncludes()) {
ProjectProto.ContentEntry.Builder contentEntry =
ProjectProto.ContentEntry.newBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

import static com.google.common.collect.ImmutableSet.toImmutableSet;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;
import com.google.idea.blaze.qsync.project.ProjectDefinition;
import java.io.File;
import java.nio.file.Path;

Expand All @@ -29,6 +31,11 @@ public class TestSourceGlobMatcher {

private final ImmutableSet<String> testSourceGlobs;

public static TestSourceGlobMatcher create(ProjectDefinition projectDefinition) {
return new TestSourceGlobMatcher(projectDefinition.testSources());
}

@VisibleForTesting
public TestSourceGlobMatcher(ImmutableSet<String> testSources) {
this.testSourceGlobs =
testSources.stream().map(TestSourceGlobMatcher::modifyPattern).collect(toImmutableSet());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.idea.blaze.qsync.QuerySyncTestUtils.createSrcJar;

import com.google.common.collect.ImmutableSet;
import com.google.idea.blaze.qsync.GeneratedSourceProjectUpdater.GeneratedSourceJar;
import com.google.idea.blaze.qsync.QuerySyncTestUtils.PathPackage;
import com.google.idea.blaze.qsync.project.ProjectPath;
import com.google.idea.blaze.qsync.project.ProjectPath.Resolver;
Expand Down Expand Up @@ -105,7 +106,7 @@ public void addGenSrcContentEntry_withGenSrcJar_addsContentEntry() throws Except
new GeneratedSourceProjectUpdater(
project,
ImmutableSet.of(),
ImmutableSet.of(srcJarPath),
ImmutableSet.of(GeneratedSourceJar.create(srcJarPath, false)),
Resolver.create(workspaceRoot, projectRoot));

ProjectProto.Project newProject = updater.addGenSrcContentEntry();
Expand All @@ -120,6 +121,29 @@ public void addGenSrcContentEntry_withGenSrcJar_addsContentEntry() throws Except
ProjectProto.SourceFolder sourceFolder = contentEntry.getSources(0);
assertThat(sourceFolder.getProjectPath()).isEqualTo(srcJarPath.toProto());
assertThat(sourceFolder.getIsGenerated()).isTrue();
assertThat(sourceFolder.getIsTest()).isFalse();
}

@Test
public void addGenSrcContentEntry_withGenSrcJar_addsContentEntry_testSources() throws Exception {
ProjectProto.Project project =
ProjectProtos.forTestProject(TestData.JAVA_LIBRARY_NO_DEPS_QUERY);

ProjectPath srcJarPath = ProjectPath.projectRelative("generated/srcjars/sources.srcjar");
createSrcJar(
projectRoot.resolve(srcJarPath.relativePath()),
PathPackage.of("com/package/Class.java", "com.package"));

GeneratedSourceProjectUpdater updater =
new GeneratedSourceProjectUpdater(
project,
ImmutableSet.of(),
ImmutableSet.of(GeneratedSourceJar.create(srcJarPath, true)),
Resolver.create(workspaceRoot, projectRoot));

ProjectProto.Project newProject = updater.addGenSrcContentEntry();

assertThat(newProject.getModules(0).getContentEntries(1).getSources(0).getIsTest()).isTrue();
}

@Test
Expand All @@ -137,7 +161,7 @@ public void addGenSrcContentEntry_withGenSrcJarWithNestedRoot_addsContentEntryWi
new GeneratedSourceProjectUpdater(
project,
ImmutableSet.of(),
ImmutableSet.of(srcJarPath),
ImmutableSet.of(GeneratedSourceJar.create(srcJarPath, false)),
Resolver.create(workspaceRoot, projectRoot));

ProjectProto.Project newProject = updater.addGenSrcContentEntry();
Expand Down

0 comments on commit 4f71f12

Please sign in to comment.