Skip to content

Commit 943e8cf

Browse files
Wyveraldcopybara-github
Wyverald
authored and
copybara-github
committed
Parse command-line options twice; the second time with the main repo mapping
This CL adds an additional parsing round for all command-line options. The second round passes the main repo mapping as a conversion context, so any label-typed options can correctly go through repo mapping. Similarly, when options are set in a Starlark transition, they'd also go through repo mapping. (except for Starlark-defined options, which will be fixed in a follow-up CL) #14852 PiperOrigin-RevId: 460221378 Change-Id: I3aab3ee02cd5097743172edbe95d33d5e140300a
1 parent 35c02b7 commit 943e8cf

11 files changed

Lines changed: 158 additions & 64 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1833,6 +1833,7 @@ java_library(
18331833
name = "config/run_under_converter",
18341834
srcs = ["config/RunUnderConverter.java"],
18351835
deps = [
1836+
":config/core_option_converters",
18361837
":config/run_under",
18371838
"//src/main/java/com/google/devtools/build/lib/cmdline",
18381839
"//src/main/java/com/google/devtools/build/lib/shell",

‎src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,11 @@ public Label getLabel() {
9090
return target.getLabel();
9191
}
9292

93+
public Label.PackageContext getPackageContext() {
94+
return Label.PackageContext.of(
95+
getLabel().getPackageIdentifier(), target.getPackage().getRepositoryMapping());
96+
}
97+
9398
/**
9499
* Returns the configuration for this target. This may return null if the target is supposed to be
95100
* configuration-independent (like an input file, or a visibility rule). However, this is

‎src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,14 @@
2222
import static com.google.devtools.build.lib.packages.Type.STRING;
2323
import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
2424

25+
import com.google.common.base.Preconditions;
2526
import com.google.common.base.Splitter;
2627
import com.google.common.collect.ImmutableList;
2728
import com.google.common.collect.ImmutableMap;
2829
import com.google.devtools.build.lib.cmdline.Label;
2930
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
31+
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
32+
import com.google.devtools.build.lib.cmdline.RepositoryName;
3033
import com.google.devtools.build.lib.packages.Type;
3134
import com.google.devtools.common.options.Converter;
3235
import com.google.devtools.common.options.Converters.BooleanConverter;
@@ -207,13 +210,30 @@ public StrictDepsConverter() {
207210
private static Label convertOptionsLabel(String input, @Nullable Object conversionContext)
208211
throws OptionsParsingException {
209212
try {
213+
if (conversionContext instanceof Label.PackageContext) {
214+
// This can happen if this converter is being used to convert flag values specified in
215+
// Starlark, for example in a transition implementation function.
216+
return Label.parseWithPackageContext(input, (Label.PackageContext) conversionContext);
217+
}
210218
// Check if the input starts with '/'. We don't check for "//" so that
211219
// we get a better error message if the user accidentally tries to use
212220
// an absolute path (starting with '/') for a label.
213221
if (!input.startsWith("/") && !input.startsWith("@")) {
214222
input = "//" + input;
215223
}
216-
return Label.parseAbsolute(input, ImmutableMap.of());
224+
if (conversionContext == null) {
225+
// This can happen in the first round of option parsing, before repo mappings are
226+
// calculated. In this case, it actually doesn't matter how we parse label-typed flags, as
227+
// they shouldn't be used anywhere anyway.
228+
return Label.parseCanonical(input);
229+
}
230+
Preconditions.checkArgument(
231+
conversionContext instanceof RepositoryMapping,
232+
"bad conversion context type: %s",
233+
conversionContext.getClass().getName());
234+
// This can happen in the second round of option parsing.
235+
return Label.parseWithRepoContext(
236+
input, Label.RepoContext.of(RepositoryName.MAIN, (RepositoryMapping) conversionContext));
217237
} catch (LabelSyntaxException e) {
218238
throw new OptionsParsingException(e.getMessage());
219239
}

‎src/main/java/com/google/devtools/build/lib/analysis/config/RunUnderConverter.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,8 @@
1414
package com.google.devtools.build.lib.analysis.config;
1515

1616
import com.google.common.collect.ImmutableList;
17-
import com.google.common.collect.ImmutableMap;
17+
import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelConverter;
1818
import com.google.devtools.build.lib.cmdline.Label;
19-
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
2019
import com.google.devtools.build.lib.shell.ShellUtils;
2120
import com.google.devtools.build.lib.shell.ShellUtils.TokenizationException;
2221
import com.google.devtools.common.options.Converter;
@@ -28,6 +27,8 @@
2827

2928
/** --run_under options converter. */
3029
public class RunUnderConverter implements Converter<RunUnder> {
30+
private static final LabelConverter LABEL_CONVERTER = new LabelConverter();
31+
3132
@Override
3233
public RunUnder convert(final String input, Object conversionContext)
3334
throws OptionsParsingException {
@@ -45,9 +46,9 @@ public RunUnder convert(final String input, Object conversionContext)
4546
ImmutableList.copyOf(runUnderList.subList(1, runUnderList.size()));
4647
if (runUnderCommand.startsWith("//") || runUnderCommand.startsWith("@")) {
4748
try {
48-
final Label runUnderLabel = Label.parseAbsolute(runUnderCommand, ImmutableMap.of());
49+
final Label runUnderLabel = LABEL_CONVERTER.convert(runUnderCommand, conversionContext);
4950
return new RunUnderLabel(input, runUnderLabel, runUnderSuffix);
50-
} catch (LabelSyntaxException e) {
51+
} catch (OptionsParsingException e) {
5152
throw new OptionsParsingException("Not a valid label " + e.getMessage());
5253
}
5354
} else {

‎src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.google.common.collect.Ordering;
2424
import com.google.common.collect.Sets;
2525
import com.google.devtools.build.lib.cmdline.Label;
26+
import com.google.devtools.build.lib.cmdline.Label.PackageContext;
2627
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
2728
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
2829
import com.google.devtools.build.lib.events.Event;
@@ -77,6 +78,7 @@ public enum Settings {
7778
private final ImmutableMap<String, String> inputsCanonicalizedToGiven;
7879
private final ImmutableMap<String, String> outputsCanonicalizedToGiven;
7980
private final Location location;
81+
private final Label.PackageContext packageContext;
8082

8183
private StarlarkDefinedConfigTransition(
8284
List<String> inputs,
@@ -86,13 +88,18 @@ private StarlarkDefinedConfigTransition(
8688
Location location)
8789
throws EvalException {
8890
this.location = location;
91+
packageContext = Label.PackageContext.of(parentLabel.getPackageIdentifier(), repoMapping);
8992

9093
this.outputsCanonicalizedToGiven =
9194
getCanonicalizedSettings(repoMapping, parentLabel, outputs, Settings.OUTPUTS);
9295
this.inputsCanonicalizedToGiven =
9396
getCanonicalizedSettings(repoMapping, parentLabel, inputs, Settings.INPUTS);
9497
}
9598

99+
public PackageContext getPackageContext() {
100+
return packageContext;
101+
}
102+
96103
/**
97104
* Returns a build settings in canonicalized form taking into account repository remappings.
98105
* Native options only have one form so they are always returned unchanged (i.e.

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import java.util.LinkedHashSet;
4545
import java.util.List;
4646
import java.util.Map;
47+
import java.util.Objects;
4748
import java.util.Set;
4849
import java.util.TreeSet;
4950
import java.util.stream.Stream;
@@ -267,10 +268,10 @@ private static BuildOptions applyTransition(
267268
if (!optionKey.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
268269
// The transition changes a Starlark option.
269270
Label optionLabel = Label.parseAbsoluteUnchecked(optionKey);
271+
// TODO(#15728): `optionValue` needs to go through repo mapping, if it's a string and the
272+
// option is label-typed.
270273
Object oldValue = fromOptions.getStarlarkOptions().get(optionLabel);
271-
if ((oldValue == null && optionValue != null)
272-
|| (oldValue != null && optionValue == null)
273-
|| (oldValue != null && !oldValue.equals(optionValue))) {
274+
if (!Objects.equals(oldValue, optionValue)) {
274275
changedStarlarkOptions.put(optionLabel, optionValue);
275276
convertedAffectedOptions.add(optionLabel.toString());
276277
}
@@ -313,8 +314,14 @@ private static BuildOptions applyTransition(
313314
convertedValue =
314315
def.getConverter()
315316
.convert(
316-
optionValueAsList.stream().map(Object::toString).collect(joining(",")),
317-
/*conversionContext=*/ null);
317+
optionValueAsList.stream()
318+
.map(
319+
element ->
320+
element instanceof Label
321+
? ((Label) element).getUnambiguousCanonicalForm()
322+
: element.toString())
323+
.collect(joining(",")),
324+
starlarkTransition.getPackageContext());
318325
}
319326
} else if (def.getType() == List.class && optionValue == null) {
320327
throw ValidationException.format(
@@ -327,15 +334,14 @@ private static BuildOptions applyTransition(
327334
convertedValue = optionValue;
328335
} else if (optionValue instanceof String) {
329336
convertedValue =
330-
def.getConverter().convert((String) optionValue, /*conversionContext=*/ null);
337+
def.getConverter()
338+
.convert((String) optionValue, starlarkTransition.getPackageContext());
331339
} else {
332340
throw ValidationException.format("Invalid value type for option '%s'", optionName);
333341
}
334342

335343
Object oldValue = field.get(fromOptions.get(optionInfo.getOptionClass()));
336-
if ((oldValue == null && convertedValue != null)
337-
|| (oldValue != null && convertedValue == null)
338-
|| (oldValue != null && !oldValue.equals(convertedValue))) {
344+
if (!Objects.equals(oldValue, convertedValue)) {
339345
if (toOptions == null) {
340346
toOptions = fromOptions.clone();
341347
}

‎src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -585,13 +585,9 @@ private static String maybeCanonicalizeLabel(
585585
if (!BuildType.isLabelType(flagTarget.getProvider(BuildSettingProvider.class).getType())) {
586586
return expectedValue;
587587
}
588-
if (!expectedValue.startsWith(":")) {
589-
return expectedValue;
590-
}
591588
try {
592-
return Label.create(
593-
ruleContext.getRule().getPackage().getPackageIdentifier(), expectedValue.substring(1))
594-
.getCanonicalForm();
589+
return Label.parseWithPackageContext(expectedValue, ruleContext.getPackageContext())
590+
.getUnambiguousCanonicalForm();
595591
} catch (LabelSyntaxException e) {
596592
// Swallow this: the subsequent type conversion already checks for this.
597593
return expectedValue;

‎src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515

1616
import com.google.common.base.Splitter;
1717
import com.google.common.collect.ImmutableList;
18-
import com.google.common.collect.ImmutableMap;
1918
import com.google.common.collect.ImmutableSet;
2019
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
2120
import com.google.devtools.build.lib.analysis.config.CompilationMode;
@@ -24,7 +23,6 @@
2423
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
2524
import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
2625
import com.google.devtools.build.lib.cmdline.Label;
27-
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
2826
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
2927
import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode;
3028
import com.google.devtools.build.lib.rules.cpp.CppConfiguration.StripMode;
@@ -77,15 +75,15 @@ public StripModeConverter() {
7775
}
7876
}
7977

80-
private static final String LIBC_RELATIVE_LABEL = ":everything";
81-
8278
/**
8379
* Converts a String, which is a package label into a label that can be used for a LibcTop object.
8480
*/
85-
public static class LibcTopLabelConverter extends Converter.Contextless<Label> {
81+
public static class LibcTopLabelConverter implements Converter<Label> {
82+
private static final LabelConverter LABEL_CONVERTER = new LabelConverter();
83+
8684
@Nullable
8785
@Override
88-
public Label convert(String input) throws OptionsParsingException {
86+
public Label convert(String input, Object conversionContext) throws OptionsParsingException {
8987
if (input.equals(TARGET_LIBC_TOP_NOT_YET_SET)) {
9088
return Label.createUnvalidated(
9189
PackageIdentifier.EMPTY_PACKAGE_ID, TARGET_LIBC_TOP_NOT_YET_SET);
@@ -98,12 +96,8 @@ public Label convert(String input) throws OptionsParsingException {
9896
} else if (!input.startsWith("//")) {
9997
throw new OptionsParsingException("Not a label");
10098
}
101-
try {
102-
return Label.parseAbsolute(input, ImmutableMap.of())
103-
.getRelativeWithRemapping(LIBC_RELATIVE_LABEL, ImmutableMap.of());
104-
} catch (LabelSyntaxException e) {
105-
throw new OptionsParsingException(e.getMessage());
106-
}
99+
return Label.createUnvalidated(
100+
LABEL_CONVERTER.convert(input, conversionContext).getPackageIdentifier(), "everything");
107101
}
108102

109103
@Override

‎src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java‎

Lines changed: 54 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions;
3535
import com.google.devtools.build.lib.buildtool.buildevent.ProfilerStartedEvent;
3636
import com.google.devtools.build.lib.clock.BlazeClock;
37+
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
3738
import com.google.devtools.build.lib.events.Event;
3839
import com.google.devtools.build.lib.events.EventHandler;
3940
import com.google.devtools.build.lib.events.EventKind;
@@ -294,24 +295,14 @@ private BlazeCommandResult execExclusively(
294295
BlazeWorkspace workspace = runtime.getWorkspace();
295296

296297
StoredEventHandler storedEventHandler = new StoredEventHandler();
298+
// Provide the options parser so that we can cache OptionsData here.
299+
OptionsParser optionsParser = createOptionsParser(command);
297300
BlazeOptionHandler optionHandler =
298301
new BlazeOptionHandler(
299-
runtime,
300-
workspace,
301-
command,
302-
commandAnnotation,
303-
// Provide the options parser so that we can cache OptionsData here.
304-
createOptionsParser(command),
305-
invocationPolicy);
302+
runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
306303
DetailedExitCode earlyExitCode = optionHandler.parseOptions(args, storedEventHandler);
307304
OptionsParsingResult options = optionHandler.getOptionsResult();
308305

309-
CommandLineEvent originalCommandLineEvent =
310-
new CommandLineEvent.OriginalCommandLineEvent(
311-
runtime, commandName, options, startupOptionsTaggedWithBazelRc);
312-
CommandLineEvent canonicalCommandLineEvent =
313-
new CommandLineEvent.CanonicalCommandLineEvent(runtime, commandName, options);
314-
315306
// The initCommand call also records the start time for the timestamp granularity monitor.
316307
List<String> commandEnvWarnings = new ArrayList<>();
317308
CommandEnvironment env =
@@ -519,13 +510,6 @@ private BlazeCommandResult execExclusively(
519510
return result;
520511
}
521512

522-
// Log the command line now that the modules have all had a change to register their listeners
523-
// to the event bus.
524-
env.getEventBus().post(unstructuredServerCommandLineEvent);
525-
env.getEventBus().post(originalCommandLineEvent);
526-
env.getEventBus().post(canonicalCommandLineEvent);
527-
env.getEventBus().post(commonOptions.toolCommandLine);
528-
529513
for (BlazeModule module : runtime.getBlazeModules()) {
530514
try (SilentCloseable closeable =
531515
Profiler.instance().profile(module + ".injectExtraPrecomputedValues")) {
@@ -553,11 +537,42 @@ private BlazeCommandResult execExclusively(
553537
earlyExitCode = e.getDetailedExitCode();
554538
}
555539
if (!earlyExitCode.isSuccess()) {
556-
replayEarlyExitEvents(
557-
outErr,
558-
optionHandler,
559-
storedEventHandler,
560-
env,
540+
reporter.post(
541+
new NoBuildEvent(
542+
commandName, firstContactTime, false, true, env.getCommandId().toString()));
543+
result = BlazeCommandResult.detailedExitCode(earlyExitCode);
544+
return result;
545+
}
546+
547+
// Compute the repo mapping of the main repo and re-parse options so that we get correct
548+
// values for label-typed options.
549+
try {
550+
RepositoryMapping mainRepoMapping =
551+
env.getSkyframeExecutor().getMainRepoMapping(reporter);
552+
optionsParser = optionsParser.toBuilder().withConversionContext(mainRepoMapping).build();
553+
} catch (InterruptedException e) {
554+
Thread.currentThread().interrupt();
555+
String message = "command interrupted while computing main repo mapping";
556+
reporter.handle(Event.error(message));
557+
earlyExitCode = InterruptedFailureDetails.detailedExitCode(message);
558+
} catch (AbruptExitException e) {
559+
logger.atInfo().withCause(e).log("Error computing main repo mapping");
560+
reporter.handle(Event.error(e.getMessage()));
561+
earlyExitCode = e.getDetailedExitCode();
562+
}
563+
if (!earlyExitCode.isSuccess()) {
564+
reporter.post(
565+
new NoBuildEvent(
566+
commandName, firstContactTime, false, true, env.getCommandId().toString()));
567+
result = BlazeCommandResult.detailedExitCode(earlyExitCode);
568+
return result;
569+
}
570+
optionHandler =
571+
new BlazeOptionHandler(
572+
runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
573+
earlyExitCode = optionHandler.parseOptions(args, reporter);
574+
if (!earlyExitCode.isSuccess()) {
575+
reporter.post(
561576
new NoBuildEvent(
562577
commandName, firstContactTime, false, true, env.getCommandId().toString()));
563578
result = BlazeCommandResult.detailedExitCode(earlyExitCode);
@@ -566,20 +581,28 @@ private BlazeCommandResult execExclusively(
566581
}
567582

568583
// Parse starlark options.
569-
earlyExitCode = optionHandler.parseStarlarkOptions(env, storedEventHandler);
584+
earlyExitCode = optionHandler.parseStarlarkOptions(env, reporter);
570585
if (!earlyExitCode.isSuccess()) {
571-
replayEarlyExitEvents(
572-
outErr,
573-
optionHandler,
574-
storedEventHandler,
575-
env,
586+
reporter.post(
576587
new NoBuildEvent(
577588
commandName, firstContactTime, false, true, env.getCommandId().toString()));
578589
result = BlazeCommandResult.detailedExitCode(earlyExitCode);
579590
return result;
580591
}
581592
options = optionHandler.getOptionsResult();
582593

594+
// Log the command line now that the modules have all had a change to register their listeners
595+
// to the event bus, and the flags have been re-parsed.
596+
CommandLineEvent originalCommandLineEvent =
597+
new CommandLineEvent.OriginalCommandLineEvent(
598+
runtime, commandName, options, startupOptionsTaggedWithBazelRc);
599+
CommandLineEvent canonicalCommandLineEvent =
600+
new CommandLineEvent.CanonicalCommandLineEvent(runtime, commandName, options);
601+
env.getEventBus().post(unstructuredServerCommandLineEvent);
602+
env.getEventBus().post(originalCommandLineEvent);
603+
env.getEventBus().post(canonicalCommandLineEvent);
604+
env.getEventBus().post(commonOptions.toolCommandLine);
605+
583606
// Run the command.
584607
result = command.exec(env, options);
585608

0 commit comments

Comments
 (0)