close
Skip to content

Use path mapping collision support in HeaderDiscovery#29611

Draft
layus wants to merge 1 commit into
bazelbuild:masterfrom
layus:feature/header-discovery-collision-support
Draft

Use path mapping collision support in HeaderDiscovery#29611
layus wants to merge 1 commit into
bazelbuild:masterfrom
layus:feature/header-discovery-collision-support

Conversation

@layus
Copy link
Copy Markdown
Contributor

@layus layus commented May 21, 2026

Description

This adds a multimap to track all the artefacts behind a sandbox path in header discovery.

Motivation

This should enable header discovery to properly track dependencies on multiple inputs providing the same in put path.
This is in particular important for path mapping, where files can be mapped to the same path with #29601.

Build API Changes

No

Checklist

  • I have added tests for the new use cases (if any).
  • I have updated the documentation (if applicable).

Release Notes

RELNOTES: None

@layus layus requested review from a team, lberki, pzembrod and trybka as code owners May 21, 2026 08:42
@layus layus requested review from dabanki and removed request for a team May 21, 2026 08:42
@github-actions github-actions Bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels May 21, 2026
@bharadwaj08-one
Copy link
Copy Markdown

@layus Could you please take a look at the failing checks?

@bharadwaj08-one bharadwaj08-one added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 21, 2026
@layus
Copy link
Copy Markdown
Contributor Author

layus commented May 21, 2026

Also, there is something fishy about these ubuntu tests. They failed on all my recent PRs.

@fmeum
Copy link
Copy Markdown
Collaborator

fmeum commented May 21, 2026

Rebasong onto latest master should fix the failures

@layus layus force-pushed the feature/header-discovery-collision-support branch from b1fc41d to 5093972 Compare May 21, 2026 19:30
Update HeaderDiscovery to use Multimap for tracking artifacts, enabling
it to handle path-mapped collisions where artifacts from different
configurations map to the same path in order to track the dependency on
all the conflicting sources instead of the first.
@layus layus force-pushed the feature/header-discovery-collision-support branch from 5093972 to 8ab0634 Compare May 21, 2026 19:41
@layus
Copy link
Copy Markdown
Contributor Author

layus commented May 21, 2026

Okay, I had my git repo in a broken state where upstream would not update correctly. Fixed for all four PRs.

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

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

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.

@layus layus marked this pull request as draft May 21, 2026 20:43
@bharadwaj08-one bharadwaj08-one added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants