Support path mapping collisions for identical files#29601
Conversation
github-actions
Bot
added
team-Configurability
layus
force-pushed
the
feature/allow-mapping-collision-of-identical-files
branch
from
b8632a4 to
e9d1321
Compare
layus
force-pushed
the
feature/allow-mapping-collision-of-identical-files
branch
from
e9d1321 to
03a0959
Compare
|
@layus Could you please take a look at the failing checks? |
bharadwaj08-one
added
awaiting-user-response
|
@bharadwaj08-one Not much to go by: src/main/tools/linux-sandbox-pid1.cc:204: "mount": Permission denied ERROR: 'linux-sandbox' was requested for explicit default strategies but no strategy with that identifier was registered. Valid values are: [dynamic_worker, processwrapper-sandbox, standalone, dynamic, remote, worker, sandboxed, local] For some reason the errors seem sanbox related. Seems pretty unrelated to the changes here, unless they all cover collisions that were silently ignored before ? |
Add InputMetadataProvider to PathMappers.create() to verify that colliding inputs from different configurations have identical file digests. Update StrippingPathMapper and MerkleTreeComputer to check digest equality when path collisions are detected.
layus
force-pushed
the
feature/allow-mapping-collision-of-identical-files
branch
from
03a0959 to
6235cef
Compare
|
@bharadwaj08-one Done, rebased and fixed. |
| private static PathFragment strip(PathFragment execPath) { | ||
| return execPath | ||
| .subFragment(0, 1) | ||
| // Keep the config segment, but replace it with a fixed string to improve cacheability while |
|
|
||
| /** Tests for collision handling in {@link StrippingPathMapper#isPathStrippable}. */ | ||
| @RunWith(JUnit4.class) | ||
| public final class StrippingPathMapperTest { |
There was a problem hiding this comment.
This test feels too synthetic to me. I think you can drop it.
Instead, could you add an integration test to path_mapping_test.sh that creates a situation in which two collising inputs are in fact identical and then verify that path mapping was used for the action?
There was a problem hiding this comment.
How would you reliably check that path mapping is used. the only thing I can think of right now is parsing bazel build -s to find /cfg/ in there.
There was a problem hiding this comment.
You can put the test in https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/integration/config_stripped_outputs_test.sh and use the helpers defined in the _lib.sh file.
bharadwaj08-one
added
awaiting-review
3 participants
Description
As per https://bazelbuild.slack.com/archives/CA31HN1T3/p1778851259301539?thread_ts=1775817669.172489&cid=CA31HN1T3 discussion with @fmeum , This implements support for path collisions amongst mapped files as long as their content is the same.
Motivation
I hope this fixes two issues. One with silently colliding declared inputs, taking the first one. And another when the same file ends-up showing in the dependencies through two different transitions, in particular with discovered inputs, like cc rules do.
Build API Changes
Oddly enough, the change has minimal impact on the APIs. All it adds is an optional
@NullableInputMetadataProviderto PathMapper create() method.Checklist
Release Notes
RELNOTES: None