update fsutil and docker

[alexcb]

update fsutil to include this patch: tonistiigi/fsutil@d952e50

docker also had to be updated due to tonistiigi/fsutil@2d121ce

Signed-off-by: Alex Couture-Beil alex@earthly.dev

[alexcb]

This is failing linting due to the deprecation that was added under docker/buildx#850 in particular under https://github.com/moby/moby/pull/43037/files#diff-709ecb416d43d86b2812c0eda71926f0af93cb7ef5f0486067a48e1e7ef90521R175

[aaronlehmann]

We should update contenthash to use the new methods and be consistent with fsutil, otherwise we may have cache inconsistencies. I can work on this and open a PR against your branch.

[alexcb]

We should update contenthash to use the new methods and be consistent with fsutil, otherwise we may have cache inconsistencies. I can work on this and open a PR against your branch.

If you're able to lend a hand with that, it would be greatly appreciated.

Otherwise, I'm happy to give it a try, but might need some pointers. Are you suggesting we change the signature of shouldIncludePath to return (bool, fileutils.MatchInfo, error), or just simply (fileutils.MatchInfo, error)?

[aaronlehmann]

I'll have time to do this tomorrow, but I'm also happy to do the code review if you want to give it a try.

shouldIncludePath can keep the same signature, but instead of recording the boolean result in maybeIncludedPath.matchedIncludePattern, it needs to store the MatchInfo in maybeIncludedPath so we can keep track of these for parent directories. Then this can get passed into subsequent calls for child directories (as we now do now with matchedIncludePattern). You can look at the changes we're vendoring here in fsutil/copy/copy.go as an example - that is ultimately the behavior the cache needs to match.

[tonistiigi]

@aaronlehmann Are there any actual cases where checksums on contenthash can change with this function switch?

[aaronlehmann]

@aaronlehmann Are there any actual cases where checksums on contenthash can change with this function switch?

Only the cases where we're intentionally changing the include/exclude behavior (for example the scenario in docker/buildx#850)

[aaronlehmann]

LGTM

[alexcb]

I think it's going to need some more work (maybe related to @tonistiigi 's question) as the tests are not happy.

In particular I'm able to get this test to fail for me when I run it locally in this branch (where as it works on master):

go test ./... -test.run=TestChecksumIncludeExclude
?   	github.com/moby/buildkit	[no test files]
?   	github.com/moby/buildkit/api/services/control	[no test files]
?   	github.com/moby/buildkit/api/types	[no test files]
ok  	github.com/moby/buildkit/cache	0.064s [no tests to run]
--- FAIL: TestChecksumIncludeExclude (0.00s)
    --- FAIL: TestChecksumIncludeExclude/wildcard_true (0.29s)
        checksum_test.go:553: 
            	Error Trace:	checksum_test.go:553
            	            				checksum_test.go:446
            	Error:      	Should not be: "sha256:04b3804d158e8960bd1ed0a0517cd9997c9474fa2b5e835fc847f1c4ce57b9e0"
            	Test:       	TestChecksumIncludeExclude/wildcard_true
    --- FAIL: TestChecksumIncludeExclude/wildcard_false (0.31s)
        checksum_test.go:553: 
            	Error Trace:	checksum_test.go:553
            	            				checksum_test.go:445
            	Error:      	Should not be: "sha256:04b3804d158e8960bd1ed0a0517cd9997c9474fa2b5e835fc847f1c4ce57b9e0"
            	Test:       	TestChecksumIncludeExclude/wildcard_false
FAIL

[aaronlehmann]

This should be excludePatternMatcher, not includePatternMatcher. Same two lines below. Didn't catch this in the first review.

[aaronlehmann]

Unfortunately, there is a bug in MatchesUsingParentResults. I opened moby/moby#43047 to fix it. We'll need to get that merged and then revendor.

[aaronlehmann]

In addition to the two things I pointed out, there is one more change we should make here. The code is currently clearing parentDirHeaders whenever a file or dir is selected for inclusion. This is suboptimal because it means we lose the context on whether the parent matched each pattern, and have to recompute this. The following code:

buildkit/cache/contenthash/checksum.go

Lines 667 to 677 in 159bb1e

	if !shouldInclude {
	if cr.Type == CacheRecordTypeDirHeader {
	// We keep track of non-included parent dir headers in case an
	// include pattern matches a file inside one of these dirs.
	parentDirHeaders = append(parentDirHeaders, maybeIncludedPath)
	}
	} else {
	includedPaths = append(includedPaths, parentDirHeaders...)
	parentDirHeaders = nil
	includedPaths = append(includedPaths, maybeIncludedPath)
	}

should change to:

		if shouldInclude {
			for _, parentDir := range parentDirHeaders {
				if !parentDir.included {
					includedPaths = append(includedPaths, parentDir)
					parentDir.included = true
				}
			}
			includedPaths = append(includedPaths, maybeIncludedPath)
			maybeIncludedPath.included = true
		}

		if cr.Type == CacheRecordTypeDirHeader {
			// We keep track of parent dir headers whether
			// they are immediately included or not, in case
			// an include pattern matches a file inside one
			// of these dirs.
			parentDirHeaders = append(parentDirHeaders, maybeIncludedPath)
		}

(and add a boolean included in includedPath)

[aaronlehmann]

@alexcb: moby/moby#43047 was merged, so it should be possible to move forward with this. Let me know if you want me to carry the PR.

[alexcb]

Thanks @aaronlehmann , I just opened up tonistiigi/fsutil#117 to pull those changes into fsutils, then once that's merged I'll pull that in here.

[aaronlehmann]

LGTM