diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java index cb8eefd224e9f8..c1486b852be811 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractCommandLine.java @@ -56,8 +56,7 @@ public void addToFingerprint( CoreOptions.OutputPathsMode outputPathsMode, Fingerprint fingerprint) throws CommandLineExpansionException, InterruptedException { - for (String s : - arguments(/* artifactExpander= */ null, PathMapper.forActionKey(outputPathsMode))) { + for (String s : arguments()) { fingerprint.addString(s); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 0b2131a3589007..f820edec64e4f4 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -313,7 +313,6 @@ java_library( "ArtifactRoot.java", "Artifacts.java", "PathMapper.java", - "PathMapperConstants.java", ], deps = [ ":action_lookup_data", @@ -323,7 +322,6 @@ java_library( ":commandline_item", ":package_roots", "//src/main/java/com/google/devtools/build/docgen/annot", - "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//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/collect/nestedset:fingerprint_cache", diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java index 51d58c34f30f21..edbb6f8c6cc3e6 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java @@ -14,10 +14,11 @@ package com.google.devtools.build.lib.actions; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn; import com.google.devtools.build.lib.actions.CommandLineItem.MapFn; -import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.starlarkbuildapi.FileRootApi; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.errorprone.annotations.CheckReturnValue; @@ -44,7 +45,7 @@ public interface PathMapper { * {@link #storeIn(StarlarkSemantics)}. */ static PathMapper loadFrom(StarlarkSemantics semantics) { - return semantics.get(PathMapperConstants.SEMANTICS_KEY); + return semantics.get(SEMANTICS_KEY); } /** @@ -64,11 +65,10 @@ static PathMapper loadFrom(StarlarkSemantics semantics) { default StarlarkSemantics storeIn(StarlarkSemantics semantics) { // This in particular covers the case where the semantics do not have a path mapper yet and this // is NOOP. - if (semantics.get(PathMapperConstants.SEMANTICS_KEY) == this) { + if (semantics.get(SEMANTICS_KEY) == this) { return semantics; } - return new StarlarkSemantics( - semantics.toBuilder().set(PathMapperConstants.SEMANTICS_KEY, this).build()) { + return new StarlarkSemantics(semantics.toBuilder().set(SEMANTICS_KEY, this).build()) { // The path mapper doesn't affect which fields or methods are available on any given Starlark // object; it just affects the behavior of certain methods on Artifact. We thus preserve the // original semantics as a cache key. Otherwise, even if PathMapper#equals returned true for @@ -82,13 +82,6 @@ public StarlarkSemantics getStarlarkClassDescriptorCacheKey() { }; } - /** Returns the instance to use during action key computation. */ - static PathMapper forActionKey(CoreOptions.OutputPathsMode outputPathsMode) { - return outputPathsMode == CoreOptions.OutputPathsMode.OFF - ? NOOP - : PathMapperConstants.FOR_FINGERPRINTING; - } - /** * Returns the exec path with the path mapping applied. * @@ -149,7 +142,7 @@ default FileRootApi mapRoot(Artifact artifact) { if (root.isSourceRoot()) { // Source roots' paths are never mapped, but we still need to wrap them in a // MappedArtifactRoot to ensure correct Starlark comparison behavior. - return PathMapperConstants.mappedSourceRoots.get(root); + return mappedSourceRoots.get(root); } // It would *not* be correct to just apply #map to the exec path of the root: The root part of // the mapped exec path of this artifact may depend on its complete exec path as well as on e.g. @@ -199,6 +192,15 @@ public FileRootApi mapRoot(Artifact artifact) { } }; + StarlarkSemantics.Key SEMANTICS_KEY = + new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP); + + // Not meant for use outside this interface. + LoadingCache mappedSourceRoots = + Caffeine.newBuilder() + .weakKeys() + .build(sourceRoot -> new MappedArtifactRoot(sourceRoot.getExecPath())); + /** A {@link FileRootApi} returned by {@link PathMapper#mapRoot(Artifact)}. */ @StarlarkBuiltin( name = "mapped_root", diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathMapperConstants.java b/src/main/java/com/google/devtools/build/lib/actions/PathMapperConstants.java deleted file mode 100644 index c45938f45db567..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/actions/PathMapperConstants.java +++ /dev/null @@ -1,81 +0,0 @@ -// Copyright 2025 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.actions; - -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.LoadingCache; -import com.google.devtools.build.lib.vfs.PathFragment; -import net.starlark.java.eval.StarlarkSemantics; - -/** Holder class for symbols used by the PathMapper interface that shouldn't be public. */ -final class PathMapperConstants { - - public static final StarlarkSemantics.Key SEMANTICS_KEY = - new StarlarkSemantics.Key<>("path_mapper", PathMapper.NOOP); - public static final LoadingCache mappedSourceRoots = - Caffeine.newBuilder() - .weakKeys() - .build(sourceRoot -> new PathMapper.MappedArtifactRoot(sourceRoot.getExecPath())); - - private static final PathFragment BAZEL_OUT = PathFragment.create("bazel-out"); - private static final PathFragment BLAZE_OUT = PathFragment.create("blaze-out"); - - /** - * A special instance for use in {@link AbstractAction#computeKey} when path mapping is generally - * enabled for an action. - * - *

When computing an action key, the following approaches to taking path mapping into account - * do not work: - * - *

    - *
  • Using the actual path mapper is prohibitive since constructing it requires checking for - * collisions among the action input's paths when computing the action key, which flattens - * the input depsets of all actions that opt into path mapping and also increases CPU usage. - *
  • Unconditionally using {@link - * com.google.devtools.build.lib.analysis.actions.StrippingPathMapper} can result in stale - * action keys when an action is opted out of path mapping at execution time due to input - * path collisions after stripping. See path_mapping_test for an example. - *
  • Using {@link PathMapper#NOOP} does not distinguish between map_each results built from - * strings and those built from {@link - * com.google.devtools.build.lib.starlarkbuildapi.FileApi#getExecPathStringForStarlark}. - * While the latter will be mapped at execution time, the former won't, resulting in the - * same digest for actions that behave differently at execution time. This is covered by - * tests in StarlarkRuleImplementationFunctionsTest. - *
- * - *

Instead, we use a special path mapping instance that preserves the equality relations - * between the original config segments, but prepends a fixed string to distinguish hard-coded - * path strings from mapped paths. This relies on actions using path mapping to be "root - * agnostic": they must not contain logic that depends on any particular (output) root path. - */ - static final PathMapper FOR_FINGERPRINTING = - execPath -> { - if (!execPath.startsWith(BAZEL_OUT) && !execPath.startsWith(BLAZE_OUT)) { - // This is not an output path. - return execPath; - } - String execPathString = execPath.getPathString(); - int startOfConfigSegment = execPathString.indexOf('/') + 1; - if (startOfConfigSegment == 0) { - return execPath; - } - return PathFragment.createAlreadyNormalized( - execPathString.substring(0, startOfConfigSegment) - + "pm-" - + execPathString.substring(startOfConfigSegment)); - }; - - private PathMapperConstants() {} -} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java index 4150e22c1847c0..f64be1edebc72c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java @@ -40,6 +40,49 @@ * PathMapper}). */ public final class PathMappers { + private static final PathFragment BAZEL_OUT = PathFragment.create("bazel-out"); + private static final PathFragment BLAZE_OUT = PathFragment.create("blaze-out"); + + /** + * A special instance for use in {@link AbstractAction#computeKey} when path mapping is generally + * enabled for an action. + * + *

When computing an action key, the following approaches to taking path mapping into account + * do not work: + * + *

    + *
  • Using the actual path mapper is prohibitive since constructing it requires checking for + * collisions among the action input's paths when computing the action key, which flattens + * the input depsets of all actions that opt into path mapping and also increases CPU usage. + *
  • Unconditionally using {@link StrippingPathMapper} can result in stale action keys when an + * action is opted out of path mapping at execution time due to input path collisions after + * stripping. See path_mapping_test for an example. + *
  • Using {@link PathMapper#NOOP} does not distinguish between map_each results built from + * strings and those built from {@link + * com.google.devtools.build.lib.starlarkbuildapi.FileApi#getExecPathStringForStarlark}. + * While the latter will be mapped at execution time, the former won't, resulting in the + * same digest for actions that behave differently at execution time. This is covered by + * tests in StarlarkRuleImplementationFunctionsTest. + *
+ * + *

Instead, we use a special path mapping instance that preserves the equality relations + * between the original config segments, but prepends a fixed string to distinguish hard-coded + * path strings from mapped paths. This relies on actions using path mapping to be "root + * agnostic": they must not contain logic that depends on any particular (output) root path. + */ + private static final PathMapper FOR_FINGERPRINTING = + execPath -> { + if (!execPath.startsWith(BAZEL_OUT) && !execPath.startsWith(BLAZE_OUT)) { + // This is not an output path. + return execPath; + } + String execPathString = execPath.getPathString(); + int startOfConfigSegment = execPathString.indexOf('/') + 1; + return PathFragment.createAlreadyNormalized( + execPathString.substring(0, startOfConfigSegment) + + "pm-" + + execPathString.substring(startOfConfigSegment)); + }; // TODO: Remove actions from this list by adding ExecutionRequirements.SUPPORTS_PATH_MAPPING to // their execution info instead. @@ -93,6 +136,13 @@ public static void addToFingerprint( } } + /** Returns the instance to use during action key computation. */ + public static PathMapper forActionKey(OutputPathsMode outputPathsMode) { + return outputPathsMode == OutputPathsMode.OFF + ? PathMapper.NOOP + : PathMappers.FOR_FINGERPRINTING; + } + /** * Actions that support path mapping should call this method when creating their {@link Spawn}. * 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 ccd86ecd214229..76d663da2a777c 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 @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.SingleStringArgFormatter; +import com.google.devtools.build.lib.analysis.actions.PathMappers; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryMapping; @@ -574,7 +575,7 @@ private int addToFingerprint( maybeExpandDirectories( artifactExpander, arguments.subList(argi, argi + count), - PathMapper.forActionKey(outputPathsMode)); + PathMappers.forActionKey(outputPathsMode)); argi += count; if (mapEach != null) { // TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase) @@ -589,7 +590,7 @@ private int addToFingerprint( fingerprint::addString, location, artifactExpander, - PathMapper.forActionKey(outputPathsMode), + PathMappers.forActionKey(outputPathsMode), starlarkSemantics); } else { for (Object value : maybeExpandedValues) { @@ -1204,7 +1205,7 @@ public void expandToCommandLine(Object object, Consumer args) args, location, artifactExpander, - PathMapper.forActionKey(outputPathsMode), + PathMappers.forActionKey(outputPathsMode), starlarkSemantics); } @@ -1214,7 +1215,7 @@ private List maybeExpandDirectory(Object object) throws CommandLineExpan } return VectorArg.expandDirectories( - artifactExpander, ImmutableList.of(object), PathMapper.forActionKey(outputPathsMode)); + artifactExpander, ImmutableList.of(object), PathMappers.forActionKey(outputPathsMode)); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java index 172c0a7d211085..5309498ddbfa22 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java @@ -105,40 +105,24 @@ public CcToolchainVariables getCcToolchainVariables(Dict buildVariables) CcToolchainVariables.Builder ccToolchainVariables = CcToolchainVariables.builder(); for (var entry : buildVariables.entrySet()) { - String key = (String) entry.getKey(); - Object value = entry.getValue(); - switch (value) { - case String s -> ccToolchainVariables.addStringVariable(key, s); - case Artifact a -> ccToolchainVariables.addArtifactVariable(key, a); - case Boolean b -> ccToolchainVariables.addBooleanValue(key, b); - case Iterable values -> { - if (key.equals("libraries_to_link")) { - SequenceBuilder sb = new SequenceBuilder(); - for (var v : (Iterable) values) { - sb.addValue(v); - } - ccToolchainVariables.addCustomBuiltVariable(key, sb); - } else { - ccToolchainVariables.addStringSequenceVariable(key, (Iterable) values); + if (entry.getValue() instanceof String) { + ccToolchainVariables.addStringVariable((String) entry.getKey(), (String) entry.getValue()); + } else if (entry.getValue() instanceof Boolean) { + ccToolchainVariables.addBooleanValue((String) entry.getKey(), (Boolean) entry.getValue()); + } else if (entry.getValue() instanceof Iterable) { + if (entry.getKey().equals("libraries_to_link")) { + SequenceBuilder sb = new SequenceBuilder(); + for (var value : (Iterable) entry.getValue()) { + sb.addValue((VariableValue) value); } + ccToolchainVariables.addCustomBuiltVariable((String) entry.getKey(), sb); + } else { + ccToolchainVariables.addStringSequenceVariable( + (String) entry.getKey(), (Iterable) entry.getValue()); } - case Depset depset -> { - Class type = depset.getElementClass(); - // Type doesn't matter for empty depsets. - if (type == String.class || type == null) { - ccToolchainVariables.addStringSequenceVariable(key, depset.getSet(String.class)); - } else if (type == Artifact.class) { - ccToolchainVariables.addArtifactSequenceVariable(key, depset.getSet(Artifact.class)); - } else if (type == PathFragment.class) { - ccToolchainVariables.addPathFragmentSequenceVariable( - key, depset.getSet(PathFragment.class)); - } else { - throw new IllegalStateException("Unexpected depset element type: %s".formatted(type)); - } - } - default -> - throw new IllegalStateException( - "Unexpected value: %s (%s)".formatted(value, value.getClass())); + } else if (entry.getValue() instanceof Depset) { + ccToolchainVariables.addStringSequenceVariable( + (String) entry.getKey(), ((Depset) entry.getValue()).getSet(String.class)); } } return ccToolchainVariables.build(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java index 4180604d1fc3c8..e1ff458481176b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java @@ -412,18 +412,18 @@ private boolean canBeExpanded( } if (expandIfTrue != null && (!variables.isAvailable(expandIfTrue, expander) - || !variables.getVariable(expandIfTrue, pathMapper).isTruthy())) { + || !variables.getVariable(expandIfTrue).isTruthy())) { return false; } if (expandIfFalse != null && (!variables.isAvailable(expandIfFalse, expander) - || variables.getVariable(expandIfFalse, pathMapper).isTruthy())) { + || variables.getVariable(expandIfFalse).isTruthy())) { return false; } if (expandIfEqual != null && (!variables.isAvailable(expandIfEqual.variable, expander) || !variables - .getVariable(expandIfEqual.variable, pathMapper) + .getVariable(expandIfEqual.variable) .getStringValue(expandIfEqual.variable, pathMapper) .equals(expandIfEqual.value))) { return false; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java index 3ed41d02038a38..ffc07540653d40 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.rules.cpp; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableList; @@ -179,7 +178,7 @@ public StringValueParser(String value) throws EvalException { parse(); } - /** Returns the parsed chunks for this string. */ + /** @return the parsed chunks for this string. */ public ImmutableList getChunks() { return chunks.build(); } @@ -311,15 +310,13 @@ public static ImmutableList toStringList( * @throws ExpansionException when no such variable or no such field are present, or when * accessing a field of non-structured variable */ - VariableValue getVariable(String name, PathMapper pathMapper) throws ExpansionException { - return lookupVariable( - name, /* throwOnMissingVariable= */ true, /* expander= */ null, pathMapper); + VariableValue getVariable(String name) throws ExpansionException { + return lookupVariable(name, /* throwOnMissingVariable= */ true, /* expander= */ null); } - private VariableValue getVariable( - String name, @Nullable ArtifactExpander expander, PathMapper pathMapper) + private VariableValue getVariable(String name, @Nullable ArtifactExpander expander) throws ExpansionException { - return lookupVariable(name, /* throwOnMissingVariable= */ true, expander, pathMapper); + return lookupVariable(name, /* throwOnMissingVariable= */ true, expander); } /** @@ -328,10 +325,7 @@ private VariableValue getVariable( */ @Nullable private VariableValue lookupVariable( - String name, - boolean throwOnMissingVariable, - @Nullable ArtifactExpander expander, - PathMapper pathMapper) + String name, boolean throwOnMissingVariable, @Nullable ArtifactExpander expander) throws ExpansionException { VariableValue var = getNonStructuredVariable(name); if (var != null) { @@ -354,8 +348,7 @@ private VariableValue lookupVariable( Object variableOrError = structuredVariableCache.get(name); if (variableOrError == null) { try { - VariableValue variable = - getStructureVariable(name, throwOnMissingVariable, expander, pathMapper); + VariableValue variable = getStructureVariable(name, throwOnMissingVariable, expander); variableOrError = variable != null ? variable : NULL_MARKER; } catch (ExpansionException e) { if (throwOnMissingVariable) { @@ -383,10 +376,7 @@ private VariableValue lookupVariable( @Nullable private VariableValue getStructureVariable( - String name, - boolean throwOnMissingVariable, - @Nullable ArtifactExpander expander, - PathMapper pathMapper) + String name, boolean throwOnMissingVariable, @Nullable ArtifactExpander expander) throws ExpansionException { if (!name.contains(".")) { return null; @@ -408,8 +398,7 @@ private VariableValue getStructureVariable( while (!fieldsToAccess.empty()) { String field = fieldsToAccess.pop(); - variable = - variable.getFieldValue(structPath, field, expander, pathMapper, throwOnMissingVariable); + variable = variable.getFieldValue(structPath, field, expander, throwOnMissingVariable); if (variable == null) { if (throwOnMissingVariable) { throw new ExpansionException( @@ -427,21 +416,19 @@ private VariableValue getStructureVariable( public String getStringVariable(String variableName, PathMapper pathMapper) throws ExpansionException { - return getVariable(variableName, /* expander= */ null, pathMapper) - .getStringValue(variableName, pathMapper); + return getVariable(variableName, /* expander= */ null).getStringValue(variableName, pathMapper); } public Iterable getSequenceVariable( String variableName, PathMapper pathMapper) throws ExpansionException { - return getVariable(variableName, /* expander= */ null, pathMapper) + return getVariable(variableName, /* expander= */ null) .getSequenceValue(variableName, pathMapper); } public Iterable getSequenceVariable( String variableName, @Nullable ArtifactExpander expander, PathMapper pathMapper) throws ExpansionException { - return getVariable(variableName, expander, pathMapper) - .getSequenceValue(variableName, pathMapper); + return getVariable(variableName, expander).getSequenceValue(variableName, pathMapper); } /** Returns whether {@code variable} is set. */ @@ -451,10 +438,7 @@ public boolean isAvailable(String variable) { boolean isAvailable(String variable, @Nullable ArtifactExpander expander) { try { - // Availability doesn't depend on the path mapper. - return lookupVariable( - variable, /* throwOnMissingVariable= */ false, expander, PathMapper.NOOP) - != null; + return lookupVariable(variable, /* throwOnMissingVariable= */ false, expander) != null; } catch (ExpansionException e) { throw new IllegalStateException( "Should not happen - call to lookupVariable threw when asked not to.", e); @@ -484,6 +468,7 @@ interface VariableValue { * StringValue), or throw exception if it cannot (e.g. Sequence). * * @param variableName name of the variable value at hand, for better exception message. + * @param pathMapper */ String getStringValue(String variableName, PathMapper pathMapper) throws ExpansionException; @@ -492,6 +477,7 @@ interface VariableValue { * (e.g. Sequence), or throw exception if it cannot (e.g. StringValue). * * @param variableName name of the variable value at hand, for better exception message. + * @param pathMapper */ Iterable getSequenceValue(String variableName, PathMapper pathMapper) throws ExpansionException; @@ -502,25 +488,15 @@ Iterable getSequenceValue(String variableName, PathMapp * * @param variableName name of the variable value at hand, for better exception message. */ + VariableValue getFieldValue(String variableName, String field) throws ExpansionException; + VariableValue getFieldValue( String variableName, String field, @Nullable ArtifactExpander expander, - PathMapper pathMapper, boolean throwOnMissingVariable) throws ExpansionException; - @VisibleForTesting - default VariableValue getFieldValue(String variableName, String field) - throws ExpansionException { - return getFieldValue( - variableName, - field, - /* expander= */ null, - PathMapper.NOOP, - /* throwOnMissingVariable= */ true); - } - /** Returns true if the variable is truthy */ boolean isTruthy(); } @@ -540,13 +516,19 @@ abstract static class VariableValueAdapter implements VariableValue { @Override public abstract boolean isTruthy(); + @Override + public VariableValue getFieldValue(String variableName, String field) + throws ExpansionException { + return getFieldValue( + variableName, field, /* expander= */ null, /* throwOnMissingVariable= */ true); + } + @Nullable @Override public VariableValue getFieldValue( String variableName, String field, @Nullable ArtifactExpander expander, - PathMapper pathMapper, boolean throwOnMissingVariable) throws ExpansionException { if (throwOnMissingVariable) { @@ -734,7 +716,6 @@ public VariableValue getFieldValue( String variableName, String field, @Nullable ArtifactExpander expander, - PathMapper pathMapper, boolean throwOnMissingVariable) { if (TYPE_FIELD_NAME.equals(field)) { return new StringValue(getTypeName()); @@ -789,17 +770,11 @@ public VariableValue getFieldValue( String variableName, String field, @Nullable ArtifactExpander expander, - PathMapper pathMapper, boolean throwOnMissingVariable) { if (NAME_FIELD_NAME.equals(field)) { - if (pathMapper.isNoop()) { - return new StringValue(name); - } - return new StringValue( - pathMapper.map(PathFragment.createAlreadyNormalized(name)).getPathString()); + return new StringValue(name); } - return super.getFieldValue( - variableName, field, expander, pathMapper, throwOnMissingVariable); + return super.getFieldValue(variableName, field, expander, throwOnMissingVariable); } @Override @@ -843,13 +818,11 @@ public VariableValue getFieldValue( String variableName, String field, @Nullable ArtifactExpander expander, - PathMapper pathMapper, boolean throwOnMissingVariable) { if (PATH_FIELD_NAME.equals(field)) { return new StringValue(path); } - return super.getFieldValue( - variableName, field, expander, pathMapper, throwOnMissingVariable); + return super.getFieldValue(variableName, field, expander, throwOnMissingVariable); } @Override @@ -942,7 +915,6 @@ public VariableValue getFieldValue( String variableName, String field, @Nullable ArtifactExpander expander, - PathMapper pathMapper, boolean throwOnMissingVariable) { if (NAME_FIELD_NAME.equals(field)) { return null; @@ -954,17 +926,15 @@ public VariableValue getFieldValue( if (objectFile.isTreeArtifact() && expander != null) { expandedObjectFiles.addAll( Collections2.transform( - expander.tryExpandTreeArtifact(objectFile), - pathMapper::getMappedExecPathString)); + expander.tryExpandTreeArtifact(objectFile), Artifact::getExecPathString)); } else { - expandedObjectFiles.add(pathMapper.getMappedExecPathString(objectFile)); + expandedObjectFiles.add(objectFile.getExecPathString()); } } return StringSequence.of(expandedObjectFiles.build()); } - return super.getFieldValue( - variableName, field, expander, pathMapper, throwOnMissingVariable); + return super.getFieldValue(variableName, field, expander, throwOnMissingVariable); } @Override @@ -1283,7 +1253,6 @@ public VariableValue getFieldValue( String variableName, String field, @Nullable ArtifactExpander expander, - PathMapper pathMapper, boolean throwOnMissingVariable) { return value.getOrDefault(field, null); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index 14c468a1f7f868..05ee07c6439f70 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -66,8 +66,7 @@ public final class CppLinkAction extends SpawnAction { LinkCommandLine linkCommandLine, ActionEnvironment env, ImmutableMap toolchainEnv, - ImmutableMap executionRequirements, - OutputPathsMode outputPathsMode) + ImmutableMap executionRequirements) throws EvalException { super( owner, @@ -80,7 +79,7 @@ public final class CppLinkAction extends SpawnAction { /* executionInfo= */ executionRequirements, /* progressMessage= */ progressMessage, /* mnemonic= */ mnemonic, - /* outputPathsMode= */ outputPathsMode); + /* outputPathsMode= */ OutputPathsMode.OFF); this.toolchainEnv = toolchainEnv; } @@ -91,7 +90,6 @@ public ImmutableMap getEffectiveEnvironment(Map Maps.newLinkedHashMapWithExpectedSize(getEnvironment().estimatedSize()); getEnvironment().resolve(result, clientEnv); - // TODO(fmeum): The environment is not path mapped. result.putAll(toolchainEnv); if (!getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_DARWIN)) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index b6d08848f02175..589ac39781ac83 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.actions.ParameterFile; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; -import com.google.devtools.build.lib.analysis.actions.PathMappers; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.PerLabelOptions; import com.google.devtools.build.lib.cmdline.Label; @@ -850,16 +849,16 @@ public CppLinkAction build() throws EvalException, InterruptedException { CcToolchainVariables.Builder buildVariables = LinkBuildVariables.setupLinkingVariables( - output, + output.getExecPathString(), SolibSymlinkAction.getDynamicLibrarySoname( output.getRootRelativePath(), /* preserveName= */ linkType != LinkTargetType.NODEPS_DYNAMIC_LIBRARY, linkActionConstruction.getContext().getConfiguration().getMnemonic()), - thinltoParamFile, + thinltoParamFile != null ? thinltoParamFile.getExecPathString() : null, toolchain, featureConfiguration, - toolchain.getInterfaceSoBuilder(), - interfaceOutput, + toolchain.getInterfaceSoBuilder().getExecPathString(), + interfaceOutput != null ? interfaceOutput.getExecPathString() : null, fdoContext); ImmutableList userLinkFlags = @@ -1121,8 +1120,7 @@ private CppLinkAction buildLinkAction( linkCommandLine, linkActionConstruction.getConfig().getActionEnvironment(), toolchainEnv, - ImmutableMap.copyOf(executionInfo), - PathMappers.getOutputPathsMode(linkActionConstruction.getConfig())); + ImmutableMap.copyOf(executionInfo)); } /** Returns the output of this action as a {@link LibraryInput} or null if it is an executable. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java index 5ad20353a7ffdd..1f8e775058fa2a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java @@ -17,7 +17,6 @@ import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.SequenceBuilder; @@ -159,24 +158,24 @@ public static CcToolchainVariables.Builder setupLtoIndexingVariables( } public static CcToolchainVariables.Builder setupLinkingVariables( - Artifact outputFile, + String outputFile, String runtimeSolibName, - Artifact thinltoParamFile, + String thinltoParamFile, CcToolchainProvider ccToolchainProvider, FeatureConfiguration featureConfiguration, - Artifact interfaceLibraryBuilder, - Artifact interfaceLibraryOutput, + String interfaceLibraryBuilder, + String interfaceLibraryOutput, FdoContext fdoContext) throws EvalException { CcToolchainVariables.Builder buildVariables = CcToolchainVariables.builder(); if (thinltoParamFile != null) { // This is a normal link action and we need to use param file created by lto-indexing. - buildVariables.addArtifactVariable( + buildVariables.addStringVariable( LinkBuildVariables.THINLTO_PARAM_FILE.getVariableName(), thinltoParamFile); } // output exec path - buildVariables.addArtifactVariable( + buildVariables.addStringVariable( LinkBuildVariables.OUTPUT_EXECPATH.getVariableName(), outputFile); buildVariables.addStringVariable( @@ -187,26 +186,25 @@ public static CcToolchainVariables.Builder setupLinkingVariables( && featureConfiguration.isEnabled(CppRuleClasses.PROPELLER_OPTIMIZE) && fdoContext.getPropellerOptimizeInputFile() != null && fdoContext.getPropellerOptimizeInputFile().getLdArtifact() != null) { - buildVariables.addArtifactVariable( + buildVariables.addStringVariable( LinkBuildVariables.PROPELLER_OPTIMIZE_LD_PATH.getVariableName(), - fdoContext.getPropellerOptimizeInputFile().getLdArtifact()); + fdoContext.getPropellerOptimizeInputFile().getLdArtifact().getExecPathString()); } boolean shouldGenerateInterfaceLibrary = outputFile != null && interfaceLibraryBuilder != null && interfaceLibraryOutput != null; - if (shouldGenerateInterfaceLibrary) { - buildVariables.addStringVariable(GENERATE_INTERFACE_LIBRARY.getVariableName(), "yes"); - buildVariables.addArtifactVariable( - INTERFACE_LIBRARY_BUILDER.getVariableName(), interfaceLibraryBuilder); - buildVariables.addArtifactVariable(INTERFACE_LIBRARY_INPUT.getVariableName(), outputFile); - buildVariables.addArtifactVariable( - INTERFACE_LIBRARY_OUTPUT.getVariableName(), interfaceLibraryOutput); - } else { - buildVariables.addStringVariable(GENERATE_INTERFACE_LIBRARY.getVariableName(), "no"); - buildVariables.addStringVariable(INTERFACE_LIBRARY_BUILDER.getVariableName(), "ignored"); - buildVariables.addStringVariable(INTERFACE_LIBRARY_INPUT.getVariableName(), "ignored"); - buildVariables.addStringVariable(INTERFACE_LIBRARY_OUTPUT.getVariableName(), "ignored"); - } + buildVariables.addStringVariable( + GENERATE_INTERFACE_LIBRARY.getVariableName(), + shouldGenerateInterfaceLibrary ? "yes" : "no"); + buildVariables.addStringVariable( + INTERFACE_LIBRARY_BUILDER.getVariableName(), + shouldGenerateInterfaceLibrary ? interfaceLibraryBuilder : "ignored"); + buildVariables.addStringVariable( + INTERFACE_LIBRARY_INPUT.getVariableName(), + shouldGenerateInterfaceLibrary ? outputFile : "ignored"); + buildVariables.addStringVariable( + INTERFACE_LIBRARY_OUTPUT.getVariableName(), + shouldGenerateInterfaceLibrary ? interfaceLibraryOutput : "ignored"); return buildVariables; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java index 028ca995c42cb5..d4efb821461dbc 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java @@ -89,8 +89,7 @@ public CcToolchainVariables getBuildVariables() { return this.variables; } - public ImmutableList getParamCommandLine( - @Nullable ArtifactExpander expander, PathMapper pathMapper) + public ImmutableList getParamCommandLine(@Nullable ArtifactExpander expander) throws CommandLineExpansionException { ImmutableList.Builder argv = ImmutableList.builder(); try { @@ -98,17 +97,17 @@ public ImmutableList getParamCommandLine( // Filter out linker_param_file String linkerParamFile = variables - .getVariable(LINKER_PARAM_FILE.getVariableName(), pathMapper) - .getStringValue(LINKER_PARAM_FILE.getVariableName(), pathMapper); + .getVariable(LINKER_PARAM_FILE.getVariableName()) + .getStringValue(LINKER_PARAM_FILE.getVariableName(), PathMapper.NOOP); argv.addAll( featureConfiguration - .getCommandLine(actionName, variables, expander, pathMapper) + .getCommandLine(actionName, variables, expander, PathMapper.NOOP) .stream() .filter(s -> !s.contains(linkerParamFile)) .collect(toImmutableList())); } else { argv.addAll( - featureConfiguration.getCommandLine(actionName, variables, expander, pathMapper)); + featureConfiguration.getCommandLine(actionName, variables, expander, PathMapper.NOOP)); } } catch (ExpansionException e) { throw new CommandLineExpansionException(e.getMessage()); @@ -154,13 +153,13 @@ ParamFileInfo getParamFileInfo() throws EvalException { @Override public List arguments() throws CommandLineExpansionException { - return arguments(null, PathMapper.NOOP); + return arguments(null, null); } @Override public List arguments(ArtifactExpander artifactExpander, PathMapper pathMapper) throws CommandLineExpansionException { - return getParamCommandLine(artifactExpander, pathMapper); + return getParamCommandLine(artifactExpander); } /** A builder for a {@link LinkCommandLine}. */ diff --git a/src/main/starlark/builtins_bzl/common/cc/link/cpp_link_action.bzl b/src/main/starlark/builtins_bzl/common/cc/link/cpp_link_action.bzl index 4743e49d4160cc..97be0fadcd5158 100644 --- a/src/main/starlark/builtins_bzl/common/cc/link/cpp_link_action.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/link/cpp_link_action.bzl @@ -196,14 +196,14 @@ def link_action( build_variables = variables_extensions | setup_linking_variables( cc_toolchain, feature_configuration, - output, + output.path, cc_internal.dynamic_library_soname( actions, output.short_path, link_type != NODEPS_DYNAMIC_LIBRARY, ), - interface_output, - thinlto_param_file, + interface_output.path if interface_output else None, + thinlto_param_file.path if thinlto_param_file else None, ) user_link_flags = linkopts + cc_toolchain._cpp_configuration.linkopts diff --git a/src/main/starlark/builtins_bzl/common/cc/link/link_build_variables.bzl b/src/main/starlark/builtins_bzl/common/cc/link/link_build_variables.bzl index 6721d153c55429..492d89a987d5d0 100644 --- a/src/main/starlark/builtins_bzl/common/cc/link/link_build_variables.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/link/link_build_variables.bzl @@ -284,10 +284,10 @@ def setup_linking_variables( Args: cc_toolchain: cc_toolchain for which we are creating build variables. feature_configuration: Feature configuration to be queried. - output_file: (File) Optional output file. Used also as an input to interface_library builder. + output_file: (str) Optional output file path. Used also as an input to interface_library builder. runtime_solib_name: (str) The name of the runtime solib symlink of the shared library. - interface_library_output: (File) Optional interface library to generate using the ifso builder tool. - thinlto_param_file: (File) Optional Thinlto param file consumed by the final link action. (Produced + interface_library_output: (str) Path where to generate interface library using the ifso builder tool. + thinlto_param_file: (str) Thinlto param file consumed by the final link action. (Produced by thin-lto indexing action) Returns: (dict[str, ?]) linking build variables @@ -309,13 +309,13 @@ def setup_linking_variables( feature_configuration.is_enabled("propeller_optimize") and fdo_context.propeller_optimize_info and fdo_context.propeller_optimize_info.ld_profile): - vars[LINK_BUILD_VARIABLES.PROPELLER_OPTIMIZE_LD_PATH] = fdo_context.propeller_optimize_info.ld_profile + vars[LINK_BUILD_VARIABLES.PROPELLER_OPTIMIZE_LD_PATH] = fdo_context.propeller_optimize_info.ld_profile.path # ifso variables - should_generate_interface_library = output_file and cc_toolchain._if_so_builder and interface_library_output + should_generate_interface_library = output_file and cc_toolchain._if_so_builder.path and interface_library_output if should_generate_interface_library: vars[LINK_BUILD_VARIABLES.GENERATE_INTERFACE_LIBRARY] = "yes" - vars[LINK_BUILD_VARIABLES.INTERFACE_LIBRARY_BUILDER] = cc_toolchain._if_so_builder + vars[LINK_BUILD_VARIABLES.INTERFACE_LIBRARY_BUILDER] = cc_toolchain._if_so_builder.path vars[LINK_BUILD_VARIABLES.INTERFACE_LIBRARY_INPUT] = output_file vars[LINK_BUILD_VARIABLES.INTERFACE_LIBRARY_OUTPUT] = interface_library_output else: diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/PathMappersTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/PathMappersTest.java index cbe5ae9c2c0ca5..a58f05555eccbc 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/PathMappersTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/PathMappersTest.java @@ -19,7 +19,6 @@ import static java.lang.String.format; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; @@ -299,7 +298,7 @@ def my_rule_impl(ctx): @Test public void forActionKey() { - var pathMapper = PathMapper.forActionKey(CoreOptions.OutputPathsMode.STRIP); + var pathMapper = PathMappers.forActionKey(CoreOptions.OutputPathsMode.STRIP); assertThat(pathMapper.isNoop()).isFalse(); assertThat(pathMapper.map(PathFragment.create("pkg/file"))) .isEqualTo(PathFragment.create("pkg/file")); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java index 2f1b3d6a45a874..b3d680a5582b6d 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java @@ -780,10 +780,7 @@ public void testExpandIfTrueExpandsIfZero() throws Exception { } private static VariableValue booleanValue(boolean val) throws ExpansionException { - return CcToolchainVariables.builder() - .addBooleanValue("name", val) - .build() - .getVariable("name", PathMapper.NOOP); + return CcToolchainVariables.builder().addBooleanValue("name", val).build().getVariable("name"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java index 0e72deec4374f2..c89a1edf097b8c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibraryToLinkValueTest.java @@ -109,7 +109,6 @@ public void getFieldValue_forDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("dynamic_library"); @@ -119,7 +118,6 @@ public void getFieldValue_forDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("0"); @@ -129,7 +127,6 @@ public void getFieldValue_forDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo"); @@ -138,7 +135,6 @@ public void getFieldValue_forDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); } @@ -153,7 +149,6 @@ public void getFieldValue_forVersionedDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("versioned_dynamic_library"); @@ -163,7 +158,6 @@ public void getFieldValue_forVersionedDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "path", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo/bar.so"); @@ -173,7 +167,6 @@ public void getFieldValue_forVersionedDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("0"); @@ -183,7 +176,6 @@ public void getFieldValue_forVersionedDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo"); @@ -192,7 +184,6 @@ public void getFieldValue_forVersionedDynamicLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); } @@ -206,7 +197,6 @@ public void getFieldValue_forInterfaceLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("interface_library"); @@ -216,7 +206,6 @@ public void getFieldValue_forInterfaceLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("0"); @@ -226,7 +215,6 @@ public void getFieldValue_forInterfaceLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo"); @@ -235,7 +223,6 @@ public void getFieldValue_forInterfaceLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); } @@ -250,7 +237,6 @@ public void getFieldValue_forStaticLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("static_library"); @@ -260,7 +246,6 @@ public void getFieldValue_forStaticLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("0"); @@ -270,7 +255,6 @@ public void getFieldValue_forStaticLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo"); @@ -279,7 +263,6 @@ public void getFieldValue_forStaticLibrary() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); } @@ -294,7 +277,6 @@ public void getFieldValue_forStaticLibrary_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("static_library"); @@ -304,7 +286,6 @@ public void getFieldValue_forStaticLibrary_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("1"); @@ -314,7 +295,6 @@ public void getFieldValue_forStaticLibrary_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo"); @@ -323,7 +303,6 @@ public void getFieldValue_forStaticLibrary_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); } @@ -338,7 +317,6 @@ public void getFieldValue_forObjectFile() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("object_file"); @@ -348,7 +326,6 @@ public void getFieldValue_forObjectFile() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("0"); @@ -358,7 +335,6 @@ public void getFieldValue_forObjectFile() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo"); @@ -367,7 +343,6 @@ public void getFieldValue_forObjectFile() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); } @@ -382,7 +357,6 @@ public void getFieldValue_forObjectFile_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("object_file"); @@ -392,7 +366,6 @@ public void getFieldValue_forObjectFile_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("1"); @@ -402,7 +375,6 @@ public void getFieldValue_forObjectFile_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("foo"); @@ -411,7 +383,6 @@ public void getFieldValue_forObjectFile_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); } @@ -434,7 +405,6 @@ public void getFieldValue_forObjectFileGroup() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("object_file_group"); @@ -444,7 +414,6 @@ public void getFieldValue_forObjectFileGroup() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("0"); @@ -453,7 +422,6 @@ public void getFieldValue_forObjectFileGroup() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); assertThat( @@ -463,7 +431,6 @@ public void getFieldValue_forObjectFileGroup() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getSequenceValue("variable name doesn't matter", PathMapper.NOOP)) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) @@ -488,7 +455,6 @@ public void getFieldValue_forObjectFileGroup_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "type", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("object_file_group"); @@ -498,7 +464,6 @@ public void getFieldValue_forObjectFileGroup_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "is_whole_archive", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) .isEqualTo("1"); @@ -507,7 +472,6 @@ public void getFieldValue_forObjectFileGroup_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "name", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false)) .isNull(); assertThat( @@ -517,7 +481,6 @@ public void getFieldValue_forObjectFileGroup_isWholeArchive() throws Exception { /* variableName= */ "variable name doesn't matter", /* field= */ "object_files", /* expander= */ null, - PathMapper.NOOP, /* throwOnMissingVariable= */ false) .getSequenceValue("variable name doesn't matter", PathMapper.NOOP)) .getStringValue("variable name doesn't matter", PathMapper.NOOP)) diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java index e2ad7123207e88..ababd1fe4546c7 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java @@ -92,8 +92,7 @@ public void testLibrariesToLinkAreExported() throws Exception { CcToolchainVariables variables = getLinkBuildVariables(target, LinkTargetType.NODEPS_DYNAMIC_LIBRARY); VariableValue librariesToLinkSequence = - variables.getVariable( - LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); + variables.getVariable(LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName()); assertThat(librariesToLinkSequence).isNotNull(); Iterable librariesToLink = librariesToLinkSequence.getSequenceValue( @@ -139,8 +138,7 @@ public void testLinkSimpleLibName() throws Exception { CcToolchainVariables variables = getLinkBuildVariables(target, LinkTargetType.EXECUTABLE); VariableValue librariesToLinkSequence = - variables.getVariable( - LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); + variables.getVariable(LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName()); Iterable librariestoLink = librariesToLinkSequence.getSequenceValue( LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); @@ -167,8 +165,7 @@ public void testLinkVersionedLibName() throws Exception { CcToolchainVariables variables = getLinkBuildVariables(target, LinkTargetType.EXECUTABLE); VariableValue librariesToLinkSequence = - variables.getVariable( - LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); + variables.getVariable(LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName()); Iterable librariestoLink = librariesToLinkSequence.getSequenceValue( LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); @@ -195,8 +192,7 @@ public void testLinkUnusualLibName() throws Exception { CcToolchainVariables variables = getLinkBuildVariables(target, LinkTargetType.EXECUTABLE); VariableValue librariesToLinkSequence = - variables.getVariable( - LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); + variables.getVariable(LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName()); Iterable librariestoLink = librariesToLinkSequence.getSequenceValue( LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); @@ -470,9 +466,7 @@ public void testIsCcTestLinkActionBuildVariable() throws Exception { getLinkBuildVariables(testTarget, LinkTargetType.EXECUTABLE); assertThat( - testVariables - .getVariable(LinkBuildVariables.IS_CC_TEST.getVariableName(), PathMapper.NOOP) - .isTruthy()) + testVariables.getVariable(LinkBuildVariables.IS_CC_TEST.getVariableName()).isTruthy()) .isTrue(); ConfiguredTarget binaryTarget = getConfiguredTarget("//x:foo"); @@ -480,9 +474,7 @@ public void testIsCcTestLinkActionBuildVariable() throws Exception { getLinkBuildVariables(binaryTarget, LinkTargetType.EXECUTABLE); assertThat( - binaryVariables - .getVariable(LinkBuildVariables.IS_CC_TEST.getVariableName(), PathMapper.NOOP) - .isTruthy()) + binaryVariables.getVariable(LinkBuildVariables.IS_CC_TEST.getVariableName()).isTruthy()) .isFalse(); } @@ -513,9 +505,7 @@ public void testDisableIsCcTestLinkActionBuildVariable() throws Exception { assertThrows( ExpansionException.class, - () -> - testVariables.getVariable( - LinkBuildVariables.IS_CC_TEST.getVariableName(), PathMapper.NOOP)); + () -> testVariables.getVariable(LinkBuildVariables.IS_CC_TEST.getVariableName())); ConfiguredTarget binaryTarget = getConfiguredTarget("//x:foo"); CcToolchainVariables binaryVariables = @@ -523,9 +513,7 @@ public void testDisableIsCcTestLinkActionBuildVariable() throws Exception { assertThrows( ExpansionException.class, - () -> - binaryVariables.getVariable( - LinkBuildVariables.IS_CC_TEST.getVariableName(), PathMapper.NOOP)); + () -> binaryVariables.getVariable(LinkBuildVariables.IS_CC_TEST.getVariableName())); } @Test @@ -669,8 +657,7 @@ public void testLinkerInputsOverrideWholeArchive() throws Exception { getLinkBuildVariables(testTarget, LinkTargetType.DYNAMIC_LIBRARY); VariableValue librariesToLinkSequence = - testVariables.getVariable( - LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName(), PathMapper.NOOP); + testVariables.getVariable(LinkBuildVariables.LIBRARIES_TO_LINK.getVariableName()); assertThat(librariesToLinkSequence).isNotNull(); Iterable librariesToLink = librariesToLinkSequence.getSequenceValue( diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java index c215a93ae2bcbf..b3d799fd77ca65 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java @@ -272,7 +272,7 @@ public void testSplitStaticLinkCommand() throws Exception { assertThat(linkConfig.getCommandLines().unpack().get(0).commandLine.arguments()) .containsExactly("foo/bar/ar"); assertThat(linkConfig.getCommandLines().unpack().get(1).paramFileInfo.always()).isTrue(); - assertThat(linkConfig.getParamCommandLine(null, PathMapper.NOOP)) + assertThat(linkConfig.getParamCommandLine(null)) .containsExactly("rcsD", "a/FakeOutput") .inOrder(); } @@ -295,7 +295,7 @@ public void testSplitDynamicLinkCommand() throws Exception { .build(); assertThat(linkConfig.getCommandLines().unpack().get(0).commandLine.arguments()) .containsExactly("foo/bar/linker"); - assertThat(linkConfig.getParamCommandLine(null, PathMapper.NOOP)) + assertThat(linkConfig.getParamCommandLine(null)) .containsExactly("-shared", "-o", "a/FakeOutput", "") .inOrder(); } @@ -339,7 +339,7 @@ public void testSplitAlwaysLinkLinkCommand() throws Exception { assertThat(linkConfig.getCommandLines().unpack().get(0).commandLine.arguments()) .containsExactly("foo/bar/ar"); - assertThat(linkConfig.getParamCommandLine(null, PathMapper.NOOP)) + assertThat(linkConfig.getParamCommandLine(null)) .containsExactly("rcsD", "a/FakeOutput", "foo.o", "bar.o") .inOrder(); } @@ -398,9 +398,7 @@ public void testTreeArtifactLink() throws Exception { linkConfig.arguments(null, PathMapper.NOOP), treeArtifactsPaths, treeFileArtifactsPaths); verifyArguments(linkConfig.arguments(), treeArtifactsPaths, treeFileArtifactsPaths); verifyArguments( - linkConfig.getParamCommandLine(null, PathMapper.NOOP), - treeArtifactsPaths, - treeFileArtifactsPaths); + linkConfig.getParamCommandLine(null), treeArtifactsPaths, treeFileArtifactsPaths); // Should only reference tree file artifacts. verifyArguments( @@ -408,12 +406,8 @@ public void testTreeArtifactLink() throws Exception { treeFileArtifactsPaths, treeArtifactsPaths); verifyArguments( - linkConfig.arguments(expander, PathMapper.NOOP), - treeFileArtifactsPaths, - treeArtifactsPaths); + linkConfig.arguments(expander, null), treeFileArtifactsPaths, treeArtifactsPaths); verifyArguments( - linkConfig.getParamCommandLine(expander, PathMapper.NOOP), - treeFileArtifactsPaths, - treeArtifactsPaths); + linkConfig.getParamCommandLine(expander), treeFileArtifactsPaths, treeArtifactsPaths); } } diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 36fb6c5e066d4e..b7189dd50c532c 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -1392,7 +1392,6 @@ sh_test( data = [ ":test-deps", "//src/tools/remote:worker", - "@local_jdk//:jdk", # for remote_helpers ], tags = [ "no_windows", diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index 0d046f24b8e9a4..d0535f55550aa5 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -736,7 +736,7 @@ EOF bazel run \ --verbose_failures \ --experimental_output_paths=strip \ - --modify_execution_info=CppCompile=+supports-path-mapping,CppModuleMap=+supports-path-mapping,CppArchive=+supports-path-mapping \ + --modify_execution_info=CppCompile=+supports-path-mapping,CppModuleMap=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=layering_check \ "//$pkg:main" &>"$TEST_log" || fail "Expected success" @@ -750,10 +750,9 @@ EOF bazel run \ --verbose_failures \ --experimental_output_paths=strip \ - --modify_execution_info=CppCompile=+supports-path-mapping,CppModuleMap=+supports-path-mapping,CppArchive=+supports-path-mapping \ + --modify_execution_info=CppCompile=+supports-path-mapping,CppModuleMap=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=layering_check \ - -s \ "//$pkg:transitioned_main" &>"$TEST_log" || fail "Expected success" expect_log 'Hi there, lib1!' @@ -761,22 +760,8 @@ EOF expect_log 'Hello, TreeArtifact!' expect_log '42 43' # Compilation actions for lib1, lib2 and main should result in cache hits due - # to path stripping, utils is legitimately different and should not (4 cached - # out of 5 total). - # Likewise, link actions for lib1 and lib2 should result in cache hits, but - # the one for utils does not and the linking action for main doesn't support - # path mapping (2 cached out of 4 in total). In CI, the C++ toolchain on Linux - # uses --start-lib/--end-lib linker support to avoid the CppArchive actions - # entirely (0 cached out of 1 in total). - # The two custom actions and the four genrule actions are not path-mapped - # (0 cached out of 6 in total). - if is_darwin; then - expect_log ' 6 remote cache hit' - expect_log ' 9 remote' - else - expect_log ' 4 remote cache hit' - expect_log ' 8 remote' - fi + # to path stripping, utils is legitimately different and should not. + expect_log ' 4 remote cache hit' } function test_path_stripping_action_key_not_stale_for_path_collision() {