api/server/router/build: BuilderVersion: allow buildkit on Windows #48038
Conversation
thaJeztah
added
area/builder
status/2-code-review
kind/experimental
area/builder/buildkit
|
I have no IDEA whatsoever if things are wired up already inside the daemon; this PR removes a gate that prevented the daemon from advertising it supports BuildKit, but I don't have a Windows machine to test on if any of that already works. |
api/server/router/build/build.go
Outdated
| } | ||
|
|
||
| bv := types.BuilderBuildKit | ||
| // Allow the features field in the daemon config to override the | ||
| // default builder advertise. | ||
| if v, ok := features["buildkit"]; ok && !v { | ||
| bv = types.BuilderV1 |
There was a problem hiding this comment.
This won't do anything on Windows, because we already overrode it above 🙈 . I'll fix that.
Commit 7b153b9 changed the daemon to advertise the recommended builder to use to V2 (BuildKit) for Linux daemons, and V1 (Legacy Builder) for Windows daemons. For Linux daemons we allowed the default to be overridden through the "features" field in the daemon config (daemon.json), but for Windows we hard-coded it to be V1, and no option to override. With work in progress on implementing support for Windows in BuildKit, we should remove this hardcoded assumption, and allow the default to be overridden to advertise that BuildKit is supported. Note that BuildKit on Windows is still very much a "work in progress", and enabling it in the daemon may not even work, so users should not try to enable this feature; a warning-level log is added to make it visible that the feature is enabled. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
builder_allow_buildkit
branch
from
2d59090
to
6943a5a
Compare
|
@vvoland PTAL; I think this is generally "safe" (requires the user to set these options, so no change in any defaults). |
|
I wonder if that would break Docker Desktop users. I think we always set the |
|
Hm... good one; is that same configuration used for the Windows and Linux daemon? At least we must remove that option from Docker Desktop, as it's for sure no longer needed on Linux. |
|
Temporarily moving back to draft while we figure out exactly when / in what cases Docker Desktop sets Alternatively, I could add an |
api/server/router/build: BuilderVersion: allow buildkit on Windows
Commit 7b153b9 changed the daemon to
advertise the recommended builder to use to V2 (BuildKit) for Linux
daemons, and V1 (Legacy Builder) for Windows daemons. For Linux daemons
we allowed the default to be overridden through the "features" field
in the daemon config (daemon.json), but for Windows we hard-coded it
to be V1, and no option to override.
With work in progress on implementing support for Windows in BuildKit,
we should remove this hardcoded assumption, and allow the default to
be overridden to advertise that BuildKit is supported.
Note that BuildKit on Windows is still very much a "work in progress",
and enabling it in the daemon may not even work, so users should not
try to enable this feature; a warning-level log is added to make it
visible that the feature is enabled.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)