From a65ab2bc66edf2e78d3059e2dd39a78c4aa5064b Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 6 Dec 2023 06:29:51 -0800 Subject: [PATCH] Implemenent improved, more accurate calculation of android resource directories. - Rather than considering all sources that have the `.xml` extension, only use those referred to by the `resource_files` attribute of an android rule. - Rather than hardcoding `res` as the expected directory name, simply take the grandparent of android resource files. This should be safe since the structure of android resoures is pretty rigid ,and the docs explicitly state that no resource files may exist directly in the `res` directory itself. Since this change is not risk free, also add an experiment flag to turn it off should be start causing problems. PiperOrigin-RevId: 588404986 --- .../idea/blaze/base/qsync/ProjectLoader.java | 3 +- .../idea/blaze/base/qsync/QuerySync.java | 3 ++ .../qsync/BlazeProjectSnapshotBuilder.java | 9 ++++-- .../blaze/qsync/GraphToProjectConverter.java | 32 ++++++++++++++++--- .../blaze/qsync/project/BuildGraphData.java | 4 +++ .../blaze/qsync/util/ProjectSpecBuilder.java | 8 ++++- 6 files changed, 51 insertions(+), 8 deletions(-) diff --git a/base/src/com/google/idea/blaze/base/qsync/ProjectLoader.java b/base/src/com/google/idea/blaze/base/qsync/ProjectLoader.java index f4dcf4ad984..210ae79c533 100644 --- a/base/src/com/google/idea/blaze/base/qsync/ProjectLoader.java +++ b/base/src/com/google/idea/blaze/base/qsync/ProjectLoader.java @@ -156,7 +156,8 @@ public QuerySyncProject loadProject(BlazeContext context) throws BuildException workspaceRoot.path(), handledRules, ProjectProtoTransform.compose( - artifactTracker::updateProjectProto, new CcProjectProtoTransform(artifactTracker))); + artifactTracker::updateProjectProto, new CcProjectProtoTransform(artifactTracker)), + QuerySync.USE_NEW_RES_DIR_LOGIC::getValue); QueryRunner queryRunner = createQueryRunner(buildSystem); ProjectQuerier projectQuerier = createProjectQuerier(projectRefresher, queryRunner, vcsHandler); QuerySyncSourceToTargetMap sourceToTargetMap = diff --git a/base/src/com/google/idea/blaze/base/qsync/QuerySync.java b/base/src/com/google/idea/blaze/base/qsync/QuerySync.java index 93a4ee99a60..e2fca5d6e1c 100644 --- a/base/src/com/google/idea/blaze/base/qsync/QuerySync.java +++ b/base/src/com/google/idea/blaze/base/qsync/QuerySync.java @@ -29,6 +29,9 @@ public class QuerySync { private static final FeatureRolloutExperiment ENABLED = new FeatureRolloutExperiment("query.sync"); + public static final BoolExperiment USE_NEW_RES_DIR_LOGIC = + new BoolExperiment("query.sync.new.resdir.logic", true); + /** * Previously, query sync was enabled by an experiment. Some users still have that experiment set * and we don't want to inadvertently disable query sync for them. diff --git a/querysync/java/com/google/idea/blaze/qsync/BlazeProjectSnapshotBuilder.java b/querysync/java/com/google/idea/blaze/qsync/BlazeProjectSnapshotBuilder.java index 50a8462128c..57985c9c8a1 100644 --- a/querysync/java/com/google/idea/blaze/qsync/BlazeProjectSnapshotBuilder.java +++ b/querysync/java/com/google/idea/blaze/qsync/BlazeProjectSnapshotBuilder.java @@ -26,6 +26,7 @@ import com.google.idea.blaze.qsync.project.ProjectProto.Project; import com.google.idea.blaze.qsync.query.QuerySummary; import java.nio.file.Path; +import java.util.function.Supplier; /** * Project refresher creates an appropriate {@link RefreshOperation} based on the project and @@ -38,18 +39,21 @@ public class BlazeProjectSnapshotBuilder { private final Path workspaceRoot; private final ImmutableSet handledRuleKinds; private final ProjectProtoTransform projectProtoTransform; + private final Supplier useNewResDirLogic; public BlazeProjectSnapshotBuilder( ListeningExecutorService executor, PackageReader workspaceRelativePackageReader, Path workspaceRoot, ImmutableSet handledRuleKinds, - ProjectProtoTransform projectProtoTransform) { + ProjectProtoTransform projectProtoTransform, + Supplier useNewResDirLogic) { this.executor = executor; this.workspaceRelativePackageReader = workspaceRelativePackageReader; this.workspaceRoot = workspaceRoot; this.handledRuleKinds = handledRuleKinds; this.projectProtoTransform = projectProtoTransform; + this.useNewResDirLogic = useNewResDirLogic; } /** {@code Function} that can throw exceptions. */ @@ -85,7 +89,8 @@ public BlazeProjectSnapshot createBlazeProjectSnapshot( effectiveWorkspaceRoot, context, postQuerySyncData.projectDefinition(), - executor); + executor, + useNewResDirLogic); QuerySummary querySummary = postQuerySyncData.querySummary(); BuildGraphData graph = new BlazeQueryParser(querySummary, context, handledRuleKinds).parse(); Project project = diff --git a/querysync/java/com/google/idea/blaze/qsync/GraphToProjectConverter.java b/querysync/java/com/google/idea/blaze/qsync/GraphToProjectConverter.java index 07fb230bf51..5427d46f6bd 100644 --- a/querysync/java/com/google/idea/blaze/qsync/GraphToProjectConverter.java +++ b/querysync/java/com/google/idea/blaze/qsync/GraphToProjectConverter.java @@ -23,6 +23,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; @@ -70,6 +71,7 @@ import java.util.TreeSet; import java.util.concurrent.ExecutionException; import java.util.function.Predicate; +import java.util.function.Supplier; import javax.annotation.Nullable; /** Converts a {@link BuildGraphData} instance into a project proto. */ @@ -81,18 +83,21 @@ public class GraphToProjectConverter { private final ProjectDefinition projectDefinition; private final ListeningExecutorService executor; + private final Supplier useNewResDirLogic; public GraphToProjectConverter( PackageReader packageReader, Path workspaceRoot, Context context, ProjectDefinition projectDefinition, - ListeningExecutorService executor) { + ListeningExecutorService executor, + Supplier useNewResDirLogic) { this.packageReader = packageReader; this.fileExistenceCheck = p -> Files.isRegularFile(workspaceRoot.resolve(p)); this.context = context; this.projectDefinition = projectDefinition; this.executor = executor; + this.useNewResDirLogic = useNewResDirLogic; } @VisibleForTesting @@ -107,6 +112,7 @@ public GraphToProjectConverter( this.context = context; this.projectDefinition = projectDefinition; this.executor = executor; + this.useNewResDirLogic = Suppliers.ofInstance(true); } /** @@ -487,11 +493,29 @@ public ProjectProto.Project createProject(BuildGraphData graph) throws BuildExce ImmutableMultimap rootToNonJavaSource = nonJavaSourceFolders( graph.getSourceFilesByRuleKindAndType(not(RuleKinds::isJava), SourceType.all())); - ImmutableSet dirs = computeAndroidResourceDirectories(graph.getAllSourceFiles()); + ImmutableSet androidResDirs; + if (useNewResDirLogic.get()) { + // Note: according to: + // https://developer.android.com/guide/topics/resources/providing-resources + // "Never save resource files directly inside the res/ directory. It causes a compiler error." + // This implies that we can safely take the grandparent of each resource file to find the + // top level res dir: + List resList = graph.getAndroidResourceFiles(); + androidResDirs = + resList.stream() + .map(Path::getParent) + .distinct() + .map(Path::getParent) + .distinct() + .collect(toImmutableSet()); + } else { + // TODO(mathewi) Remove this and the corresponding experiment once the locig has been proven. + androidResDirs = computeAndroidResourceDirectories(graph.getAllSourceFiles()); + } ImmutableSet pkgs = computeAndroidSourcePackages(graph.getAndroidSourceFiles(), javaSourceRoots); - context.output(PrintOutput.log("%-10d Android resource directories", dirs.size())); + context.output(PrintOutput.log("%-10d Android resource directories", androidResDirs.size())); context.output(PrintOutput.log("%-10d Android resource packages", pkgs.size())); ProjectProto.Library depsLib = @@ -513,7 +537,7 @@ public ProjectProto.Project createProject(BuildGraphData graph) throws BuildExce .setType(ProjectProto.ModuleType.MODULE_TYPE_DEFAULT) .addLibraryName(depsLib.getName()) .addAllAndroidResourceDirectories( - dirs.stream().map(Path::toString).collect(toImmutableList())) + androidResDirs.stream().map(Path::toString).collect(toImmutableList())) .addAllAndroidSourcePackages(pkgs) .addAllAndroidCustomPackages(graph.getAllCustomPackages()); diff --git a/querysync/java/com/google/idea/blaze/qsync/project/BuildGraphData.java b/querysync/java/com/google/idea/blaze/qsync/project/BuildGraphData.java index ec18789da89..7131969313f 100644 --- a/querysync/java/com/google/idea/blaze/qsync/project/BuildGraphData.java +++ b/querysync/java/com/google/idea/blaze/qsync/project/BuildGraphData.java @@ -357,6 +357,10 @@ public List getAndroidSourceFiles() { return getSourceFilesByRuleKindAndType(RuleKinds::isAndroid, SourceType.REGULAR); } + public List getAndroidResourceFiles() { + return getSourceFilesByRuleKindAndType(RuleKinds::isAndroid, SourceType.ANDROID_RESOURCES); + } + /** Returns a list of custom_package fields that used by current project. */ public ImmutableSet getAllCustomPackages() { return targetMap().values().stream() diff --git a/querysync/java/com/google/idea/blaze/qsync/util/ProjectSpecBuilder.java b/querysync/java/com/google/idea/blaze/qsync/util/ProjectSpecBuilder.java index df854e414a3..c5b23206958 100644 --- a/querysync/java/com/google/idea/blaze/qsync/util/ProjectSpecBuilder.java +++ b/querysync/java/com/google/idea/blaze/qsync/util/ProjectSpecBuilder.java @@ -15,6 +15,7 @@ */ package com.google.idea.blaze.qsync.util; +import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; @@ -89,7 +90,12 @@ private int run() throws IOException, BuildException { new BlazeQueryParser(snapshot.querySummary(), context, ImmutableSet.of()).parse(); GraphToProjectConverter converter = new GraphToProjectConverter( - packageReader, workspaceRoot, context, snapshot.projectDefinition(), executor); + packageReader, + workspaceRoot, + context, + snapshot.projectDefinition(), + executor, + Suppliers.ofInstance(true)); System.out.println(TextFormat.printer().printToString(converter.createProject(buildGraph))); return context.hasError() ? 1 : 0; }