solver: use toSelectors to filter root paths instead of custom logic #4281
Conversation
| @@ -468,3 +470,54 @@ COPY --from=base /tmpfssize / | |||
| require.NoError(t, err) | |||
| require.Contains(t, string(dt), `size=131072k`) | |||
| } | |||
|
|
|||
| func testMountDuplicate(t *testing.T, sb integration.Sandbox) { | |||
There was a problem hiding this comment.
Leave a comment // moby/buildkit#4123 here so it is clear that this is a regression test.
|
I was looking at adding a better unit test for I've removed the unit test as it no longer serves a purpose and the integration test now exists. |
13caaaf to
ff1afcc
Compare
| func forceTrailingSlash(s string) string { | ||
| if strings.HasSuffix(s, "/") { | ||
| return s | ||
| } | ||
| return s + "/" | ||
| } |
There was a problem hiding this comment.
This is to handle the situation where / is the prefix path. Unfortunately, this doesn't work if dedupePaths is called with []string{"", "foo"}.
|
I want to also fix up |
ff1afcc to
a22902a
Compare
|
It looks like a potential correctness issue. Since the dedupe happens in Should be an easy fix though. |
I think I was wrong in here. Selectors digest should not be deduplicated. Eg. if you have two mounts and one is inside another, if you switch their order, that should change the definition-based cache key because the result of the op changes. |
Ok I'll revert that and keep the selectors digest as not being deduplicated at all. |
70e30cd to
5cec511
Compare
|
Ok I've reverted the behavior. The main change is that the definition based key will most likely include the root binding in the definition while it previously omitted it. This is just due to the check being done after the selectors are added rather than before. I don't think that should end up mattering though. |
This updates moby#4270 to add an integration test and also merge some of the logic for how the selectors are created. Now, `toSelectors` will perform the root path detection instead of some custom logic in `getMountDeps`. `dedupePaths` has also been updated to check if the number of paths is 1 or less so it can avoid an allocation when the function is a no-op. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
5cec511 to
091fb2c
Compare
This updates #4270 to add an integration test and also merge some of the
logic for how the selectors are created. Now,
toSelectorswill performthe root path detection instead of some custom logic in
getMountDeps.dedupePathshas also been updated to check if the number of paths is 1or less so it can avoid an allocation when the function is a no-op.