build: allow BuildKit to be used on Windows daemons that advertise it #5178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit 6fef143 switched the CLI to use BuildKit by default, but as part of that removed the use of the BuildkitVersion field as returned by Ping. Some follow-up changes in commits e38e6c5 and e7a8748 updated the logic for detecting whether BuildKit should be used or the legacy builder, but hard-coded using the legacy builder for Windows daemons. While Windows / WCOW does not yet support BuildKit by default, there is work in progress to implement it, so we should not hard-code the assumption that a Windows daemon cannot support BuildKit. On the daemon-side, [moby@7b153b9] (Docker v23.0) changed the default as advertised by the daemon to be BuildKit for Linux daemons. That change still hardcoded BuildKit to be unsupported for Windows daemons (and does not yet allow overriding the config), but this may change for future versions of the daemon, or test-builds. This patch: - Re-introduces checks for the BuildkitVersion field in the "Ping" response. - If the Ping response from the daemon advertises that it supports BuildKit, the CLI will now use BuildKit as builder. - If we didn't get a Ping response, or the Ping response did NOT advertise that the daemon supported BuildKit, we continue to use the current defaults (BuildKit for Linux daemons, and the legacy builder for Windows) - Handling of the DOCKER_BUILDKIT environment variable is unchanged; for CLI.BuildKitEnabled, DOCKER_BUILDKIT always takes precedence, and for processBuilder the value is taken into account, but will print a warning when BuildKit is disabled and a Linux daemon is used. For Windows daemons, no warning is printed. [moby@7b153b9]: moby/moby@7b153b9 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
buildkit_windows
branch
from
ae0ff02
to
e5d26a8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5178 +/- ##
==========================================
- Coverage 61.76% 61.73% -0.04%
==========================================
Files 297 294 -3
Lines 20768 20767 -1
==========================================
- Hits 12828 12820 -8
- Misses 7024 7029 +5
- Partials 916 918 +2 |
krissetto
approved these changes
Jun 20, 2024
|
@crazy-max @colinhemmings PTAL - please double check this change |
crazy-max
approved these changes
Jun 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
build: allow BuildKit to be used on Windows daemons that advertise it
Commit 6fef143 switched the CLI to use
BuildKit by default, but as part of that removed the use of the
BuildkitVersion field as returned by Ping.
Some follow-up changes in commits e38e6c5 and
e7a8748 updated the logic for detecting whether
BuildKit should be used or the legacy builder, but hard-coded using the
legacy builder for Windows daemons.
While Windows / WCOW does not yet support BuildKit by default, there is
work in progress to implement it, so we should not hard-code the assumption
that a Windows daemon cannot support BuildKit.
On the daemon-side, moby@7b153b9 (Docker v23.0) changed the default as
advertised by the daemon to be BuildKit for Linux daemons. That change
still hardcoded BuildKit to be unsupported for Windows daemons (and does
not yet allow overriding the config), but this may change for future
versions of the daemon, or test-builds.
This patch:
the CLI will now use BuildKit as builder.
that the daemon supported BuildKit, we continue to use the current
defaults (BuildKit for Linux daemons, and the legacy builder for Windows)
CLI.BuildKitEnabled, DOCKER_BUILDKIT always takes precedence, and for
processBuilder the value is taken into account, but will print a warning
when BuildKit is disabled and a Linux daemon is used. For Windows daemons,
no warning is printed.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)