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 12bab8df8168a8..7fba9c3c74720c 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 @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.MiddlemanFactory; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey; 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; @@ -176,4 +177,9 @@ ImmutableList getBuildInfo( 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 837c287f8d3cfa..3a6be604719a0e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -414,6 +414,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", @@ -424,6 +425,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", @@ -2420,9 +2422,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 d9c9d5d171ffbb..6e50ea6fb989f8 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 @@ -30,10 +30,13 @@ import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoCollection; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +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.BuildInfoCollectionValue; +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; @@ -385,6 +388,19 @@ public ImmutableList getBuildInfo( return stamp ? collection.getStampedBuildInfo() : collection.getRedactedBuildInfo(); } + @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 a063d8381a6ab9..f85578805dd14b 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 @@ -24,10 +24,13 @@ import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.SingleStringArgFormatter; import com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine.ScalarArg; +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; @@ -71,7 +74,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) { @@ -103,7 +107,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. @@ -158,7 +163,7 @@ public ImmutableSet getDirectoryArtifacts() { } @Override - public CommandLine build() { + public CommandLine build(InterruptibleSupplier mainRepoMappingSupplier) { return commandLine; } @@ -260,6 +265,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; @@ -308,6 +319,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; + } addScalarArg(value, format != Starlark.NONE ? (String) format : null); return this; } @@ -437,8 +451,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; @@ -452,8 +470,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); } @@ -575,8 +601,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 @@ -587,18 +615,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 d00394c5a815f4..192dc70c83fedf 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 @@ -55,6 +55,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; @@ -72,6 +73,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; @@ -362,7 +364,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(); @@ -377,7 +380,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()); @@ -403,7 +406,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); @@ -412,7 +415,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"); @@ -599,7 +602,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); @@ -607,7 +610,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' @@ -671,8 +674,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 { List stringArgs = new ArrayList<>(); for (Object value : argumentsList) { if (value instanceof String) { @@ -684,7 +690,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", @@ -1094,6 +1100,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 2ae61bdb1293b5..1da07e10e35d07 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 @@ -18,6 +18,9 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; 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.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.Artifact; @@ -33,6 +36,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; @@ -181,7 +186,8 @@ private int eval( int argi, List builder, @Nullable ArtifactExpander artifactExpander, - PathMapper pathMapper) + PathMapper pathMapper, + @Nullable RepositoryMapping mainRepoMapping) throws CommandLineExpansionException, InterruptedException { final Location location = ((features & HAS_LOCATION) != 0) ? (Location) arguments.get(argi++) : null; @@ -215,7 +221,7 @@ private int eval( 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 @@ -614,9 +620,14 @@ private static void push(List arguments, Builder arg) { } private int eval( - List arguments, int argi, List builder, PathMapper pathMapper) { + List arguments, + int argi, + List builder, + PathMapper pathMapper, + @Nullable RepositoryMapping mainRepoMapping) { Object object = arguments.get(argi++); - String stringValue = StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper); + String stringValue = + StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper, mainRepoMapping); if (hasFormat) { String formatStr = (String) arguments.get(argi++); stringValue = SingleStringArgFormatter.format(formatStr, stringValue); @@ -704,8 +715,15 @@ Builder add(ScalarArg.Builder scalarArg) { return this; } - StarlarkCustomCommandLine build(boolean flagPerLine) { - Object[] args = arguments.toArray(); + StarlarkCustomCommandLine build( + boolean flagPerLine, @Nullable RepositoryMapping mainRepoMapping) { + 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); @@ -739,21 +757,40 @@ public Iterable arguments( List result = new ArrayList<>(); 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).eval(arguments, argi, result, artifactExpander, pathMapper); + argi = + ((VectorArg) arg) + .eval(arguments, argi, result, artifactExpander, pathMapper, mainRepoMapping); } else if (arg instanceof ScalarArg) { - argi = ((ScalarArg) arg).eval(arguments, argi, result, pathMapper); + argi = ((ScalarArg) arg).eval(arguments, argi, result, pathMapper, mainRepoMapping); } else { - result.add(expandToCommandLine(arg, pathMapper)); + result.add(expandToCommandLine(arg, pathMapper, mainRepoMapping)); } } return ImmutableList.copyOf(pathMapper.mapCustomStarlarkArgs(result)); } - 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. @@ -792,7 +829,17 @@ public Iterable arguments( Iterator startIndexIterator = argStartIndexes.iterator(); int nextStartIndex = startIndexIterator.hasNext() ? startIndexIterator.next() : -1; - for (int argi = 0; argi < 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; ) { // If we're grouping arguments, record the actual beginning of each group if (argi == nextStartIndex) { @@ -802,11 +849,14 @@ public Iterable arguments( Object arg = arguments.get(argi++); if (arg instanceof VectorArg) { - argi = ((VectorArg) arg).eval(arguments, argi, result, artifactExpander, pathMapper); + argi = + ((VectorArg) arg) + .eval(arguments, argi, result, artifactExpander, pathMapper, mainRepoMapping); } else if (arg instanceof ScalarArg) { - argi = ((ScalarArg) arg).eval(arguments, argi, result, pathMapper); + argi = ((ScalarArg) arg).eval(arguments, argi, result, pathMapper, mainRepoMapping); } else { - result.add(StarlarkCustomCommandLine.expandToCommandLine(arg, pathMapper)); + result.add( + StarlarkCustomCommandLine.expandToCommandLine(arg, pathMapper, mainRepoMapping)); } } @@ -842,7 +892,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 f697aa45630654..60e53fc27c43ea 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", @@ -554,7 +555,7 @@ void run( Object shadowedAction, Object resourceSetUnchecked, Object toolchainUnchecked) - throws EvalException; + throws EvalException, InterruptedException; @StarlarkMethod( name = "run_shell", @@ -807,7 +808,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 e53c7bc4e5b0e2..f515027753c836 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 @@ -35,6 +35,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; @@ -218,7 +219,7 @@ private static StarlarkCustomCommandLine createCustomCommandLine( VectorArg.Builder vectorArgBuilder) { return new StarlarkCustomCommandLine.Builder(StarlarkSemantics.DEFAULT) .add(vectorArgBuilder) - .build(/*flagPerLine=*/ false); + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); } private static ArtifactExpander createArtifactExpander( 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 a892023d0a4e31..3d96e98208a175 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 @@ -50,6 +50,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; @@ -233,6 +234,11 @@ public ImmutableSet getTreeArtifactsConflictingWithFiles() { public ActionKeyContext getActionKeyContext() { return original.getActionKeyContext(); } + + @Override + public RepositoryMapping getMainRepoMapping() throws InterruptedException { + return original.getMainRepoMapping(); + } } /** A dummy WorkspaceStatusAction. */ @@ -469,6 +475,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 f3d92c2f087341..0c7fec28d120c4 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 @@ -114,6 +114,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; @@ -2326,6 +2327,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 7a887cabfdd05f..ffa40d40d9c13a 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/starlark/BUILD @@ -83,6 +83,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/starlark/util", "//src/test/java/com/google/devtools/build/lib/testutil", 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 647342aedf3657..32063ff0284167 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; @@ -53,6 +54,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; @@ -2574,6 +2576,156 @@ public void testLazyArgsObjectImmutability() throws Exception { 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( @@ -3197,7 +3349,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 @@ -3220,7 +3372,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"); @@ -3246,7 +3398,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"); @@ -3297,7 +3449,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"); @@ -3317,7 +3469,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 fef7cccad28b87..7517f98b2d7d79 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -263,6 +263,9 @@ sh_test( "@bazel_tools//tools/bash/runfiles", ], exec_compatible_with = ["//:highcpu_machine"], + tags = [ + "requires-network", # For Bzlmod + ], ) JAVA_VERSIONS = ("11", "17", "21") @@ -292,6 +295,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 ] @@ -316,8 +322,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 92db25e7c9fa02..6d5a1e339d0b1a 100755 --- a/src/test/shell/bazel/bazel_java_test.sh +++ b/src/test/shell/bazel/bazel_java_test.sh @@ -1981,4 +1981,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"