Skip to content

remove buildkit as dependency from the CLI (integrate github.com/moby/buildkit/util/appcontext) - #4457

Merged
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:no_buildkit
Sep 27, 2023
Merged

remove buildkit as dependency from the CLI (integrate github.com/moby/buildkit/util/appcontext)#4457
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:no_buildkit

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Jul 20, 2023

Copy link
Copy Markdown
Member

This copies the github.com/moby/buildkit/util/appcontext
package as an internal package. The appcontext package from
BuildKit was the only remaining dependency on BuildKit, and
while we may need some of its functionality, the implementation
is not correct for how it's used in docker/cli (so would need
a rewrite).

Moving a copy of the code into the docker/cli (but as internal
package to prevent others from depending on it) is a first step
in that process, and removes the circular dependency between
BuildKit and the CLi.

We are only using these:

tree vendor/github.com/moby/buildkit
vendor/github.com/moby/buildkit
├── AUTHORS
├── LICENSE
└── util
└── appcontext
├── appcontext.go
├── appcontext_unix.go
├── appcontext_windows.go
└── register.go
3 directories, 6 files

Before this:

go mod graph | grep ' github.com/docker/cli'
github.com/moby/buildkit@v0.11.6 github.com/docker/cli@v23.0.0-rc.1+incompatible

After this:

go mod graph | grep ' github.com/docker/cli'
# (nothing)

But indirect dependencies and circular dependency on moby/moby -> buildkit -> moby/moby and docker/cli can cause "hell".

- A picture of a cute animal (not mandatory but encouraged)

Comment threadcmd/docker/appcontext/appcontext.go
@codecov-commenter

codecov-commenter commented Aug 23, 2023

Copy link
Copy Markdown

Codecov Report

Merging #4457 (112d79a) into master (35442a6) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@ Coverage Diff @@## master #4457 +/- ##
=======================================
Coverage 59.70% 59.70% =======================================
Files 288 288 Lines 24816 24816 =======================================
Hits 14817 14817 Misses 9113 9113 Partials 886 886 

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Quick blurb, because we discussed this in one of the maintainers calls with @tonistiigi@neersighted (and others, but they may recall the exact details);

  • the current use of appcontext in the CLI is not correct (also see plugin sigterm handling behavior is odd #4402)
  • ... but we may want to keep parts of the behavior
  • and.. this is where my memory fails me 🙈 😊 because I don't recall the exact parts we should keep and which parts to remove
  • I think the "CTRL-C x3 to force stop" was something that was desirable
  • But ... I forgot the other details 🙈

@thaJeztahthaJeztah changed the title WIP: remove buildkit as dependency from the CLIremove buildkit as dependency from the CLI (integrate github.com/moby/buildkit/util/appcontext)Sep 27, 2023
@thaJeztah
thaJeztah marked this pull request as ready for review September 27, 2023 21:32
@thaJeztahthaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Sep 27, 2023
@thaJeztahthaJeztah added this to the 25.0.0 milestone Sep 27, 2023
This copies the github.com/moby/buildkit/util/appcontext
package as an internal package. The appcontext package from
BuildKit was the only remaining dependency on BuildKit, and
while we may need some of its functionality, the implementation
is not correct for how it's used in docker/cli (so would need
a rewrite).
Moving a copy of the code into the docker/cli (but as internal
package to prevent others from depending on it) is a first step
in that process, and removes the circular dependency between
BuildKit and the CLi.
We are only using these:
tree vendor/github.com/moby/buildkit
vendor/github.com/moby/buildkit
├── AUTHORS
├── LICENSE
└── util
└── appcontext
├── appcontext.go
├── appcontext_unix.go
├── appcontext_windows.go
└── register.go
3 directories, 6 files
Before this:
go mod graph | grep ' github.com/docker/cli'
github.com/moby/buildkit@v0.11.6 github.com/docker/cli@v23.0.0-rc.1+incompatible
After this:
go mod graph | grep ' github.com/docker/cli'
# (nothing)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@tonistiigi@neersighted PTAL; this keeps the status-quo on the functionality and does not fix the issues that were discussed about appcontext, but does remove the BuildKit dependency, which I think may be worth considering.

I updated the PR to move the appcontext package to internal (at least until we fixed the issues that were discussed).

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Let me bring this one in; we can use #4402 as tracking issue for the remaining work

@thaJeztah
thaJeztah merged commit 9358631 into docker:masterSep 27, 2023
@thaJeztah
thaJeztah deleted the no_buildkit branch September 27, 2023 22:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactorPR's that refactor, or clean-up codestatus/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah@codecov-commenter@neersighted