Skip to content

Commit

Permalink
Support layering_check with C++ path mapping
Browse files Browse the repository at this point in the history
Users can opt into path mapping for C++ module map actions via `--modify_execution_info=CppModuleMap=+supports-path-mapping`. This is achieved by mapping paths in the module map files as well as converting the sequence variable for module map paths to a new structured `ArtifactSequenceVariable`.

Also makes it so that `ExecutionServer` gracefully handles failing commands instead of crashing.

Closes #22957.

PiperOrigin-RevId: 675073116
Change-Id: I13835c7fb01354a89ec5fd141cf892c6b733efe4
  • Loading branch information
fmeum authored and copybara-github committed Sep 16, 2024
1 parent 164705b commit d41b5f8
Show file tree
Hide file tree
Showing 9 changed files with 191 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,8 @@ void addAdditionalInputs(NestedSetBuilder<Artifact> builder) {
}
}

/** @return modules maps from direct dependencies. */
public Iterable<Artifact> getDirectModuleMaps() {
/** Returns modules maps from direct dependencies. */
public ImmutableList<Artifact> getDirectModuleMaps() {
return directModuleMaps;
}

Expand All @@ -588,8 +588,8 @@ DerivedArtifact getSeparateHeaderModule(boolean usePic) {
}

/**
* @return all declared headers of the current module if the current target is compiled as a
* module.
* Returns all declared headers of the current module if the current target is compiled as a
* module.
*/
ImmutableList<Artifact> getHeaderModuleSrcs(boolean separateModule) {
if (separateModule) {
Expand Down Expand Up @@ -644,7 +644,7 @@ public static CcCompilationContext createWithExtraHeaderTokens(
headerTokens.build());
}

/** @return the C++ module map of the owner. */
/** Returns the C++ module map of the owner. */
public CppModuleMap getCppModuleMap() {
return cppModuleMap;
}
Expand Down Expand Up @@ -1147,7 +1147,7 @@ public ImmutableList<E> getMergedResult() {
* Gathers data about the PIC and no-PIC .pcm files belonging to this context and the associated
* information about the headers, e.g. modular vs. textual headers and pre-grepped header files.
*
* <p>This also implements a data structure very similar to NestedSet, but chosing slightly
* <p>This also implements a data structure very similar to NestedSet, but choosing slightly
* different trade-offs to account for the specific data stored in here, specifically, we know
* that there is going to be a single entry in every node of the DAG. Contrary to NestedSet, we
* reuse memoization data from dependencies to conserve both runtime and memory. Experiments have
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.devtools.build.lib.analysis.LicensesProvider.TargetLicense;
import com.google.devtools.build.lib.analysis.LicensesProviderImpl;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.actions.PathMappers;
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.starlark.Args;
Expand Down Expand Up @@ -419,7 +420,11 @@ public void createModuleMapAction(
compiledModule,
moduleMapHomeIsCwd,
generateSubmodules,
withoutExternDependencies));
withoutExternDependencies,
PathMappers.getOutputPathsMode(ruleContext.getConfiguration()),
ruleContext
.getConfiguration()
.modifiedExecutionInfo(ImmutableMap.of(), CppModuleMapAction.MNEMONIC)));
}

@SerializationConstant @VisibleForSerialization
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,54 @@ public int hashCode() {
}
}

@Immutable
private static final class ArtifactSetSequence extends VariableValueAdapter {
private final NestedSet<Artifact> values;

private ArtifactSetSequence(NestedSet<Artifact> values) {
Preconditions.checkNotNull(values);
this.values = values;
}

@Override
public ImmutableList<VariableValue> getSequenceValue(
String variableName, PathMapper pathMapper) {
ImmutableList<Artifact> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (Artifact value : valuesList) {
sequences.add(new StringValue(pathMapper.getMappedExecPathString(value)));
}
return sequences.build();
}

@Override
public String getVariableTypeName() {
return Sequence.SEQUENCE_VARIABLE_TYPE_NAME;
}

@Override
public boolean isTruthy() {
return !values.isEmpty();
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (!(other instanceof ArtifactSetSequence otherArtifacts)) {
return false;
}
return values.shallowEquals(otherArtifacts.values);
}

@Override
public int hashCode() {
return values.shallowHashCode();
}
}

/**
* Single structure value. Be careful not to create sequences of single structures, as the memory
* overhead is prohibitively big.
Expand Down Expand Up @@ -1496,6 +1544,19 @@ public Builder addPathFragmentSequenceVariable(String name, NestedSet<PathFragme
return this;
}

/**
* Add a sequence variable that expands {@code name} to {@link Artifact} {@code values}.
*
* <p>Accepts values as NestedSet. Nested set is stored directly, not cloned, not flattened.
*/
@CanIgnoreReturnValue
public Builder addArtifactSequenceVariable(String name, NestedSet<Artifact> values) {
checkVariableNotPresentAlready(name);
Preconditions.checkNotNull(values, "Cannot set null as a value for variable '%s'", name);
variablesMap.put(name, new ArtifactSetSequence(values));
return this;
}

/**
* Add a variable built using {@code VariableValueBuilder} api that expands {@code name} to the
* value returned by the {@code builder}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
Artifact diagnosticsFile,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
Expand Down Expand Up @@ -229,7 +229,7 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
Artifact diagnosticsFile,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<String> includeDirs,
NestedSet<String> quoteIncludeDirs,
NestedSet<String> systemIncludeDirs,
Expand Down Expand Up @@ -297,7 +297,7 @@ private static CcToolchainVariables setupVariables(
Artifact diagnosticsFile,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
Expand Down Expand Up @@ -471,7 +471,7 @@ public static void setupCommonVariables(
boolean isUsingMemProf,
List<VariablesExtension> variablesExtensions,
Map<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
ImmutableList<PathFragment> includeDirs,
ImmutableList<PathFragment> quoteIncludeDirs,
ImmutableList<PathFragment> systemIncludeDirs,
Expand Down Expand Up @@ -507,7 +507,7 @@ private static void setupCommonVariablesInternal(
boolean isUsingMemProf,
List<VariablesExtension> variablesExtensions,
Map<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
Expand All @@ -526,9 +526,9 @@ private static void setupCommonVariablesInternal(
buildVariables.addStringVariable(MODULE_NAME.getVariableName(), cppModuleMap.getName());
buildVariables.addArtifactVariable(
MODULE_MAP_FILE.getVariableName(), cppModuleMap.getArtifact());
buildVariables.addStringSequenceVariable(
buildVariables.addArtifactSequenceVariable(
DEPENDENT_MODULE_MAP_FILES.getVariableName(),
Iterables.transform(directModuleMaps, Artifact::getExecPathString));
NestedSetBuilder.wrap(Order.STABLE_ORDER, directModuleMaps));
}
if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) {
// Module inputs will be set later when the action is executed.
Expand Down
Loading

0 comments on commit d41b5f8

Please sign in to comment.