diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index e44639afe8329e..26b75a247f370f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java index 3227fb0940e707..900d0570d2e4a7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java +++ b/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 diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java index dca99f397ae8d2..f202f45cadc3a1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java +++ b/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()); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/RunUnderConverter.java b/src/main/java/com/google/devtools/build/lib/analysis/config/RunUnderConverter.java index a959e4772ca7c3..c74eddb7ee6e7d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/RunUnderConverter.java +++ b/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 { + 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 { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java index 8c662bd0dcd489..97592f1f1c9000 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java +++ b/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 inputsCanonicalizedToGiven; private final ImmutableMap outputsCanonicalizedToGiven; private final Location location; + private final Label.PackageContext packageContext; private StarlarkDefinedConfigTransition( List inputs, @@ -86,6 +88,7 @@ private StarlarkDefinedConfigTransition( Location location) throws EvalException { this.location = location; + packageContext = Label.PackageContext.of(parentLabel.getPackageIdentifier(), repoMapping); this.outputsCanonicalizedToGiven = getCanonicalizedSettings(repoMapping, parentLabel, outputs, Settings.OUTPUTS); @@ -93,6 +96,10 @@ private StarlarkDefinedConfigTransition( 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. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java index 245b1d47245a49..0113f624632da2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java +++ b/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(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java index a84de0afa7a2bc..deaff32747b39b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java +++ b/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; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java index da95bc9aefb844..99674edc4f5f7a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java +++ b/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