client: modify SolveOpt to take fsutil.FS objects #4094
Conversation
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
2 times, most recently
from
0de1f87
to
b69a117
Compare
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
2 times, most recently
from
8079693
to
2dea35e
Compare
client/solve.go
Outdated
| sourceFS := make(map[string]fsutil.FS) | ||
| for k, fs := range opt.LocalFiles { | ||
| sourceFS[k] = fs | ||
| } | ||
| for k, dir := range opt.LocalDirs { | ||
| fs, err := fsutil.NewFS(dir) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| sourceFS[k] = fs | ||
| } |
There was a problem hiding this comment.
I think it would be better to move this into a function. mergeLocalX with X being the name we decide for the solve opt name. So if it's local mounts, mergeLocalMounts, createLocalMounts, or maybe just createSourceFS?
Alternatively, since LocalDirs is deprecated, maybe we shouldn't implement this merging behavior? If LocalFiles is defined, use it exclusively. If it's not, fallback on LocalDirs.
There was a problem hiding this comment.
I think it would be better to move this into a function.
mergeLocalXwithXbeing the name we decide for the solve opt name. So if it's local mounts,mergeLocalMounts,createLocalMounts, or maybe justcreateSourceFS?
Given this is such a short snippet and not used anywhere, I don't see the need to move it - I've annotated with a comment though.
Alternatively, since
LocalDirsis deprecated, maybe we shouldn't implement this merging behavior? IfLocalFilesis defined, use it exclusively. If it's not, fallback onLocalDirs.
If we're aiming to have a release in which both are supported, then the most obvious behavior to me as a user is that setting both result in both getting set. The scenarios it makes sense to choose one over the other exclusively is at the gRPC API level, when any well-behaved client cannot set both. If we're aiming to not release anything where both are set (e.g. in a follow-up removing opt.LocalDirs), then it doesn't really matter the behavior atm.
There was a problem hiding this comment.
I generally prefer a function if it's possible. It reduces the number of live local variables, self-documents itself, and also reduces the complexity for reading the function. In isolation, this snippet may be simple. In the overall context, each small bit of reduced complexity helps someone understand the intention of the top level function. In this case, I think this behavior is inconsequential to understanding solve so it's helpful to just remove it.
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
from
2dea35e
to
cfadf74
Compare
|
@tonistiigi PTAL when you have a moment. |
Signed-off-by: Justin Chadwell <me@jedevc.com>
This patch modifies the function signature of the FSSync provider to take an fsutil.FS instead of a simple raw path resolved to the client's root filesystem. Internally, we were already creating an fsutil.FS to Send to the buildkit server, however, this abstraction didn't reach the session attachable parameters, so we couldn't provide our own custom FS implementation. The rationale behind this change is to allow providing more abstract custom filesystem implementations to a BuildKit client. This way, we can start to build from filesystems that might not be on disk - for example, we could use our Static filesystem implementation in tests to prevent creating lots of temporary directories, or we could use our Merge filesystem implementation to allow easily creating variants of a single context. Signed-off-by: Justin Chadwell <me@jedevc.com>
This completes propogating the fsutil.FS abstraction into the SolveOpt, deprecating the old LocalDirs. Since this is entirely a golang-level abstraction, we could potentially investigate just removing the old LocalDirs directly. Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Now we error out if there is any clash between LocalDirs and LocalMounts. Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
fsutil-fs-in-opt
branch
from
cfadf74
to
0f343f9
Compare
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
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 <1951866+crazy-max@users.noreply.github.com>
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 <1951866+crazy-max@users.noreply.github.com>
This patch modifies the function signature of the FSSync provider to take an fsutil.FS instead of a simple raw path resolved to the client's root filesystem, and follows this through to allow specifying fsutil.FS objects on the SolveOpt itself.
Internally, we were already creating an fsutil.FS to Send to the buildkit server, however, this abstraction didn't reach the session attachable parameters, so we couldn't provide our own custom FS implementation.
The rationale behind this change is to allow providing more abstract custom filesystem implementations to a BuildKit client. This way, we can start to build from filesystems that might not be on disk - for example, we could use our Static filesystem implementation in tests to prevent creating lots of temporary directories, or we could use our Merge filesystem implementation to allow easily creating variants of a single context.