From 9d3a8b02b95ed9b51e5d8902463df2b261cd8d15 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 1 Apr 2024 12:49:22 -0700 Subject: [PATCH] Stringify `Label`s in display form in `Args` `Label`s added to `Args` are now formatted in display form, which means that they will be repo-relative if referring to the main repo and use an apparent repository name if possible. Previously, labels were formatted in repo-relative form if referring to the main repo and using canonical repository names exclusively for external repos. At the same time, `Args` docs explicitly stated that the exact stringification of any type other than `File` is unspecified. The previous behavior was problematic since it neither always used canonical labels (which could be useful for writing tests that check these labels against an allowlist) nor provided labels that could be included in BUILD files (canonical names are explicitly unstable). Furthermore, whether a label resulted in a string prefixed with a single or two `@`s already dependended on a user choice, namely the value of `--enable_bzlmod`. Thus, this change is not considered to be breaking. It makes it so that existing rulesets relying on default stringification to return a BUILD-file compatible label in WORKSPACE continue to do so with Bzlmod with no code changes. This change aims to be as memory efficient as possible: * Single labels or sequences of labels that reference targets in the main repo incur no memory overhead. * Labels referring to external repos as well as `NestedSet`s of labels result in an additional Skyframe dependency for each target using the command line as well as one additional reference (4 bytes) stored in the command line's `arguments`. Work towards #20486 Closes #21702. PiperOrigin-RevId: 620925978 Change-Id: I54aa807c41bf783aee223482d2309f5cee2726b5 --- .../lib/analysis/AnalysisEnvironment.java | 6 + .../google/devtools/build/lib/analysis/BUILD | 4 + .../analysis/CachingAnalysisEnvironment.java | 16 ++ .../build/lib/analysis/starlark/Args.java | 59 +++++-- .../starlark/StarlarkActionFactory.java | 28 ++- .../starlark/StarlarkCustomCommandLine.java | 89 ++++++++-- .../build/lib/cmdline/RepositoryMapping.java | 4 + .../starlarkbuildapi/CommandLineArgsApi.java | 8 + .../StarlarkActionFactoryApi.java | 7 +- .../analysis/starlark/ArgsParamFileTest.java | 6 +- .../analysis/starlark/FlagPerLineTest.java | 6 +- .../StarlarkCustomCommandLineTest.java | 59 +++++-- .../lib/analysis/util/AnalysisTestUtil.java | 11 ++ .../lib/analysis/util/BuildViewTestCase.java | 6 + .../google/devtools/build/lib/starlark/BUILD | 1 + ...arlarkRuleImplementationFunctionsTest.java | 162 +++++++++++++++++- src/test/shell/bazel/BUILD | 13 +- src/test/shell/bazel/bazel_java_test.sh | 40 +++++ 18 files changed, 450 insertions(+), 75 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java index d4b6ec7ff51dc9..4f1ed9dec21cf2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisEnvironment.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.MiddlemanFactory; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyFunction; @@ -166,4 +167,9 @@ Artifact.DerivedArtifact getDerivedArtifact( ImmutableSet getTreeArtifactsConflictingWithFiles(); ActionKeyContext getActionKeyContext(); + + /** + * Returns and registers a Skyframe dependency on the {@link RepositoryMapping} of the main repo. + */ + RepositoryMapping getMainRepoMapping() throws InterruptedException; } 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 06eec0debc63d1..ed76de6b3acb11 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -411,6 +411,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception", + "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value", "//src/main/java/com/google/devtools/build/lib/skyframe:workspace_status_value", "//src/main/java/com/google/devtools/build/lib/skyframe/config", @@ -421,6 +422,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/core", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test", + "//src/main/java/com/google/devtools/build/lib/supplier", "//src/main/java/com/google/devtools/build/lib/unsafe:string", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", @@ -2450,9 +2452,11 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", + "//src/main/java/com/google/devtools/build/lib/supplier", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java index 0ee01e8cf7b47d..069019856446c3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java @@ -27,9 +27,12 @@ import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.MiddlemanFactory; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.skyframe.RepositoryMappingValue; import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsValue; import com.google.devtools.build.lib.skyframe.WorkspaceStatusValue; import com.google.devtools.build.lib.util.Pair; @@ -368,6 +371,19 @@ private WorkspaceStatusValue getWorkspaceStatusValue() throws InterruptedExcepti return workspaceStatusValue; } + @Override + public RepositoryMapping getMainRepoMapping() throws InterruptedException { + var mainRepoMapping = + (RepositoryMappingValue) + skyframeEnv.getValue(RepositoryMappingValue.key(RepositoryName.MAIN)); + if (mainRepoMapping == null) { + // This isn't expected to happen since the main repository mapping is computed before the + // analysis phase. + throw new MissingDepException("Restart due to missing main repository mapping"); + } + return mainRepoMapping.getRepositoryMapping(); + } + @Override public ActionLookupKey getOwner() { return owner; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java index 8c133dc8712369..77a19edf02c2fd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java @@ -23,10 +23,13 @@ import com.google.devtools.build.lib.actions.ParamFileInfo; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.SingleStringArgFormatter; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.starlarkbuildapi.CommandLineArgsApi; +import com.google.devtools.build.lib.supplier.InterruptibleSupplier; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.nio.charset.StandardCharsets; import java.util.ArrayList; @@ -70,7 +73,8 @@ public void repr(Printer printer) { @Override public void debugPrint(Printer printer, StarlarkSemantics semantics) { try { - printer.append(Joiner.on(" ").join(build().arguments())); + printer.append( + Joiner.on(" ").join(build(/* mainRepoMappingSupplier= */ () -> null).arguments())); } catch (CommandLineExpansionException e) { printer.append("Cannot expand command line: " + e.getMessage()); } catch (InterruptedException e) { @@ -102,7 +106,8 @@ public void debugPrint(Printer printer, StarlarkSemantics semantics) { public abstract ImmutableSet getDirectoryArtifacts(); /** Returns the command line built by this {@link Args} object. */ - public abstract CommandLine build(); + public abstract CommandLine build( + InterruptibleSupplier mainRepoMappingSupplier) throws InterruptedException; /** * Returns a frozen {@link Args} representation corresponding to an already-registered action. @@ -157,7 +162,7 @@ public ImmutableSet getDirectoryArtifacts() { } @Override - public CommandLine build() { + public CommandLine build(InterruptibleSupplier mainRepoMappingSupplier) { return commandLine; } @@ -259,6 +264,12 @@ private static class MutableArgs extends Args implements StarlarkValue, Mutabili */ private boolean flagPerLine = false; + /** + * True if the command line needs to stringify any {@link Label}s without an explicit 'map_each' + * function. + */ + private boolean mayStringifyExternalLabel = false; + // May be set explicitly once -- if unset defaults to ParameterFileType.SHELL_QUOTED. private ParameterFileType parameterFileType = null; private String flagFormatString; @@ -307,6 +318,9 @@ public CommandLineArgsApi addArgument( "Args.add() doesn't accept vectorized arguments. Please use Args.add_all() or" + " Args.add_joined() instead."); } + if (value instanceof Label label && !label.getRepository().isMain()) { + mayStringifyExternalLabel = true; + } addSingleArg(value, format != Starlark.NONE ? (String) format : null); return this; } @@ -436,8 +450,12 @@ private void addVectorArg( validateFormatString("format_each", formatEach); validateFormatString("format_joined", formatJoined); StarlarkCustomCommandLine.VectorArg.Builder vectorArg; - if (value instanceof Depset) { - Depset starlarkNestedSet = (Depset) value; + if (value instanceof Depset starlarkNestedSet) { + if (mapEach == null && Label.class.equals(starlarkNestedSet.getElementClass())) { + // We don't want to eagerly check whether all labels reference targets in the main repo, + // so just assume they might not. Nested sets of labels should be rare. + mayStringifyExternalLabel = true; + } NestedSet nestedSet = starlarkNestedSet.getSet(); if (nestedSet.isEmpty() && omitIfEmpty) { return; @@ -451,8 +469,16 @@ private void addVectorArg( if (starlarkList.isEmpty() && omitIfEmpty) { return; } - if (expandDirectories) { - scanForDirectories(starlarkList); + for (Object object : starlarkList) { + if (expandDirectories && isDirectory(object)) { + directoryArtifacts.add((Artifact) object); + } + // Labels referencing targets in the main repo are stringified as //pkg:name and thus + // don't require a RepositoryMapping. If a map_each function is provided, default + // stringification via Label#toString() is not used. + if (mapEach == null && object instanceof Label label && !label.getRepository().isMain()) { + mayStringifyExternalLabel = true; + } } vectorArg = new StarlarkCustomCommandLine.VectorArg.Builder(starlarkList); } @@ -573,8 +599,10 @@ private MutableArgs(@Nullable Mutability mutability, StarlarkSemantics starlarkS } @Override - public CommandLine build() { - return commandLine.build(flagPerLine); + public CommandLine build(InterruptibleSupplier mainRepoMappingSupplier) + throws InterruptedException { + return commandLine.build( + flagPerLine, mayStringifyExternalLabel ? mainRepoMappingSupplier.get() : null); } @Override @@ -585,18 +613,15 @@ public Mutability mutability() { @Override public ImmutableSet getDirectoryArtifacts() { for (NestedSet collection : potentialDirectoryArtifacts) { - scanForDirectories(collection.toList()); + for (Object object : collection.toList()) { + if (isDirectory(object)) { + directoryArtifacts.add((Artifact) object); + } + } } potentialDirectoryArtifacts.clear(); return ImmutableSet.copyOf(directoryArtifacts); } - private void scanForDirectories(Iterable objects) { - for (Object object : objects) { - if (isDirectory(object)) { - directoryArtifacts.add((Artifact) object); - } - } - } } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java index 9091c80ff8dc03..19f75a9cf303a0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java @@ -51,6 +51,7 @@ import com.google.devtools.build.lib.analysis.platform.PlatformInfo; 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.collect.nestedset.Depset; import com.google.devtools.build.lib.collect.nestedset.Depset.TypeException; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -68,6 +69,7 @@ import com.google.devtools.build.lib.starlarkbuildapi.FileApi; import com.google.devtools.build.lib.starlarkbuildapi.StarlarkActionFactoryApi; import com.google.devtools.build.lib.starlarkbuildapi.TemplateDictApi; +import com.google.devtools.build.lib.supplier.InterruptibleSupplier; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.protobuf.GeneratedMessage; @@ -332,7 +334,8 @@ public void symlink( } @Override - public void write(FileApi output, Object content, Boolean isExecutable) throws EvalException { + public void write(FileApi output, Object content, Boolean isExecutable) + throws EvalException, InterruptedException { context.checkMutable("actions.write"); RuleContext ruleContext = getRuleContext(); @@ -347,7 +350,7 @@ public void write(FileApi output, Object content, Boolean isExecutable) throws E ruleContext.getActionOwner(), NestedSetBuilder.wrap(Order.STABLE_ORDER, args.getDirectoryArtifacts()), (Artifact) output, - args.build(), + args.build(getMainRepoMappingSupplier()), args.getParameterFileType()); } else { throw new AssertionError("Unexpected type: " + content.getClass().getSimpleName()); @@ -373,7 +376,7 @@ public void run( Object shadowedActionUnchecked, Object resourceSetUnchecked, Object toolchainUnchecked) - throws EvalException { + throws EvalException, InterruptedException { context.checkMutable("actions.run"); execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked); toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked); @@ -382,7 +385,7 @@ public void run( boolean useAutoExecGroups = ruleContext.useAutoExecGroups(); StarlarkAction.Builder builder = new StarlarkAction.Builder(); - buildCommandLine(builder, arguments); + buildCommandLine(builder, arguments, getMainRepoMappingSupplier()); if (executableUnchecked instanceof Artifact) { Artifact executable = (Artifact) executableUnchecked; FilesToRunProvider provider = context.getExecutableRunfiles(executable, "executable"); @@ -560,7 +563,7 @@ public void runShell( Object shadowedActionUnchecked, Object resourceSetUnchecked, Object toolchainUnchecked) - throws EvalException { + throws EvalException, InterruptedException { context.checkMutable("actions.run_shell"); execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked); toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked); @@ -568,7 +571,7 @@ public void runShell( RuleContext ruleContext = getRuleContext(); StarlarkAction.Builder builder = new StarlarkAction.Builder(); - buildCommandLine(builder, arguments); + buildCommandLine(builder, arguments, getMainRepoMappingSupplier()); // When we use a shell command, add an empty argument before other arguments. // e.g. bash -c "cmd" '' 'arg1' 'arg2' @@ -631,8 +634,11 @@ public void runShell( builder); } - private static void buildCommandLine(SpawnAction.Builder builder, Sequence argumentsList) - throws EvalException { + private static void buildCommandLine( + SpawnAction.Builder builder, + Sequence argumentsList, + InterruptibleSupplier repoMappingSupplier) + throws EvalException, InterruptedException { ImmutableList.Builder stringArgs = null; for (Object value : argumentsList) { if (value instanceof String) { @@ -647,7 +653,7 @@ private static void buildCommandLine(SpawnAction.Builder builder, Sequence ar } Args args = (Args) value; ParamFileInfo paramFileInfo = args.getParamFileInfo(); - builder.addCommandLine(args.build(), paramFileInfo); + builder.addCommandLine(args.build(repoMappingSupplier), paramFileInfo); } else { throw Starlark.errorf( "expected list of strings or ctx.actions.args() for arguments instead of %s", @@ -1046,6 +1052,10 @@ public void repr(Printer printer) { context.repr(printer); } + private InterruptibleSupplier getMainRepoMappingSupplier() { + return context.getRuleContext().getAnalysisEnvironment()::getMainRepoMapping; + } + /** The analysis context for {@code Starlark} actions */ // For now, this contains methods necessary for SubruleContext to begin using // StarlarkActionFactory without any invasive changes to the latter. It will be improved once the diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java index 5562e5026bbb00..bfb2f15e69b063 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java @@ -22,6 +22,7 @@ import com.google.common.collect.Interner; import com.google.common.collect.Iterables; import com.google.common.collect.Iterators; +import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.common.collect.UnmodifiableIterator; import com.google.devtools.build.lib.actions.ActionKeyContext; @@ -41,6 +42,8 @@ import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.SingleStringArgFormatter; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization; @@ -256,7 +259,8 @@ private int preprocess( int argi, PreprocessedCommandLine.Builder builder, @Nullable ArtifactExpander artifactExpander, - PathMapper pathMapper) + PathMapper pathMapper, + @Nullable RepositoryMapping mainRepoMapping) throws CommandLineExpansionException, InterruptedException { StarlarkCallable mapEach = null; StarlarkSemantics starlarkSemantics = null; @@ -293,7 +297,7 @@ private int preprocess( int count = expandedValues.size(); stringValues = new ArrayList<>(expandedValues.size()); for (int i = 0; i < count; ++i) { - stringValues.add(expandToCommandLine(expandedValues.get(i), pathMapper)); + stringValues.add(expandToCommandLine(expandedValues.get(i), pathMapper, mainRepoMapping)); } } // It's safe to uniquify at this stage, any transformations after this @@ -690,6 +694,7 @@ static void push(List arguments, Object object, String format) { * @param builder the {@link PreprocessedCommandLine.Builder} in which to add a preprocessed * representation of this arg * @param pathMapper mapper for exec paths + * @param mainRepoMapping the repository mapping to use for formatting labels if needed * @return index in {@code arguments} where the next arg begins, or {@code arguments.size()} if * there are no more arguments */ @@ -697,10 +702,12 @@ static int preprocess( List arguments, int argi, PreprocessedCommandLine.Builder builder, - PathMapper pathMapper) { + PathMapper pathMapper, + @Nullable RepositoryMapping mainRepoMapping) { Object object = arguments.get(argi++); String formatStr = (String) arguments.get(argi++); - String stringValue = StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper); + String stringValue = + StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper, mainRepoMapping); builder.addPreprocessedArg(new PreprocessedSingleFormattedArg(formatStr, stringValue)); return argi; } @@ -754,11 +761,17 @@ Builder addFormatted(Object object, String format) { return this; } - CommandLine build(boolean flagPerLine) { + CommandLine build(boolean flagPerLine, @Nullable RepositoryMapping mainRepoMapping) { if (arguments.isEmpty()) { return CommandLine.empty(); } - Object[] args = arguments.toArray(); + Object[] args; + if (mainRepoMapping != null) { + args = arguments.toArray(new Object[arguments.size() + 1]); + args[arguments.size()] = mainRepoMapping; + } else { + args = arguments.toArray(); + } return flagPerLine ? new StarlarkCustomCommandLineWithIndexes(args, argStartIndexes.build()) : new StarlarkCustomCommandLine(args); @@ -791,14 +804,29 @@ public ArgChunk expand(@Nullable ArtifactExpander artifactExpander, PathMapper p PreprocessedCommandLine.Builder builder = new PreprocessedCommandLine.Builder(); List arguments = rawArgsAsList(); - for (int argi = 0; argi < arguments.size(); ) { + RepositoryMapping mainRepoMapping; + int size; + // Added in #build() if any labels in the command line require this to be formatted with an + // apparent repository name. + if (arguments.getLast() instanceof RepositoryMapping) { + mainRepoMapping = (RepositoryMapping) arguments.getLast(); + size = arguments.size() - 1; + } else { + mainRepoMapping = null; + size = arguments.size(); + } + + for (int argi = 0; argi < size; ) { Object arg = arguments.get(argi++); if (arg instanceof VectorArg) { - argi = ((VectorArg) arg).preprocess(arguments, argi, builder, artifactExpander, pathMapper); + argi = + ((VectorArg) arg) + .preprocess( + arguments, argi, builder, artifactExpander, pathMapper, mainRepoMapping); } else if (arg == SingleFormattedArg.MARKER) { - argi = SingleFormattedArg.preprocess(arguments, argi, builder, pathMapper); + argi = SingleFormattedArg.preprocess(arguments, argi, builder, pathMapper, mainRepoMapping); } else { - builder.addString(expandToCommandLine(arg, pathMapper)); + builder.addString(expandToCommandLine(arg, pathMapper, mainRepoMapping)); } } return pathMapper.mapCustomStarlarkArgs(builder.build()); @@ -816,7 +844,12 @@ public final Iterable arguments(ArtifactExpander artifactExpander, PathM return expand(artifactExpander, pathMapper).arguments(); } - private static String expandToCommandLine(Object object, PathMapper pathMapper) { + private static String expandToCommandLine( + Object object, PathMapper pathMapper, @Nullable RepositoryMapping mainRepoMapping) { + if (mainRepoMapping != null && object instanceof Label label) { + return label.getDisplayForm(mainRepoMapping); + } + // It'd be nice to build this into DerivedArtifact's CommandLine interface so we don't have // to explicitly check if an object is a DerivedArtifact. Unfortunately that would require // a lot more dependencies on the Java library DerivedArtifact is built into. @@ -848,20 +881,32 @@ public ArgChunk expand(@Nullable ArtifactExpander artifactExpander, PathMapper p List arguments = ((StarlarkCustomCommandLine) this).rawArgsAsList(); Iterator startIndexIterator = argStartIndexes.iterator(); - for (int argi = 0; argi < arguments.size(); ) { - int nextStartIndex = - startIndexIterator.hasNext() ? startIndexIterator.next() : arguments.size(); + RepositoryMapping mainRepoMapping; + int size; + if (arguments.getLast() instanceof RepositoryMapping) { + mainRepoMapping = (RepositoryMapping) arguments.getLast(); + size = arguments.size() - 1; + } else { + mainRepoMapping = null; + size = arguments.size(); + } + + for (int argi = 0; argi < size; ) { + int nextStartIndex = startIndexIterator.hasNext() ? startIndexIterator.next() : size; PreprocessedCommandLine.Builder line = new PreprocessedCommandLine.Builder(); while (argi < nextStartIndex) { Object arg = arguments.get(argi++); if (arg instanceof VectorArg) { argi = - ((VectorArg) arg).preprocess(arguments, argi, line, artifactExpander, pathMapper); + ((VectorArg) arg) + .preprocess( + arguments, argi, line, artifactExpander, pathMapper, mainRepoMapping); } else if (arg == SingleFormattedArg.MARKER) { - argi = SingleFormattedArg.preprocess(arguments, argi, line, pathMapper); + argi = + SingleFormattedArg.preprocess(arguments, argi, line, pathMapper, mainRepoMapping); } else { - line.addString(expandToCommandLine(arg, pathMapper)); + line.addString(expandToCommandLine(arg, pathMapper, mainRepoMapping)); } } @@ -879,7 +924,15 @@ public void addToFingerprint( Fingerprint fingerprint) throws CommandLineExpansionException, InterruptedException { List arguments = rawArgsAsList(); - for (int argi = 0; argi < arguments.size(); ) { + int size; + if (arguments.getLast() instanceof RepositoryMapping mainRepoMapping) { + fingerprint.addStringMap( + Maps.transformValues(mainRepoMapping.entries(), RepositoryName::getName)); + size = arguments.size() - 1; + } else { + size = arguments.size(); + } + for (int argi = 0; argi < size; ) { Object arg = arguments.get(argi++); if (arg instanceof VectorArg) { argi = diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java index 6bffe81b4b1efd..478f0d24f4c8a9 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java @@ -31,6 +31,10 @@ *

For repositories from the WORKSPACE file, if the requested repo doesn't exist in the mapping, * we fallback to the requested name. For repositories from Bzlmod, we return null to let the caller * decide what to do. + * + *

This class must not implement {@link net.starlark.java.eval.StarlarkValue} since instances of + * this class are used as markers by {@link + * com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine}. */ public class RepositoryMapping { /* A repo mapping that always falls back to using the apparent name as the canonical name. */ diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java index e5272bd39eeb04..b46b8e89ae3cfa 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/CommandLineArgsApi.java @@ -66,6 +66,14 @@ + "

  • Values that are already strings are left as-is." // + "
  • File objects are turned into" + " their File.path values." // + + "
  • Label objects are turned into" + + " a string representation that resolves back to the same object when resolved in the" + + " context of the main repository. If possible, the string representation uses the" + + " apparent name of a repository in favor of the repository's canonical name, which" + + " makes this representation suited for use in BUILD files. While the exact form of" + + " the representation is not guaranteed, typical examples are" + + " //foo:bar, @repo//foo:bar and" + + " @@canonical_name~//foo:bar.bzl." + "
  • All other types are turned into strings in an unspecified manner. For " + " this reason, you should avoid passing values that are not of string or " + " File type to add(), and if you pass them to " diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkActionFactoryApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkActionFactoryApi.java index e302b1481d260a..0e061301e88ee6 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkActionFactoryApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkActionFactoryApi.java @@ -302,7 +302,8 @@ void symlink( doc = "Whether the output file should be executable.", named = true) }) - void write(FileApi output, Object content, Boolean isExecutable) throws EvalException; + void write(FileApi output, Object content, Boolean isExecutable) + throws EvalException, InterruptedException; @StarlarkMethod( name = "run", @@ -553,7 +554,7 @@ void run( Object shadowedAction, Object resourceSetUnchecked, Object toolchainUnchecked) - throws EvalException; + throws EvalException, InterruptedException; @StarlarkMethod( name = "run_shell", @@ -804,7 +805,7 @@ void runShell( Object shadowedAction, Object resourceSetUnchecked, Object toolchainUnchecked) - throws EvalException; + throws EvalException, InterruptedException; @StarlarkMethod( name = "expand_template", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java index 258c052f8682e3..ed374fbdc52099 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/ArgsParamFileTest.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.ParamFileInfo; import com.google.devtools.build.lib.actions.ParameterFile; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.util.ShellEscaper; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -143,7 +144,10 @@ private static ImmutableList toParamFile(Args args) throws Exception { byte[] bytes; try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) { ParameterFile.writeParameterFile( - outputStream, args.build().arguments(), args.getParameterFileType(), UTF_8); + outputStream, + args.build(() -> RepositoryMapping.ALWAYS_FALLBACK).arguments(), + args.getParameterFileType(), + UTF_8); bytes = outputStream.toByteArray(); } try (ByteArrayInputStream inputStream = new ByteArrayInputStream(bytes); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java index eb5bc915bbcdc5..686f4e7ea51c0f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/FlagPerLineTest.java @@ -21,6 +21,7 @@ import com.google.common.io.CharStreams; import com.google.devtools.build.lib.actions.ParameterFile; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.InputStreamReader; @@ -166,7 +167,10 @@ private static ImmutableList toParamFile(Args args) throws Exception { byte[] bytes; try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) { ParameterFile.writeParameterFile( - outputStream, args.build().arguments(), args.getParameterFileType(), UTF_8); + outputStream, + args.build(() -> RepositoryMapping.ALWAYS_FALLBACK).arguments(), + args.getParameterFileType(), + UTF_8); bytes = outputStream.toByteArray(); } try (ByteArrayInputStream inputStream = new ByteArrayInputStream(bytes); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java index 7b0c6922520a20..772d33d70d60c2 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine.VectorArg; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.Path; @@ -74,7 +75,11 @@ public void createArtifactRoot() throws IOException { @Test public void add() throws Exception { CommandLine commandLine = - builder.add("one").add("two").add("three").build(/* flagPerLine= */ false); + builder + .add("one") + .add("two") + .add("three") + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "one", "two", "three"); } @@ -85,7 +90,7 @@ public void addFormatted() throws Exception { .addFormatted("one", "--arg1=%s") .addFormatted("two", "--arg2=%s") .addFormatted("three", "--arg3=%s") - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "--arg1=one", "--arg2=two", "--arg3=three"); } @@ -95,7 +100,7 @@ public void argName() throws Exception { builder .add(vectorArg("one", "two", "three").setArgName("--arg")) .add(vectorArg("four").setArgName("--other_arg")) - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "--arg", "one", "two", "three", "--other_arg", "four"); } @@ -105,7 +110,7 @@ public void terminateWith() throws Exception { builder .add(vectorArg("one", "two", "three").setTerminateWith("end1")) .add(vectorArg("four").setTerminateWith("end2")) - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "one", "two", "three", "end1", "four", "end2"); } @@ -115,7 +120,7 @@ public void formatEach() throws Exception { builder .add(vectorArg("one", "two", "three").setFormatEach("--arg=%s")) .add(vectorArg("four").setFormatEach("--other_arg=%s")) - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "--arg=one", "--arg=two", "--arg=three", "--other_arg=four"); } @@ -125,7 +130,7 @@ public void beforeEach() throws Exception { builder .add(vectorArg("one", "two", "three").setBeforeEach("b4")) .add(vectorArg("four").setBeforeEach("and")) - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "b4", "one", "b4", "two", "b4", "three", "and", "four"); } @@ -135,7 +140,7 @@ public void joinWith() throws Exception { builder .add(vectorArg("one", "two", "three").setJoinWith("...")) .add(vectorArg("four").setJoinWith("n/a")) - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "one...two...three", "four"); } @@ -145,7 +150,7 @@ public void formatJoined() throws Exception { builder .add(vectorArg("one", "two", "three").setJoinWith("...").setFormatJoined("--arg=%s")) .add(vectorArg("four").setJoinWith("n/a").setFormatJoined("--other_arg=%s")) - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "--arg=one...two...three", "--other_arg=four"); } @@ -156,7 +161,7 @@ public void emptyVectorArg_omit() throws Exception { .add("before") .add(vectorArg().omitIfEmpty(true).setJoinWith(",").setFormatJoined("--empty=%s")) .add("after") - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "before", "after"); } @@ -167,7 +172,7 @@ public void emptyVectorArg_noOmit() throws Exception { .add("before") .add(vectorArg().omitIfEmpty(false).setJoinWith(",").setFormatJoined("--empty=%s")) .add("after") - .build(/* flagPerLine= */ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine(commandLine, "before", "--empty=", "after"); } @@ -184,7 +189,7 @@ public void flagPerLine() throws Exception { .add("single_arg") .recordArgStart() .add(vectorArg("", "line", "four", "has", "no").setTerminateWith("flag")) - .build(/* flagPerLine= */ true); + .build(/* flagPerLine= */ true, RepositoryMapping.ALWAYS_FALLBACK); verifyCommandLine( commandLine, "--this=is line one", @@ -198,7 +203,9 @@ public void vectorArgAddToFingerprint_treeArtifactMissingExpansion_returnsDigest throws Exception { SpecialArtifact tree = createTreeArtifact("tree"); CommandLine commandLine = - builder.add(vectorArg(tree).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(tree).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); ActionKeyContext actionKeyContext = new ActionKeyContext(); Fingerprint fingerprint = new Fingerprint(); @@ -212,7 +219,9 @@ public void vectorArgAddToFingerprint_treeArtifactMissingExpansion_returnsDigest public void vectorArgAddToFingerprint_expandFileset_includesInDigest() throws Exception { SpecialArtifact fileset = createFileset("fileset"); CommandLine commandLine = - builder.add(vectorArg(fileset).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(fileset).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); FilesetOutputSymlink symlink1 = createFilesetSymlink("file1"); FilesetOutputSymlink symlink2 = createFilesetSymlink("file2"); ActionKeyContext actionKeyContext = new ActionKeyContext(); @@ -231,7 +240,9 @@ public void vectorArgAddToFingerprint_expandFileset_includesInDigest() throws Ex public void vectorArgAddToFingerprint_expandTreeArtifact_includesInDigest() throws Exception { SpecialArtifact tree = createTreeArtifact("tree"); CommandLine commandLine = - builder.add(vectorArg(tree).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(tree).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); TreeFileArtifact child = TreeFileArtifact.createTreeOutput(tree, "child"); ActionKeyContext actionKeyContext = new ActionKeyContext(); Fingerprint fingerprint = new Fingerprint(); @@ -249,7 +260,9 @@ public void vectorArgAddToFingerprint_expandTreeArtifact_includesInDigest() thro public void vectorArgAddToFingerprint_expandFilesetMissingExpansion_fails() { SpecialArtifact fileset = createFileset("fileset"); CommandLine commandLine = - builder.add(vectorArg(fileset).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(fileset).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); ActionKeyContext actionKeyContext = new ActionKeyContext(); Fingerprint fingerprint = new Fingerprint(); @@ -262,7 +275,9 @@ public void vectorArgAddToFingerprint_expandFilesetMissingExpansion_fails() { public void vectorArgArguments_expandsTreeArtifact() throws Exception { SpecialArtifact tree = createTreeArtifact("tree"); CommandLine commandLine = - builder.add(vectorArg(tree).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(tree).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(tree, "child1"); TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(tree, "child2"); ArtifactExpander artifactExpander = @@ -279,7 +294,9 @@ public void vectorArgArguments_expandsTreeArtifact() throws Exception { public void vectorArgArguments_expandsFileset() throws Exception { SpecialArtifact fileset = createFileset("fileset"); CommandLine commandLine = - builder.add(vectorArg(fileset).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(fileset).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); FilesetOutputSymlink symlink1 = createFilesetSymlink("file1"); FilesetOutputSymlink symlink2 = createFilesetSymlink("file2"); ArtifactExpander artifactExpander = @@ -296,7 +313,9 @@ public void vectorArgArguments_expandsFileset() throws Exception { public void vectorArgArguments_treeArtifactMissingExpansion_returnsEmptyList() throws Exception { SpecialArtifact tree = createTreeArtifact("tree"); CommandLine commandLine = - builder.add(vectorArg(tree).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(tree).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); // TODO(b/167696101): Fail arguments computation when we are missing the directory from inputs. assertThat(commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP)).isEmpty(); @@ -306,7 +325,9 @@ public void vectorArgArguments_treeArtifactMissingExpansion_returnsEmptyList() t public void vectorArgArguments_filesetMissingExpansion_fails() { SpecialArtifact fileset = createFileset("fileset"); CommandLine commandLine = - builder.add(vectorArg(fileset).setExpandDirectories(true)).build(/* flagPerLine= */ false); + builder + .add(vectorArg(fileset).setExpandDirectories(true)) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); assertThrows( CommandLineExpansionException.class, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java index 42ed8ad774cbbf..f8391718f26b4b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.analysis.config.BuildOptionsView; import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -225,6 +226,11 @@ public ImmutableSet getTreeArtifactsConflictingWithFiles() { public ActionKeyContext getActionKeyContext() { return original.getActionKeyContext(); } + + @Override + public RepositoryMapping getMainRepoMapping() throws InterruptedException { + return original.getMainRepoMapping(); + } } /** A dummy WorkspaceStatusAction. */ @@ -455,6 +461,11 @@ public ImmutableSet getTreeArtifactsConflictingWithFiles() { public ActionKeyContext getActionKeyContext() { return null; } + + @Override + public RepositoryMapping getMainRepoMapping() { + return RepositoryMapping.ALWAYS_FALLBACK; + } } /** Matches the output path prefix contributed by a C++ configuration fragment. */ diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index a4b40895c560a2..79cb6503d6ee99 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -111,6 +111,7 @@ import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -2141,6 +2142,11 @@ public ImmutableSet getTreeArtifactsConflictingWithFiles() { public ActionKeyContext getActionKeyContext() { return actionKeyContext; } + + @Override + public RepositoryMapping getMainRepoMapping() { + throw new UnsupportedOperationException(); + } } protected Iterable baselineCoverageArtifactBasenames(ConfiguredTarget target) diff --git a/src/test/java/com/google/devtools/build/lib/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/starlark/BUILD index fa34fb594d8cd2..3b8307d09c1e9a 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/starlark/BUILD @@ -82,6 +82,7 @@ java_test( "//src/test/java/com/google/devtools/build/lib/actions/util", "//src/test/java/com/google/devtools/build/lib/analysis/testing", "//src/test/java/com/google/devtools/build/lib/analysis/util", + "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util", "//src/test/java/com/google/devtools/build/lib/exec/util", "//src/test/java/com/google/devtools/build/lib/rules/python:PythonTestUtils", "//src/test/java/com/google/devtools/build/lib/starlark/util", diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java index 94fc1fbe80b162..932805604df9da 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.createModuleKey; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; @@ -51,6 +52,7 @@ import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.events.Event; @@ -2690,6 +2692,156 @@ def _dep_impl(ctx): assertThat(e).hasMessageThat().contains("trying to mutate a frozen Args value"); } + @Test + public void testArgsMainRepoLabel() throws Exception { + StarlarkRuleContext ruleContext = createRuleContext("//foo:foo"); + setRuleContext(ruleContext); + ev.exec( + "actions = ruleContext.actions", + "a = []", + "a.append(actions.args().add(Label('//bar')))", + "a.append(actions.args().add('-flag', Label('//bar')))", + "a.append(actions.args().add('-flag', Label('//bar'), format = '_%s_'))", + "a.append(actions.args().add_all(['foo', Label('//bar')]))", + "a.append(actions.args().add_all(depset([Label('//foo'), Label('//bar')])))", + "ruleContext.actions.run(", + " inputs = depset(ruleContext.files.srcs),", + " outputs = ruleContext.files.srcs,", + " arguments = a,", + " executable = ruleContext.files.tools[0],", + " toolchain = None", + ")"); + SpawnAction action = + (SpawnAction) + Iterables.getOnlyElement( + ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); + assertThat(action.getArguments()) + .containsExactly( + "foo/t.exe", + "//bar:bar", + "-flag", + "//bar:bar", + "-flag", + "_//bar:bar_", + "foo", + "//bar:bar", + "//foo:foo", + "//bar:bar") + .inOrder(); + } + + @Test + public void testArgsCanonicalRepoLabel() throws Exception { + StarlarkRuleContext ruleContext = createRuleContext("//foo:foo"); + setRuleContext(ruleContext); + ev.exec( + "actions = ruleContext.actions", + "a = []", + "a.append(actions.args().add(Label('@@repo~//:foo')))", + "a.append(actions.args().add('-flag', Label('@@repo~//:foo')))", + "a.append(actions.args().add('-flag', Label('@@repo~//:foo'), format = '_%s_'))", + "a.append(actions.args().add_all(['foo', Label('@@repo~//:foo')]))", + "a.append(actions.args().add_all(depset([Label('@@other_repo~//:foo')," + + " Label('@@repo~//:foo')])))", + "ruleContext.actions.run(", + " inputs = depset(ruleContext.files.srcs),", + " outputs = ruleContext.files.srcs,", + " arguments = a,", + " executable = ruleContext.files.tools[0],", + " toolchain = None", + ")"); + SpawnAction action = + (SpawnAction) + Iterables.getOnlyElement( + ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); + assertThat(action.getArguments()) + .containsExactly( + "foo/t.exe", + "@@repo~//:foo", + "-flag", + "@@repo~//:foo", + "-flag", + "_@@repo~//:foo_", + "foo", + "@@repo~//:foo", + "@@other_repo~//:foo", + "@@repo~//:foo") + .inOrder(); + } + + @Test + public void testArgsApparentRepoLabel() throws Exception { + scratch.overwriteFile("MODULE.bazel", "bazel_dep(name = 'foo', version = '1.0')"); + registry.addModule(createModuleKey("foo", "1.0"), "module(name='foo', version='1.0')"); + invalidatePackages(); + + StarlarkRuleContext ruleContext = createRuleContext("//foo:foo"); + setRuleContext(ruleContext); + ev.exec( + "actions = ruleContext.actions", + "a = []", + "a.append(actions.args().add(Label('@@foo~//:foo')))", + "a.append(actions.args().add('-flag', Label('@@foo~//:foo')))", + "a.append(actions.args().add('-flag', Label('@@foo~//:foo'), format = '_%s_'))", + "a.append(actions.args().add_all(['foo', Label('@@foo~//:foo')]))", + "a.append(actions.args().add_all(depset([Label('@@repo~//:foo'), Label('@@foo~//:foo')])))", + "ruleContext.actions.run(", + " inputs = depset(ruleContext.files.srcs),", + " outputs = ruleContext.files.srcs,", + " arguments = a,", + " executable = ruleContext.files.tools[0],", + " toolchain = None", + ")"); + SpawnAction action = + (SpawnAction) + Iterables.getOnlyElement( + ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); + assertThat(action.getArguments()) + .containsExactly( + "foo/t.exe", + "@foo//:foo", + "-flag", + "@foo//:foo", + "-flag", + "_@foo//:foo_", + "foo", + "@foo//:foo", + "@@repo~//:foo", + "@foo//:foo") + .inOrder(); + } + + @Test + public void testArgsBuiltTwiceWithExternalLabel() throws Exception { + StarlarkRuleContext ruleContext = createRuleContext("//foo:foo"); + setRuleContext(ruleContext); + ev.exec( + "args = ruleContext.actions.args()", + "args.add(Label('@@foo'))", + "ruleContext.actions.run(", + " inputs = depset(ruleContext.files.srcs),", + " outputs = ruleContext.files.srcs,", + " arguments = [args],", + " executable = ruleContext.files.tools[0],", + " toolchain = None", + ")", + "ruleContext.actions.run(", + " inputs = depset(ruleContext.files.srcs),", + " outputs = ruleContext.files.srcs,", + " arguments = [args],", + " executable = ruleContext.files.tools[0],", + " toolchain = None", + ")"); + List actions = + ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions().stream() + .map(SpawnAction.class::cast) + .toList(); + assertThat(actions).hasSize(2); + assertThat(actions.getFirst().getArguments()) + .containsExactlyElementsIn(actions.getLast().getArguments()) + .inOrder(); + } + @Test public void testConfigurationField_starlarkSplitTransitionProhibited() throws Exception { scratch.overwriteFile( @@ -3365,7 +3517,7 @@ private String getDigest(CommandLine commandLine, ArtifactExpander artifactExpan private CommandLine getCommandLine(String... lines) throws Exception { ev.exec(lines); - return ((Args) ev.eval("args")).build(); + return ((Args) ev.eval("args")).build(() -> RepositoryMapping.ALWAYS_FALLBACK); } @Test @@ -3388,7 +3540,7 @@ public void testDirectoryInArgs() throws Exception { Sequence result = (Sequence) ev.eval("args, directory"); Args args = (Args) result.get(0); Artifact directory = (Artifact) result.get(1); - CommandLine commandLine = args.build(); + CommandLine commandLine = args.build(() -> RepositoryMapping.ALWAYS_FALLBACK); // When asking for arguments without an artifact expander we just return the directory assertThat(commandLine.arguments()).containsExactly("foo/dir"); @@ -3414,7 +3566,7 @@ public void testDirectoryInArgsExpandDirectories() throws Exception { Sequence result = (Sequence) ev.eval("args, directory"); Args args = (Args) result.get(0); Artifact directory = (Artifact) result.get(1); - CommandLine commandLine = args.build(); + CommandLine commandLine = args.build(() -> RepositoryMapping.ALWAYS_FALLBACK); Artifact file1 = getBinArtifactWithNoOwner("foo/dir/file1"); Artifact file2 = getBinArtifactWithNoOwner("foo/dir/file2"); @@ -3465,7 +3617,7 @@ public void testDirectoryExpansionInArgs() throws Exception { "args.add_all([directory, file3], map_each=_expand_dirs)"); Args args = (Args) ev.eval("args"); Artifact directory = (Artifact) ev.eval("directory"); - CommandLine commandLine = args.build(); + CommandLine commandLine = args.build(() -> RepositoryMapping.ALWAYS_FALLBACK); Artifact file1 = getBinArtifactWithNoOwner("foo/dir/file1"); Artifact file2 = getBinArtifactWithNoOwner("foo/dir/file2"); @@ -3485,7 +3637,7 @@ public void testCallDirectoryExpanderWithWrongType() throws Exception { " return dir_expander.expand('oh no a string')", "args.add_all([f], map_each=_expand_dirs)"); Args args = (Args) ev.eval("args"); - CommandLine commandLine = args.build(); + CommandLine commandLine = args.build(() -> RepositoryMapping.ALWAYS_FALLBACK); assertThrows(CommandLineExpansionException.class, commandLine::arguments); } diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index b0ec4eb348f54a..ce24e5e875f7f0 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -273,6 +273,9 @@ sh_test( "@bazel_tools//tools/bash/runfiles", ], exec_compatible_with = ["//:highcpu_machine"], + tags = [ + "requires-network", # For Bzlmod + ], ) JAVA_VERSIONS = ("11", "17", "21") @@ -302,6 +305,9 @@ JAVA_VERSIONS_COVERAGE = ("11", "17", "21") "@bazel_tools//tools/bash/runfiles", ], exec_compatible_with = ["//:highcpu_machine"], + tags = [ + "requires-network", # For Bzlmod + ], ) for java_version in JAVA_VERSIONS ] @@ -326,8 +332,11 @@ JAVA_VERSIONS_COVERAGE = ("11", "17", "21") ":test-deps", "@bazel_tools//tools/bash/runfiles", ], - # This test is only run by the java_tools binaries pipeline. - tags = ["manual"], + tags = [ + # This test is only run by the java_tools binaries pipeline. + "manual", + "requires-network", # For Bzlmod + ], ) for java_version in JAVA_VERSIONS ] diff --git a/src/test/shell/bazel/bazel_java_test.sh b/src/test/shell/bazel/bazel_java_test.sh index 192bdb212df1ef..a77f00874b2411 100755 --- a/src/test/shell/bazel/bazel_java_test.sh +++ b/src/test/shell/bazel/bazel_java_test.sh @@ -1958,4 +1958,44 @@ EOF >& $TEST_log || fail "build failed" } +function test_strict_deps_error_external_repo_starlark_action() { + cat << 'EOF' > MODULE.bazel +bazel_dep( + name = "lib_c", + repo_name = "c", +) +local_path_override( + module_name = "lib_c", + path = "lib_c", +) +EOF + + mkdir -p pkg + cat << 'EOF' > pkg/BUILD +java_library(name = "a", srcs = ["A.java"], deps = [":b"]) +java_library(name = "b", srcs = ["B.java"], deps = ["@c"]) +EOF + cat << 'EOF' > pkg/A.java +public class A extends B implements C {} +EOF + cat << 'EOF' > pkg/B.java +public class B implements C {} +EOF + + mkdir -p lib_c + cat << 'EOF' > lib_c/MODULE.bazel +module(name = "lib_c") +EOF + cat << 'EOF' > lib_c/BUILD +java_library(name = "c_pregen", srcs = ["C.java"]) +java_import(name = "c", jars = ["libc_pregen.jar"], visibility = ["//visibility:public"]) +EOF + cat << 'EOF' > lib_c/C.java +public interface C {} +EOF + + bazel build //pkg:a >& $TEST_log && fail "build should fail" + expect_log "buildozer 'add deps @c//:c' //pkg:a" +} + run_suite "Java integration tests"