Skip to content

Commit

Permalink
[7.4.0] Support layering_check with C++ path mapping (#23671)
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

Fixes #23178
  • Loading branch information
fmeum authored Sep 23, 2024
1 parent 4956dff commit fd83239
Show file tree
Hide file tree
Showing 9 changed files with 193 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -581,8 +581,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 @@ -595,8 +595,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 @@ -651,7 +651,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 @@ -1154,7 +1154,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 @@ -22,7 +22,9 @@
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
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.starlark.StarlarkActionFactory;
import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext;
Expand Down Expand Up @@ -590,6 +592,7 @@ public void createModuleMapAction(
Boolean generateSubmodules,
Boolean withoutExternDependencies)
throws EvalException {
RuleContext ruleContext = actions.getRuleContext();
ActionConstructionContext actionConstructionContext = actions.getActionConstructionContext();
actions
.asActionRegistry(actions)
Expand All @@ -609,6 +612,10 @@ public void createModuleMapAction(
compiledModule,
moduleMapHomeIsCwd,
generateSubmodules,
withoutExternDependencies));
withoutExternDependencies,
PathMappers.getOutputPathsMode(ruleContext.getConfiguration()),
ruleContext
.getConfiguration()
.modifiedExecutionInfo(ImmutableMap.of(), CppModuleMapAction.MNEMONIC)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1159,6 +1159,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 @@ -1470,6 +1518,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 @@ -163,7 +163,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 @@ -237,7 +237,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 @@ -303,7 +303,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 @@ -475,7 +475,7 @@ public static void setupCommonVariables(
String fdoStamp,
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 @@ -509,7 +509,7 @@ private static void setupCommonVariablesInternal(
String fdoStamp,
List<VariablesExtension> variablesExtensions,
Map<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
ImmutableList<Artifact> directModuleMaps,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
Expand All @@ -528,9 +528,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 fd83239

Please sign in to comment.