[v0.7] revert dockerignore changes

[tonistiigi]

Reverts

• tonistiigi/fsutil@da5201e
• tonistiigi/fsutil@9d0ab16

@aaronlehmann

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

[aaronlehmann]

LGTM

[crazy-max]

I'm still reproducing #850: https://github.com/crazy-max/buildx-dockerignore-issue/runs/4607531429?check_suite_focus=true#step:4:136

[aaronlehmann]

I think this revert was meant for #883 (but maybe I misunderstood).

#850 might need the changes in moby/moby#43037 and moby/moby#43047. Even though this code is no longer being used by fsutil with the reverts here, there is code in pkg/archive/archive.go using it - perhaps that's involved?

Alternatively, moby/moby#42676 could be reverted, but this would regress moby/moby#41433.

[tonistiigi]

Alternatively, moby/moby#42676 could be reverted

Then we would need to also revert tonistiigi/fsutil#107 ? Is there a simpler fix that just doesn't call the new methods that it adds?

[tonistiigi]

there is code in pkg/archive/archive.go using it - perhaps that's involved?

I can't see how this might be related. We don't create tarballs anymore for sending local files.

[crazy-max]

@aaronlehmann

I think this revert was meant for #883 (but maybe I misunderstood).

Yes indeed, I have tested your PR(s):

• Avoid traversal of directories whose children are impossible to match tonistiigi/fsutil#121
• fileutils: Avoid compiling a regexp for simple patterns moby/moby#43095

And seems to work fine for #850 issue: crazy-max@57678ea

Repro: https://github.com/crazy-max/buildx-dockerignore-issue/runs/4609519849?check_suite_focus=true#step:4:136

Do you have a simple repro for #883 I could use in https://github.com/crazy-max/buildx-dockerignore-issue?

[aaronlehmann]

Then we would need to also revert tonistiigi/fsutil#107 ? Is there a simpler fix that just doesn't call the new methods that it adds?

I wasn't sure if the changes to builder/remotecontext/detect.go and pkg/archive/archive.go were relevant in this context. If they are not, then just reverting the changes in the walker code from tonistiigi/fsutil#107 should be enough. We should either keep the copy code as is, or also revert moby/buildkit#2319 (contenthash and copy need to stay consistent).

Yes indeed, I have tested your PR(s):
tonistiigi/fsutil#121
moby/moby#43095
And seems to work fine for #850 issue: crazy-max/buildx@57678ea

#850 has been overloaded to include multiple issues. Are you testing the original issue, or the one about file permissions? The fsutil PR you referenced is about the permissions thing (and the moby PR is an optimization that should not have any visible impact).

[tonistiigi]

Is there a simpler fix that just doesn't call the new methods that it adds?

Another approach might be to try to rework tonistiigi/fsutil@da5201e without tonistiigi/fsutil@9d0ab16 . Atm it conflicts a lot so I needed to revert both but not sure if the logic of the fix actually depends on the previous commit.

Are you testing the original issue, or the one about file permissions?

Both. Current issue seems to be with the original report. The goal would be to have no regressions between v0.6 and v0.7. For this one I believe we fixed it in v0.7.1 but these reverts are now reverting the v0.7.1 fix as well.

[aaronlehmann]

Another approach might be to try to rework tonistiigi/fsutil@da5201e without tonistiigi/fsutil@9d0ab16

I suppose, but that feels like relatively high effort/high risk. We would end up with different code in the release branch. I prefer to either revert back to a known good fsutil version (not sure if I'm missing something about why we can't do this), or fix forward with moby/moby#43037 + moby/moby#43047 + tonistiigi/fsutil#114 + tonistiigi/fsutil#120 + moby/buildkit#2486.

[tonistiigi]

known good fsutil version (not sure if I'm missing something about why we can't do this),

What is this version? Based on the previous comment it seems that we would need to split tonistiigi/fsutil#107

[aaronlehmann]

Is the issue that you don't want to regress moby/moby#41433? This was a longstanding issue and affected buildx < 0.7 AFAIK, but I can see that if it was fixed in 0.7, it would be unfortunate to reintroduce the bug.

You could revert tonistiigi/fsutil#107 and take only the first commit from moby/moby#42676. That first commit is a very simple fix for #41433, and the other commits are an optimization you asked for to create new functions that do the matching incrementally. The optimization is the cause of #850.

[crazy-max]

LGTM

[tonistiigi]

ping @aaronlehmann

[aaronlehmann]

This will regress moby/moby#41433, right? (*PatternMatcher).Matches was not updated with the fix for moby/moby#41433 in the final version of the PR. If you want to keep the fix for moby/moby#41433, we need to take the first commit from moby/moby#42676, which does update Matches.

If you don't care about removing the fix for moby/moby#41433, this looks okay to me.