Skip to content

Commit

Permalink
Implemenent improved, more accurate calculation of android resource d…
Browse files Browse the repository at this point in the history
…irectories.

- 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
  • Loading branch information
Googler authored and copybara-github committed Dec 6, 2023
1 parent 6b2aa3f commit a65ab2b
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 8 deletions.
3 changes: 2 additions & 1 deletion base/src/com/google/idea/blaze/base/qsync/ProjectLoader.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
3 changes: 3 additions & 0 deletions base/src/com/google/idea/blaze/base/qsync/QuerySync.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -38,18 +39,21 @@ public class BlazeProjectSnapshotBuilder {
private final Path workspaceRoot;
private final ImmutableSet<String> handledRuleKinds;
private final ProjectProtoTransform projectProtoTransform;
private final Supplier<Boolean> useNewResDirLogic;

public BlazeProjectSnapshotBuilder(
ListeningExecutorService executor,
PackageReader workspaceRelativePackageReader,
Path workspaceRoot,
ImmutableSet<String> handledRuleKinds,
ProjectProtoTransform projectProtoTransform) {
ProjectProtoTransform projectProtoTransform,
Supplier<Boolean> useNewResDirLogic) {
this.executor = executor;
this.workspaceRelativePackageReader = workspaceRelativePackageReader;
this.workspaceRoot = workspaceRoot;
this.handledRuleKinds = handledRuleKinds;
this.projectProtoTransform = projectProtoTransform;
this.useNewResDirLogic = useNewResDirLogic;
}

/** {@code Function<ProjectProto.Project, ProjectProto.Project>} that can throw exceptions. */
Expand Down Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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. */
Expand All @@ -81,18 +83,21 @@ public class GraphToProjectConverter {

private final ProjectDefinition projectDefinition;
private final ListeningExecutorService executor;
private final Supplier<Boolean> useNewResDirLogic;

public GraphToProjectConverter(
PackageReader packageReader,
Path workspaceRoot,
Context<?> context,
ProjectDefinition projectDefinition,
ListeningExecutorService executor) {
ListeningExecutorService executor,
Supplier<Boolean> 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
Expand All @@ -107,6 +112,7 @@ public GraphToProjectConverter(
this.context = context;
this.projectDefinition = projectDefinition;
this.executor = executor;
this.useNewResDirLogic = Suppliers.ofInstance(true);
}

/**
Expand Down Expand Up @@ -487,11 +493,29 @@ public ProjectProto.Project createProject(BuildGraphData graph) throws BuildExce
ImmutableMultimap<Path, Path> rootToNonJavaSource =
nonJavaSourceFolders(
graph.getSourceFilesByRuleKindAndType(not(RuleKinds::isJava), SourceType.all()));
ImmutableSet<Path> dirs = computeAndroidResourceDirectories(graph.getAllSourceFiles());
ImmutableSet<Path> 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<Path> 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<String> 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 =
Expand All @@ -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());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,10 @@ public List<Path> getAndroidSourceFiles() {
return getSourceFilesByRuleKindAndType(RuleKinds::isAndroid, SourceType.REGULAR);
}

public List<Path> getAndroidResourceFiles() {
return getSourceFilesByRuleKindAndType(RuleKinds::isAndroid, SourceType.ANDROID_RESOURCES);
}

/** Returns a list of custom_package fields that used by current project. */
public ImmutableSet<String> getAllCustomPackages() {
return targetMap().values().stream()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit a65ab2b

Please sign in to comment.