fs: split out include/exclude into new FilterFS#167
fs: split out include/exclude into new FilterFS#167tonistiigi merged 3 commits intotonistiigi:masterfrom
Conversation
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
2 times, most recently
from
39e86fe to
c871a2b
Compare
| return fs.fs.Open(p) | ||
| } | ||
|
|
||
| func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc) error { |
There was a problem hiding this comment.
This Walk() method here seems out of place. The signature for walk should be Walk(fs, .., cb). FS should implement something that make this possible, eg. ReadDir(name) and f.ReadDir()
There was a problem hiding this comment.
The current semantics are difficult to express if we change to that model.
Imagine the following ExcludePatterns: (/foo, !/foo/x).
Currently, if we Walk this filesystem, the callback will only be called for /foo if /foo/x exists, otherwise it gets skipped.
If we try and express ReadDir like this, then ReadDir("/") will need to check for the existence of /foo/x to know if /foo should be returned. This would be quite a major rewrite of the include/exclude patterns logic, not sure if you think this would be worth it?
There was a problem hiding this comment.
Maybe the logic then needs to be that filter walks whole baseFS internally either on constructor/function-call or on the first use.
The current semantics are difficult to express if we change to that model.
I see, but I don't see the benefit then of defining a FilterFS if it doesn't actually satisfy the FS interface.
There was a problem hiding this comment.
Lines 17 to 20 in 36ef4d8
It does though? fsutil.FS only defines Open and Walk for now.
I think switching to be more in match with io/fs is a good idea, but I'd propose that's out of scope for this PR - unless we want to make the argument to do a large PR to completely rework the internals of this package.
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
from
c871a2b to
7a2e305
Compare
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
from
5961f9b to
f8658da
Compare
tonistiigi
left a comment
tonistiigi
left a comment
There was a problem hiding this comment.
I'm still not quite sure why implement statFile with Walk instead of defining Stat() (and then maybe removing path from Walk) but that discussion can be left for some follow-up.
This patch creates a new FilterFS to pair with a new FilterOpt, which contains the old WalkOpt parameters. Essentially, this splits the functionality of the old FS into a new FS and the FilterFS. The new FS implemenation simply performs operations over a specified local filesystem, while the FilterFS wraps an existing FS implementation to apply filtering patterns. This allows the same logic for filtering to apply to *any* underlying filesystem, for example, the StaticFS and MergeFS implementations in BuildKit. To do this, we also make some reasonably substantial changes to the developer-facing API. We need to use fs.WalkDirFunc instead of the old filepath.WalkFunc, which is required to preserve the lazy stat() semantics from b9e22fc. Existing implementations and callers of FS should fairly easily be able to update to support the new API, which just requires updating to support a new "path" parameter, and modify the signature to be fs.WalkDirFunc (which may actually improve performance depending on the exact usage). Signed-off-by: Justin Chadwell <[email protected]>
Paths removed by the include/exclude filters should not be allowed to be opened by the Open function. To prevent constructing the patternmatcher objects and traversing the filesystem using FollowLinks for each call to Open, the patternmatcher objects are constructed at the point of creation of the FilterFS. Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
from
f8658da to
da27602
Compare
|
@tonistiigi is there anything blocking this? Happy to take another pass (or prep a follow-up) if there's anything needed here. |
Commit 7781b7c vendoring buildx master introduced a regression with a bump of the peer dependency github.com/tonistiigi/fsutil. full diff: tonistiigi/fsutil@36ef4d8...f098008 When bisecting, tonistiigi/fsutil#167 is the PR introducing the regression. We got a similar issue reported before DD 4.27 (docker/buildx#2207) that was fixed with tonistiigi/fsutil@master...crazy-max:fsutil:toslash-keep-gogo but Windows users encountered another new issue also related to fsutil. While a fix is being worked on fsutil repo to address this issue, I have created a branch that reverts this change in fsutil. This branch for buildkit https://github.com/crazy-max/buildkit/tree/compose-957cb50df991 has been created at the regression point and reverts moby/buildkit#4094. Another branch for buildx https://github.com/crazy-max/buildx/tree/compose-617f538cb315 has been created as well to vendor the buildkit branch and replace both buildkit and fsutil to the right commit. Signed-off-by: CrazyMax <[email protected]>
Commit 7781b7c vendoring buildx master introduced a regression with a bump of the peer dependency github.com/tonistiigi/fsutil. full diff: tonistiigi/fsutil@36ef4d8...f098008 When bisecting, tonistiigi/fsutil#167 is the PR introducing the regression. We got a similar issue reported before DD 4.27 (docker/buildx#2207) that was fixed with tonistiigi/fsutil@master...crazy-max:fsutil:toslash-keep-gogo but Windows users encountered another new issue also related to fsutil. While a fix is being worked on fsutil repo to address this issue, I have created a branch that reverts this change in fsutil. This branch for buildkit https://github.com/crazy-max/buildkit/tree/compose-957cb50df991 has been created at the regression point and reverts moby/buildkit#4094. Signed-off-by: CrazyMax <[email protected]>
2 participants
This patch creates a new FilterFS to pair with a new FilterOpt, which contains the old WalkOpt parameters.
Essentially, this splits the functionality of the old FS into a new FS and the FilterFS. The new FS implemenation simply performs operations over a specified local filesystem, while the FilterFS wraps an existing FS implementation to apply filtering patterns.
This allows the same logic for filtering to apply to any underlying filesystem, for example, the StaticFS and MergeFS implementations in BuildKit (see moby/buildkit#4094).
To do this, we also make some reasonably substantial changes to the developer-facing API. We need to use fs.WalkDirFunc instead of the old filepath.WalkFunc, which is required to preserve the lazy stat() semantics from b9e22fc.
Existing implementations and callers of FS should fairly easily be able to update to support the new API, which just requires updating to support a new "path" parameter, and modify the signature to be fs.WalkDirFunc (which may actually improve performance depending on the exact usage).