Skip to content

Commit

Permalink
Fix fetch target (fixes bazelbuild#13847)
Browse files Browse the repository at this point in the history 
Context:
- Traditional query relies on the initial loading phase of the build, this lacks the context of build configurations (flags, select() logic), leading to potentially inaccurate or over-inclusive dependency listings.

- cquery executes after the analysis phase, where Bazel has resolved configurations and determined how options influence target definitions. This allows cquery to provide the dependencies truly needed for a build under the current settings.

Considering these differences, I'm updating fetch target logic to rely on cquery instead. This ensures that all necessary repositories are fetched for an offline build while avoiding potential over-fetching

PiperOrigin-RevId: 611455579
Change-Id: I2a954476c06182fd9eb78ad86def7bd72f04074a
  • Loading branch information
@SalmaSamy @meteorcloudy
SalmaSamy authored and meteorcloudy committed Mar 5, 2024
1 parent a305a0d commit 5a27b74
Show file tree
Hide file tree
Showing 8 changed files with 291 additions and 132 deletions.
5 changes: 2 additions & 3 deletions src/main/java/com/google/devtools/build/lib/bazel/commands/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,10 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages:label_printer",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/query2/common:abstract-blaze-query-env",
"//src/main/java/com/google/devtools/build/lib/query2/common:universe-scope",
"//src/main/java/com/google/devtools/build/lib/query2",
"//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
"//src/main/java/com/google/devtools/build/lib/query2/engine",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
Expand Down
20 changes: 15 additions & 5 deletions src/main/java/com/google/devtools/build/lib/bazel/commands/FetchCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.pkgcache.PackageOptions;
import com.google.devtools.build.lib.query2.cquery.CqueryOptions;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.lib.runtime.BlazeCommand;
Expand All @@ -38,30 +39,35 @@
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.KeepGoingOption;
import com.google.devtools.build.lib.runtime.LoadingPhaseThreadsOption;
import com.google.devtools.build.lib.runtime.commands.TestCommand;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.FetchCommand.Code;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingResult;
import java.util.List;
import javax.annotation.Nullable;

/** Fetches external repositories. Which is so fetch. */
@Command(
name = FetchCommand.NAME,
builds = true,
inherits = {TestCommand.class},
options = {
FetchOptions.class,
CqueryOptions.class,
PackageOptions.class,
KeepGoingOption.class,
LoadingPhaseThreadsOption.class
},
usesConfigurationOptions = true,
help = "resource:fetch.txt",
shortDescription = "Fetches external repositories that are prerequisites to the targets.",
allowResidue = true,
Expand All @@ -70,6 +76,14 @@ public final class FetchCommand implements BlazeCommand {

public static final String NAME = "fetch";

@Override
public void editOptions(OptionsParser optionsParser) {
// We only need to inject these options with fetch target (when there is a residue)
if (!optionsParser.getResidue().isEmpty()) {
TargetFetcher.injectOptionsToFetchTarget(optionsParser);
}
}

@Override
public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult options) {
BlazeCommandResult invalidResult = validateOptions(env, options);
Expand Down Expand Up @@ -100,17 +114,13 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
BlazeCommandResult result;
LoadingPhaseThreadsOption threadsOption = options.getOptions(LoadingPhaseThreadsOption.class);
try {
env.syncPackageLoading(options);
if (!options.getResidue().isEmpty()) {
result = fetchTarget(env, options, options.getResidue());
} else if (!fetchOptions.repos.isEmpty()) {
result = fetchRepos(env, threadsOption, fetchOptions.repos);
} else { // --all, --configure, or just 'fetch'
result = fetchAll(env, threadsOption, fetchOptions.configure);
}
} catch (AbruptExitException e) {
return createFailedBlazeCommandResult(
env.getReporter(), e.getMessage(), e.getDetailedExitCode());
} catch (InterruptedException e) {
return createFailedBlazeCommandResult(
env.getReporter(), "Fetch interrupted: " + e.getMessage());
Expand Down
151 changes: 99 additions & 52 deletions src/main/java/com/google/devtools/build/lib/bazel/commands/TargetFetcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,30 +14,39 @@

package com.google.devtools.build.lib.bazel.commands;

import static com.google.common.collect.ImmutableMap.toImmutableMap;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.buildtool.BuildResult;
import com.google.devtools.build.lib.buildtool.BuildTool;
import com.google.devtools.build.lib.buildtool.CqueryProcessor;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.cmdline.TargetPattern.Parser;
import com.google.devtools.build.lib.packages.LabelPrinter;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.query2.common.AbstractBlazeQueryEnvironment;
import com.google.devtools.build.lib.query2.common.UniverseScope;
import com.google.devtools.build.lib.query2.engine.QueryEnvironment.Setting;
import com.google.devtools.build.lib.query2.engine.QueryEvalResult;
import com.google.devtools.build.lib.query2.engine.QueryException;
import com.google.devtools.build.lib.query2.NamedThreadSafeOutputFormatterCallback;
import com.google.devtools.build.lib.query2.common.CqueryNode;
import com.google.devtools.build.lib.query2.cquery.ConfiguredTargetQueryEnvironment;
import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction;
import com.google.devtools.build.lib.query2.engine.QueryExpression;
import com.google.devtools.build.lib.query2.engine.QueryParser;
import com.google.devtools.build.lib.query2.engine.QuerySyntaxException;
import com.google.devtools.build.lib.query2.engine.ThreadSafeOutputFormatterCallback;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.KeepGoingOption;
import com.google.devtools.build.lib.runtime.LoadingPhaseThreadsOption;
import com.google.devtools.build.lib.runtime.commands.QueryCommand;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
import com.google.devtools.common.options.OptionPriority.PriorityCategory;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.OptionsParsingResult;
import java.io.IOException;
import java.util.EnumSet;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Stream;

/** Fetches all repos needed for building a given set of targets. */
public class TargetFetcher {
Expand All @@ -47,7 +56,7 @@ private TargetFetcher(CommandEnvironment env) {
this.env = env;
}

/** Uses `deps` query to find and fetch all repos needed for these targets */
/** Uses cquery to find and fetch all repos needed to build these targets */
public static void fetchTargets(
CommandEnvironment env, OptionsParsingResult options, List<String> targets)
throws RepositoryMappingResolutionException, InterruptedException, TargetFetcherException {
Expand All @@ -56,28 +65,17 @@ public static void fetchTargets(

private void fetchTargets(OptionsParsingResult options, List<String> targets)
throws InterruptedException, TargetFetcherException, RepositoryMappingResolutionException {
AbstractBlazeQueryEnvironment<Target> queryEnv = getQueryEnv(options);
QueryExpression expr = createQueryExpression(targets, queryEnv);
QueryEvalResult queryEvalResult;
try {
queryEvalResult =
queryEnv.evaluateQuery(
expr,
new ThreadSafeOutputFormatterCallback<>() {
@Override
public void processOutput(Iterable<Target> partialResult) {}
});
} catch (IOException e) {
// Should be impossible since our OutputFormatterCallback doesn't throw IOException.
throw new IllegalStateException(e);
} catch (QueryException e) {
throw new TargetFetcherException(
String.format(
"Fetching target dependencies for %s encountered an error: %s",
expr, e.getMessage()));
}
QueryExpression expr = createQueryExpression(targets);
BuildRequest request = createBuildRequest(env, options, targets);
TargetPattern.Parser mainRepoTargetParser = getMainRepoMappingParser(env);

if (!queryEvalResult.getSuccess()) {
BuildResult result =
new BuildTool(
env,
new CqueryProcessor(
expr, mainRepoTargetParser, Optional.of(createNoOutputFormatter())))
.processRequest(request, /* validator= */ null);
if (!result.getSuccess()) {
throw new TargetFetcherException(
String.format(
"Fetching some target dependencies for %s failed, but --keep_going specified. "
Expand All @@ -86,33 +84,59 @@ public void processOutput(Iterable<Target> partialResult) {}
}
}

AbstractBlazeQueryEnvironment<Target> getQueryEnv(OptionsParsingResult options)
/** Creates special output formatter for fetch that doesn't print anything */
private NamedThreadSafeOutputFormatterCallback<CqueryNode> createNoOutputFormatter() {
return new NamedThreadSafeOutputFormatterCallback<CqueryNode>() {
@Override
public String getName() {
return "no_output";
}

@Override
public void processOutput(Iterable<CqueryNode> partialResult) {
// Just do nothing!
// This will be later used to collect repos for vendoring
}
};
}

private BuildRequest createBuildRequest(
CommandEnvironment env, OptionsParsingResult options, List<String> targets) {
return BuildRequest.builder()
.setCommandName(env.getCommandName())
.setId(env.getCommandId())
.setOptions(options)
.setStartupOptions(env.getRuntime().getStartupOptionsProvider())
.setOutErr(env.getReporter().getOutErr())
.setTargets(targets)
.setStartTimeMillis(env.getCommandStartTime())
.setCheckforActionConflicts(false)
.setReportIncompatibleTargets(false)
.build();
}

private Parser getMainRepoMappingParser(CommandEnvironment env)
throws RepositoryMappingResolutionException, InterruptedException {
boolean keepGoing = options.getOptions(KeepGoingOption.class).keepGoing;
LoadingPhaseThreadsOption threadsOption = options.getOptions(LoadingPhaseThreadsOption.class);
RepositoryMapping repoMapping =
env.getSkyframeExecutor()
.getMainRepoMapping(keepGoing, threadsOption.threads, env.getReporter());
TargetPattern.Parser targetParser =
new Parser(env.getRelativeWorkingDirectory(), RepositoryName.MAIN, repoMapping);
return QueryCommand.newQueryEnvironment(
env,
keepGoing,
false,
UniverseScope.EMPTY,
threadsOption.threads,
EnumSet.noneOf(Setting.class),
/* useGraphlessQuery= */ true,
targetParser,
LabelPrinter.legacy());
.getMainRepoMapping(
env.getOptions().getOptions(KeepGoingOption.class).keepGoing,
env.getOptions().getOptions(LoadingPhaseThreadsOption.class).threads,
env.getReporter());
return new Parser(env.getRelativeWorkingDirectory(), RepositoryName.MAIN, repoMapping);
}

private QueryExpression createQueryExpression(
List<String> targets, AbstractBlazeQueryEnvironment<Target> queryEnv)
private QueryExpression createQueryExpression(List<String> targets)
throws TargetFetcherException {
String query = "deps(" + Joiner.on(" union ").join(targets) + ")";

ImmutableMap<String, QueryFunction> functions =
Stream.of(ConfiguredTargetQueryEnvironment.FUNCTIONS, env.getRuntime().getQueryFunctions())
.flatMap(Collection::stream)
.collect(toImmutableMap(QueryFunction::getName, Function.identity()));

try {
return QueryExpression.parse(query, queryEnv);
return QueryParser.parse(query, functions);
} catch (QuerySyntaxException e) {
throw new TargetFetcherException(
String.format(
Expand All @@ -121,6 +145,29 @@ private QueryExpression createQueryExpression(
}
}

static void injectOptionsToFetchTarget(OptionsParser optionsParser) {
try {
optionsParser.parse(
PriorityCategory.COMPUTED_DEFAULT,
"Options required to fetch target",
ImmutableList.of("--nobuild"));
optionsParser.parse(
PriorityCategory.COMPUTED_DEFAULT,
"Fetch target should include 'tags = [\"manual\"]' targets by default",
ImmutableList.of("--build_manual_tests"));
optionsParser.parse(
PriorityCategory.SOFTWARE_REQUIREMENT,
"Fetch target should not exclude test_suite rules",
ImmutableList.of("--noexpand_test_suites"));
optionsParser.parse(
PriorityCategory.SOFTWARE_REQUIREMENT,
"Fetch target should not exclude tests",
ImmutableList.of("--nobuild_tests_only"));
} catch (OptionsParsingException e) {
throw new IllegalStateException("Fetch target needed options failed to parse", e);
}
}

static class TargetFetcherException extends Exception {
public TargetFetcherException(String message) {
super(message);
Expand Down
20 changes: 19 additions & 1 deletion src/main/java/com/google/devtools/build/lib/buildtool/CqueryProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.query2.NamedThreadSafeOutputFormatterCallback;
import com.google.devtools.build.lib.query2.PostAnalysisQueryEnvironment.TopLevelConfigurations;
import com.google.devtools.build.lib.query2.common.CqueryNode;
import com.google.devtools.build.lib.query2.cquery.ConfiguredTargetQueryEnvironment;
Expand All @@ -26,14 +27,30 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.WalkableGraph;
import java.util.Collection;
import java.util.Optional;
import net.starlark.java.eval.StarlarkSemantics;

/** Performs {@code cquery} processing. */
public final class CqueryProcessor extends PostAnalysisQueryProcessor<CqueryNode> {

/**
* Only passed when this is a call from a non query command like Fetch or Vendor, where we don't
* need the output printed
*/
private Optional<NamedThreadSafeOutputFormatterCallback<CqueryNode>> noOutputFormatter;

public CqueryProcessor(
QueryExpression queryExpression, TargetPattern.Parser mainRepoTargetParser) {
super(queryExpression, mainRepoTargetParser);
this.noOutputFormatter = Optional.empty();
}

public CqueryProcessor(
QueryExpression queryExpression,
TargetPattern.Parser mainRepoTargetParser,
Optional<NamedThreadSafeOutputFormatterCallback<CqueryNode>> noOutputFormatter) {
this(queryExpression, mainRepoTargetParser);
this.noOutputFormatter = noOutputFormatter;
}

@Override
Expand Down Expand Up @@ -66,6 +83,7 @@ protected ConfiguredTargetQueryEnvironment getQueryEnvironment(
request.getTopLevelArtifactContext(),
request
.getOptions(CqueryOptions.class)
.getLabelPrinter(starlarkSemantics, mainRepoTargetParser.getRepoMapping()));
.getLabelPrinter(starlarkSemantics, mainRepoTargetParser.getRepoMapping()),
noOutputFormatter);
}
}
Loading

0 comments on commit 5a27b74

Please sign in to comment.