close
Skip to content

Use path mapping collision support in HeaderDiscovery #29611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
layus wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Use path mapping collision support in HeaderDiscovery #29611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,6 +1368,7 @@ java_library(
"//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/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/actions:virtual_action_input",
"//src/main/java/com/google/devtools/build/lib/analysis/config:build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis/config:core_options",
Expand Down
54 changes: 21 additions & 33 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Multimap;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.Artifact;
Expand All @@ -36,11 +38,8 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import javax.annotation.Nullable;

/**
* HeaderDiscovery checks whether all header files that a compile action uses are actually declared
Expand Down Expand Up @@ -77,25 +76,16 @@ static NestedSet<Artifact> discoverInputsFromDependencies(
boolean siblingRepositoryLayout,
PathMapper pathMapper)
throws ActionExecutionException {
Map<PathFragment, Artifact> regularDerivedArtifacts = new HashMap<>();
Map<PathFragment, SpecialArtifact> treeArtifacts = new HashMap<>();
Multimap<PathFragment, Artifact> regularDerivedArtifacts = LinkedHashMultimap.create();
Multimap<PathFragment, SpecialArtifact> treeArtifacts = LinkedHashMultimap.create();
for (Artifact a : allowedDerivedInputs.toList()) {
if (a.isSourceArtifact()) {
continue;
}
// We may encounter duplicate keys in the derived inputs if two artifacts have different
// owners. Just use the first one. The two artifacts must be generated by equivalent
// (shareable) actions in order to have not generated a conflict in Bazel. If on an
// incremental build one changes without the other one changing, then if their paths remain
// the same, that will trigger an action conflict and fail the build. If one path changes,
// then this action will be re-analyzed, and will execute in Skyframe. It can legitimately get
// an action cache hit in that case, since even if it previously depended on the artifact
// whose path changed, that is not taken into account by the action cache, and it will get an
// action cache hit using the remaining un-renamed artifact.
if (a.isTreeArtifact()) {
treeArtifacts.putIfAbsent(pathMapper.map(a.getExecPath()), (SpecialArtifact) a);
treeArtifacts.put(pathMapper.map(a.getExecPath()), (SpecialArtifact) a);
} else {
regularDerivedArtifacts.putIfAbsent(pathMapper.map(a.getExecPath()), a);
regularDerivedArtifacts.put(pathMapper.map(a.getExecPath()), a);
}
}

Expand All @@ -119,8 +109,8 @@ private static NestedSet<Artifact> runDiscovery(
boolean shouldValidateInclusions,
Collection<Path> dependencies,
List<Path> permittedSystemIncludePrefixes,
Map<PathFragment, Artifact> regularDerivedArtifacts,
Map<PathFragment, SpecialArtifact> treeArtifacts,
Multimap<PathFragment, Artifact> regularDerivedArtifacts,
Multimap<PathFragment, SpecialArtifact> treeArtifacts,
Path execRoot,
ArtifactResolver artifactResolver,
boolean siblingRepositoryLayout,
Expand Down Expand Up @@ -192,8 +182,8 @@ private static NestedSet<Artifact> runDiscovery(
}
}
Collection<? extends Artifact> resolvedArtifacts = ImmutableList.of();
Artifact derivedArtifact = regularDerivedArtifacts.get(execPathFragment);
if (derivedArtifact == null) {
Collection<Artifact> derivedArtifacts = regularDerivedArtifacts.get(execPathFragment);
if (derivedArtifacts.isEmpty()) {
Optional<PackageIdentifier> pkgId =
PackageIdentifier.discoverFromExecPath(
execPathFragment, false, siblingRepositoryLayout);
Expand All @@ -212,7 +202,7 @@ private static NestedSet<Artifact> runDiscovery(
}
}
} else {
resolvedArtifacts = ImmutableList.of(derivedArtifact);
resolvedArtifacts = derivedArtifacts;
}
if (!resolvedArtifacts.isEmpty()) {
// We don't need to add the sourceFile itself as it is a mandatory input.
Expand Down Expand Up @@ -249,10 +239,10 @@ private static NestedSet<Artifact> runDiscovery(
continue;
}

SpecialArtifact treeArtifact =
findOwningTreeArtifact(execPathFragment, treeArtifacts, pathMapper);
if (treeArtifact != null) {
inputs.add(treeArtifact);
Collection<SpecialArtifact> owningTreeArtifacts =
findOwningTreeArtifacts(execPathFragment, treeArtifacts, pathMapper);
if (!owningTreeArtifacts.isEmpty()) {
Copy link
Copy Markdown
Collaborator

@fmeum fmeum May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic seems wrong, I think we would need to verify that all artifacts have an owning tree artifact, not just one.

Copy link
Copy Markdown
Contributor Author

@layus layus May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely right, the whole findOwningTreeArtifacts is bad.

inputs.addAll(owningTreeArtifacts);
} else {
// Record a problem if we see files that we can't resolve, likely caused by undeclared
// includes or illegal include constructs.
Expand Down Expand Up @@ -282,23 +272,21 @@ private static NestedSet<Artifact> runDiscovery(
return inputs.build();
}

@Nullable
private static SpecialArtifact findOwningTreeArtifact(
private static Collection<SpecialArtifact> findOwningTreeArtifacts(
PathFragment execPath,
Map<PathFragment, SpecialArtifact> treeArtifacts,
Multimap<PathFragment, SpecialArtifact> treeArtifacts,
PathMapper pathMapper) {
// Check the map for the exec path's parent directory first. If the exec path matches a direct
// child of a tree artifact (a common case), we can skip the full iteration below.
PathFragment dir = execPath.getParentDirectory();
SpecialArtifact tree = treeArtifacts.get(dir);
if (tree != null) {
return tree;
Collection<SpecialArtifact> trees = treeArtifacts.get(dir);
if (!trees.isEmpty()) {
return trees;
}

// Search for any tree artifact that encloses the exec path.
return treeArtifacts.values().stream()
.filter(a -> dir.startsWith(pathMapper.map(a.getExecPath())))
.findAny()
.orElse(null);
.collect(ImmutableList.toImmutableList());
}
}
119 changes: 119 additions & 0 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscoveryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,97 @@ public void windowsPlatform_multipleCaseInsensitiveMatches_onlyOneInActionInputs
assertThat(result.toList()).containsExactly(variant2);
}

private final ArtifactRoot fastbuildRoot =
ArtifactRoot.asDerivedRoot(execRoot, RootType.OUTPUT, DERIVED_SEGMENT, "k8-fastbuild", "bin");
private final ArtifactRoot optRoot =
ArtifactRoot.asDerivedRoot(execRoot, RootType.OUTPUT, DERIVED_SEGMENT, "k8-opt", "bin");

/**
* A simple PathMapper that strips the config segment from output paths, mapping e.g.
* "derived/k8-fastbuild/bin/gen/foo.h" → "derived/cfg/bin/gen/foo.h".
*/
private static final PathMapper STRIPPING_PATH_MAPPER =
new PathMapper() {
@Override
public PathFragment map(PathFragment execPath) {
// Only map output paths of the form "derived/<config>/bin/..."
if (execPath.startsWith(PathFragment.create(DERIVED_SEGMENT))
&& execPath.segmentCount() >= 3) {
return execPath
.subFragment(0, 1)
.getRelative("cfg")
.getRelative(execPath.subFragment(2));
}
return execPath;
}
};

@Test
public void pathMapped_twoArtifactsSameMappedPath_bothDiscovered() throws Exception {
ArtifactResolver artifactResolver = mock(ArtifactResolver.class);
Artifact derivedFastbuild =
ActionsTestUtil.createArtifact(
fastbuildRoot, execRoot.getRelative("derived/k8-fastbuild/bin/gen/foo.h"));
Artifact derivedOpt =
ActionsTestUtil.createArtifact(
optRoot, execRoot.getRelative("derived/k8-opt/bin/gen/foo.h"));

// The .d file reports the mapped path
Path mappedDepPath = execRoot.getRelative("derived/cfg/bin/gen/foo.h");

NestedSet<Artifact> result =
discoverInputs(
nonWindowsAction(),
artifactResolver,
ImmutableList.of(mappedDepPath),
NestedSetBuilder.create(Order.STABLE_ORDER, derivedFastbuild, derivedOpt),
STRIPPING_PATH_MAPPER);

// Both artifacts should appear in the discovered inputs since their mapped paths collide
assertThat(result.toList()).containsExactly(derivedFastbuild, derivedOpt);
}

@Test
public void pathMapped_treeArtifactsSameMappedPath_bothDiscovered() throws Exception {
ArtifactResolver artifactResolver = mock(ArtifactResolver.class);
SpecialArtifact treeFastbuild =
treeArtifact(execRoot.getRelative("derived/k8-fastbuild/bin/gen/tree"), fastbuildRoot);
SpecialArtifact treeOpt =
treeArtifact(execRoot.getRelative("derived/k8-opt/bin/gen/tree"), optRoot);

// The .d file reports a file under the mapped tree path
Path mappedDepPath = execRoot.getRelative("derived/cfg/bin/gen/tree/header.h");

NestedSet<Artifact> result =
discoverInputs(
nonWindowsAction(),
artifactResolver,
ImmutableList.of(mappedDepPath),
NestedSetBuilder.create(Order.STABLE_ORDER, treeFastbuild, treeOpt),
STRIPPING_PATH_MAPPER);

// Both tree artifacts should be discovered
assertThat(result.toList()).containsExactly(treeFastbuild, treeOpt);
}

@Test
public void noPathMapping_singleArtifactDiscovered() throws Exception {
ArtifactResolver artifactResolver = mock(ArtifactResolver.class);
Artifact derived =
ActionsTestUtil.createArtifact(derivedArtifactRoot, derivedRoot.getRelative("gen/foo.h"));

NestedSet<Artifact> result =
discoverInputs(
nonWindowsAction(),
artifactResolver,
ImmutableList.of(derivedRoot.getRelative("gen/foo.h")),
NestedSetBuilder.create(Order.STABLE_ORDER, derived),
PathMapper.NOOP);

// With NOOP, standard single-entry behavior
assertThat(result.toList()).containsExactly(derived);
}

// Helpers

private void checkHeaderInclusion(
Expand Down Expand Up @@ -385,6 +476,34 @@ private NestedSet<Artifact> discoverInputs(
PathMapper.NOOP);
}

private NestedSet<Artifact> discoverInputs(
ActionsTestUtil.NullAction action,
ArtifactResolver artifactResolver,
List<Path> dependencies,
NestedSet<Artifact> allowedDerivedInputs,
PathMapper pathMapper)
throws ActionExecutionException {
return HeaderDiscovery.discoverInputsFromDependencies(
action,
ActionsTestUtil.createArtifact(derivedArtifactRoot, derivedRoot.getRelative("foo.cc")),
/* shouldValidateInclusions= */ true,
dependencies,
/* permittedSystemIncludePrefixes= */ ImmutableList.of(),
allowedDerivedInputs,
execRoot,
artifactResolver,
/* siblingRepositoryLayout= */ false,
pathMapper);
}

private SpecialArtifact treeArtifact(Path path, ArtifactRoot root) {
return SpecialArtifact.create(
root,
root.getExecPath().getRelative(root.getRoot().relativize(path)),
ActionsTestUtil.NULL_ARTIFACT_OWNER,
Artifact.SpecialArtifactType.TREE);
}

private SpecialArtifact treeArtifact(Path path) {
return SpecialArtifact.create(
derivedArtifactRoot,
Expand Down