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)

bazelbuild#14852

PiperOrigin-RevId: 460221378
Change-Id: I3aab3ee02cd5097743172edbe95d33d5e140300a

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

@@ -1833,6 +1833,7 @@ java_library(
     name = "config/run_under_converter",
     srcs = ["config/RunUnderConverter.java"],
     deps = [
+        ":config/core_option_converters",
         ":config/run_under",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/shell",

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

@@ -90,6 +90,11 @@ public Label getLabel() {
     return target.getLabel();
   }
 
+  public Label.PackageContext getPackageContext() {
+    return Label.PackageContext.of(
+        getLabel().getPackageIdentifier(), target.getPackage().getRepositoryMapping());
+  }
+
   /**
    * Returns the configuration for this target. This may return null if the target is supposed to be
    * 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

@@ -22,11 +22,14 @@
 import static com.google.devtools.build.lib.packages.Type.STRING;
 import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
 
+import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.Converters.BooleanConverter;
@@ -207,13 +210,30 @@ public StrictDepsConverter() {
   private static Label convertOptionsLabel(String input, @Nullable Object conversionContext)
       throws OptionsParsingException {
     try {
+      if (conversionContext instanceof Label.PackageContext) {
+        // This can happen if this converter is being used to convert flag values specified in
+        // Starlark, for example in a transition implementation function.
+        return Label.parseWithPackageContext(input, (Label.PackageContext) conversionContext);
+      }
       // Check if the input starts with '/'. We don't check for "//" so that
       // we get a better error message if the user accidentally tries to use
       // an absolute path (starting with '/') for a label.
       if (!input.startsWith("/") && !input.startsWith("@")) {
         input = "//" + input;
       }
-      return Label.parseAbsolute(input, ImmutableMap.of());
+      if (conversionContext == null) {
+        // This can happen in the first round of option parsing, before repo mappings are
+        // calculated. In this case, it actually doesn't matter how we parse label-typed flags, as
+        // they shouldn't be used anywhere anyway.
+        return Label.parseCanonical(input);
+      }
+      Preconditions.checkArgument(
+          conversionContext instanceof RepositoryMapping,
+          "bad conversion context type: %s",
+          conversionContext.getClass().getName());
+      // This can happen in the second round of option parsing.
+      return Label.parseWithRepoContext(
+          input, Label.RepoContext.of(RepositoryName.MAIN, (RepositoryMapping) conversionContext));
     } catch (LabelSyntaxException e) {
       throw new OptionsParsingException(e.getMessage());
     }

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

@@ -14,9 +14,8 @@
 package com.google.devtools.build.lib.analysis.config;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelConverter;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.shell.ShellUtils.TokenizationException;
 import com.google.devtools.common.options.Converter;
@@ -28,6 +27,8 @@
 
 /** --run_under options converter. */
 public class RunUnderConverter implements Converter<RunUnder> {
+  private static final LabelConverter LABEL_CONVERTER = new LabelConverter();
+
   @Override
   public RunUnder convert(final String input, Object conversionContext)
       throws OptionsParsingException {
@@ -45,9 +46,9 @@ public RunUnder convert(final String input, Object conversionContext)
         ImmutableList.copyOf(runUnderList.subList(1, runUnderList.size()));
     if (runUnderCommand.startsWith("//") || runUnderCommand.startsWith("@")) {
       try {
-        final Label runUnderLabel = Label.parseAbsolute(runUnderCommand, ImmutableMap.of());
+        final Label runUnderLabel = LABEL_CONVERTER.convert(runUnderCommand, conversionContext);
         return new RunUnderLabel(input, runUnderLabel, runUnderSuffix);
-      } catch (LabelSyntaxException e) {
+      } catch (OptionsParsingException e) {
         throw new OptionsParsingException("Not a valid label " + e.getMessage());
       }
     } else {

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

@@ -23,6 +23,7 @@
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.Label.PackageContext;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.events.Event;
@@ -77,6 +78,7 @@ public enum Settings {
   private final ImmutableMap<String, String> inputsCanonicalizedToGiven;
   private final ImmutableMap<String, String> outputsCanonicalizedToGiven;
   private final Location location;
+  private final Label.PackageContext packageContext;
 
   private StarlarkDefinedConfigTransition(
       List<String> inputs,
@@ -86,13 +88,18 @@ private StarlarkDefinedConfigTransition(
       Location location)
       throws EvalException {
     this.location = location;
+    packageContext = Label.PackageContext.of(parentLabel.getPackageIdentifier(), repoMapping);
 
     this.outputsCanonicalizedToGiven =
         getCanonicalizedSettings(repoMapping, parentLabel, outputs, Settings.OUTPUTS);
     this.inputsCanonicalizedToGiven =
         getCanonicalizedSettings(repoMapping, parentLabel, inputs, Settings.INPUTS);
   }
 
+  public PackageContext getPackageContext() {
+    return packageContext;
+  }
+
   /**
    * Returns a build settings in canonicalized form taking into account repository remappings.
    * 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

@@ -44,6 +44,7 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.TreeSet;
 import java.util.stream.Stream;
@@ -267,10 +268,10 @@ private static BuildOptions applyTransition(
       if (!optionKey.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
         // The transition changes a Starlark option.
         Label optionLabel = Label.parseAbsoluteUnchecked(optionKey);
+        // TODO(#15728): `optionValue` needs to go through repo mapping, if it's a string and the
+        // option is label-typed.
         Object oldValue = fromOptions.getStarlarkOptions().get(optionLabel);
-        if ((oldValue == null && optionValue != null)
-            || (oldValue != null && optionValue == null)
-            || (oldValue != null && !oldValue.equals(optionValue))) {
+        if (!Objects.equals(oldValue, optionValue)) {
           changedStarlarkOptions.put(optionLabel, optionValue);
           convertedAffectedOptions.add(optionLabel.toString());
         }
@@ -313,8 +314,14 @@ private static BuildOptions applyTransition(
               convertedValue =
                   def.getConverter()
                       .convert(
-                          optionValueAsList.stream().map(Object::toString).collect(joining(",")),
-                          /*conversionContext=*/ null);
+                          optionValueAsList.stream()
+                              .map(
+                                  element ->
+                                      element instanceof Label
+                                          ? ((Label) element).getUnambiguousCanonicalForm()
+                                          : element.toString())
+                              .collect(joining(",")),
+                          starlarkTransition.getPackageContext());
             }
           } else if (def.getType() == List.class && optionValue == null) {
             throw ValidationException.format(
@@ -327,15 +334,14 @@ private static BuildOptions applyTransition(
             convertedValue = optionValue;
           } else if (optionValue instanceof String) {
             convertedValue =
-                def.getConverter().convert((String) optionValue, /*conversionContext=*/ null);
+                def.getConverter()
+                    .convert((String) optionValue, starlarkTransition.getPackageContext());
           } else {
             throw ValidationException.format("Invalid value type for option '%s'", optionName);
           }
 
           Object oldValue = field.get(fromOptions.get(optionInfo.getOptionClass()));
-          if ((oldValue == null && convertedValue != null)
-              || (oldValue != null && convertedValue == null)
-              || (oldValue != null && !oldValue.equals(convertedValue))) {
+          if (!Objects.equals(oldValue, convertedValue)) {
             if (toOptions == null) {
               toOptions = fromOptions.clone();
             }

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

@@ -585,13 +585,9 @@ private static String maybeCanonicalizeLabel(
     if (!BuildType.isLabelType(flagTarget.getProvider(BuildSettingProvider.class).getType())) {
       return expectedValue;
     }
-    if (!expectedValue.startsWith(":")) {
-      return expectedValue;
-    }
     try {
-      return Label.create(
-              ruleContext.getRule().getPackage().getPackageIdentifier(), expectedValue.substring(1))
-          .getCanonicalForm();
+      return Label.parseWithPackageContext(expectedValue, ruleContext.getPackageContext())
+          .getUnambiguousCanonicalForm();
     } catch (LabelSyntaxException e) {
       // Swallow this: the subsequent type conversion already checks for this.
       return expectedValue;

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

@@ -15,7 +15,6 @@
 
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.analysis.config.CompilationMode;
@@ -24,7 +23,6 @@
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.StripMode;
@@ -77,15 +75,15 @@ public StripModeConverter() {
     }
   }
 
-  private static final String LIBC_RELATIVE_LABEL = ":everything";
-
   /**
    * Converts a String, which is a package label into a label that can be used for a LibcTop object.
    */
-  public static class LibcTopLabelConverter extends Converter.Contextless<Label> {
+  public static class LibcTopLabelConverter implements Converter<Label> {
+    private static final LabelConverter LABEL_CONVERTER = new LabelConverter();
+
     @Nullable
     @Override
-    public Label convert(String input) throws OptionsParsingException {
+    public Label convert(String input, Object conversionContext) throws OptionsParsingException {
       if (input.equals(TARGET_LIBC_TOP_NOT_YET_SET)) {
         return Label.createUnvalidated(
             PackageIdentifier.EMPTY_PACKAGE_ID, TARGET_LIBC_TOP_NOT_YET_SET);
@@ -98,12 +96,8 @@ public Label convert(String input) throws OptionsParsingException {
       } else if (!input.startsWith("//")) {
         throw new OptionsParsingException("Not a label");
       }
-      try {
-        return Label.parseAbsolute(input, ImmutableMap.of())
-            .getRelativeWithRemapping(LIBC_RELATIVE_LABEL, ImmutableMap.of());
-      } catch (LabelSyntaxException e) {
-        throw new OptionsParsingException(e.getMessage());
-      }
+      return Label.createUnvalidated(
+          LABEL_CONVERTER.convert(input, conversionContext).getPackageIdentifier(), "everything");
     }
 
     @Override

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

@@ -34,6 +34,7 @@
 import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions;
 import com.google.devtools.build.lib.buildtool.buildevent.ProfilerStartedEvent;
 import com.google.devtools.build.lib.clock.BlazeClock;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
@@ -294,24 +295,14 @@ private BlazeCommandResult execExclusively(
     BlazeWorkspace workspace = runtime.getWorkspace();
 
     StoredEventHandler storedEventHandler = new StoredEventHandler();
+    // Provide the options parser so that we can cache OptionsData here.
+    OptionsParser optionsParser = createOptionsParser(command);
     BlazeOptionHandler optionHandler =
         new BlazeOptionHandler(
-            runtime,
-            workspace,
-            command,
-            commandAnnotation,
-            // Provide the options parser so that we can cache OptionsData here.
-            createOptionsParser(command),
-            invocationPolicy);
+            runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
     DetailedExitCode earlyExitCode = optionHandler.parseOptions(args, storedEventHandler);
     OptionsParsingResult options = optionHandler.getOptionsResult();
 
-    CommandLineEvent originalCommandLineEvent =
-        new CommandLineEvent.OriginalCommandLineEvent(
-            runtime, commandName, options, startupOptionsTaggedWithBazelRc);
-    CommandLineEvent canonicalCommandLineEvent =
-        new CommandLineEvent.CanonicalCommandLineEvent(runtime, commandName, options);
-
     // The initCommand call also records the start time for the timestamp granularity monitor.
     List<String> commandEnvWarnings = new ArrayList<>();
     CommandEnvironment env =
@@ -519,13 +510,6 @@ private BlazeCommandResult execExclusively(
         return result;
       }
 
-      // Log the command line now that the modules have all had a change to register their listeners
-      // to the event bus.
-      env.getEventBus().post(unstructuredServerCommandLineEvent);
-      env.getEventBus().post(originalCommandLineEvent);
-      env.getEventBus().post(canonicalCommandLineEvent);
-      env.getEventBus().post(commonOptions.toolCommandLine);
-
       for (BlazeModule module : runtime.getBlazeModules()) {
         try (SilentCloseable closeable =
             Profiler.instance().profile(module + ".injectExtraPrecomputedValues")) {
@@ -553,11 +537,42 @@ private BlazeCommandResult execExclusively(
           earlyExitCode = e.getDetailedExitCode();
         }
         if (!earlyExitCode.isSuccess()) {
-          replayEarlyExitEvents(
-              outErr,
-              optionHandler,
-              storedEventHandler,
-              env,
+          reporter.post(
+              new NoBuildEvent(
+                  commandName, firstContactTime, false, true, env.getCommandId().toString()));
+          result = BlazeCommandResult.detailedExitCode(earlyExitCode);
+          return result;
+        }
+
+        // Compute the repo mapping of the main repo and re-parse options so that we get correct
+        // values for label-typed options.
+        try {
+          RepositoryMapping mainRepoMapping =
+              env.getSkyframeExecutor().getMainRepoMapping(reporter);
+          optionsParser = optionsParser.toBuilder().withConversionContext(mainRepoMapping).build();
+        } catch (InterruptedException e) {
+          Thread.currentThread().interrupt();
+          String message = "command interrupted while computing main repo mapping";
+          reporter.handle(Event.error(message));
+          earlyExitCode = InterruptedFailureDetails.detailedExitCode(message);
+        } catch (AbruptExitException e) {
+          logger.atInfo().withCause(e).log("Error computing main repo mapping");
+          reporter.handle(Event.error(e.getMessage()));
+          earlyExitCode = e.getDetailedExitCode();
+        }
+        if (!earlyExitCode.isSuccess()) {
+          reporter.post(
+              new NoBuildEvent(
+                  commandName, firstContactTime, false, true, env.getCommandId().toString()));
+          result = BlazeCommandResult.detailedExitCode(earlyExitCode);
+          return result;
+        }
+        optionHandler =
+            new BlazeOptionHandler(
+                runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy);
+        earlyExitCode = optionHandler.parseOptions(args, reporter);
+        if (!earlyExitCode.isSuccess()) {
+          reporter.post(
               new NoBuildEvent(
                   commandName, firstContactTime, false, true, env.getCommandId().toString()));
           result = BlazeCommandResult.detailedExitCode(earlyExitCode);
@@ -566,20 +581,28 @@ private BlazeCommandResult execExclusively(
       }
 
       // Parse starlark options.
-      earlyExitCode = optionHandler.parseStarlarkOptions(env, storedEventHandler);
+      earlyExitCode = optionHandler.parseStarlarkOptions(env, reporter);
       if (!earlyExitCode.isSuccess()) {
-        replayEarlyExitEvents(
-            outErr,
-            optionHandler,
-            storedEventHandler,
-            env,
+        reporter.post(
             new NoBuildEvent(
                 commandName, firstContactTime, false, true, env.getCommandId().toString()));
         result = BlazeCommandResult.detailedExitCode(earlyExitCode);
         return result;
       }
       options = optionHandler.getOptionsResult();
 
+      // Log the command line now that the modules have all had a change to register their listeners
+      // to the event bus, and the flags have been re-parsed.
+      CommandLineEvent originalCommandLineEvent =
+          new CommandLineEvent.OriginalCommandLineEvent(
+              runtime, commandName, options, startupOptionsTaggedWithBazelRc);
+      CommandLineEvent canonicalCommandLineEvent =
+          new CommandLineEvent.CanonicalCommandLineEvent(runtime, commandName, options);
+      env.getEventBus().post(unstructuredServerCommandLineEvent);
+      env.getEventBus().post(originalCommandLineEvent);
+      env.getEventBus().post(canonicalCommandLineEvent);
+      env.getEventBus().post(commonOptions.toolCommandLine);
+
       // Run the command.
       result = command.exec(env, options);