Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: split out include/exclude into new FilterFS #167

Merged
merged 3 commits into from
Aug 25, 2023

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented Aug 2, 2023

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).

filter.go Outdated Show resolved Hide resolved
@jedevc jedevc force-pushed the fsutil-fs-in-opt branch 2 times, most recently from 39e86fe to c871a2b Compare August 3, 2023 10:09
@jedevc jedevc marked this pull request as ready for review August 4, 2023 15:56
filter.go Outdated Show resolved Hide resolved
filter.go Outdated
return fs.fs.Open(p)
}

func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc) error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fsutil/fs.go

Lines 17 to 20 in 36ef4d8

type FS interface {
Walk(context.Context, filepath.WalkFunc) error
Open(string) (io.ReadCloser, error)
}

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.

filter.go Outdated Show resolved Hide resolved
followlinks.go Outdated Show resolved Hide resolved
followlinks.go Show resolved Hide resolved
Copy link
Owner

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

followlinks.go Outdated Show resolved Hide resolved
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]>
@jedevc
Copy link
Collaborator Author

jedevc commented Aug 24, 2023

@tonistiigi is there anything blocking this? Happy to take another pass (or prep a follow-up) if there's anything needed here.

@tonistiigi tonistiigi merged commit eec1962 into tonistiigi:master Aug 25, 2023
5 checks passed
@jedevc jedevc deleted the fsutil-fs-in-opt branch January 24, 2024 17:10
crazy-max added a commit to crazy-max/compose that referenced this pull request Jan 30, 2024
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:/crazy-max/buildkit/tree/compose-957cb50df991
has been created at the regression point and reverts moby/buildkit#4094.
Another branch for buildx https:/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]>
crazy-max added a commit to crazy-max/compose that referenced this pull request Jan 30, 2024
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:/crazy-max/buildkit/tree/compose-957cb50df991
has been created at the regression point and reverts moby/buildkit#4094.

Signed-off-by: CrazyMax <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants