-
-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
docker-buildx: symlink plugin to brew owned cli-plugins #162200
Conversation
Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request. |
09371e5
to
0b5ba03
Compare
Formula/d/docker-buildx.rb
Outdated
@@ -32,11 +33,18 @@ def install | |||
generate_completions_from_executable(bin/"docker-buildx", "completion") | |||
end | |||
|
|||
def post_install | |||
cli_plugins = HOMEBREW_PREFIX/"lib/docker/cli-plugins" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you motivate this choice of path? Is lib/docker/cli-plugins
a "well-known" path (in some sense)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, following the original design docker/cli#1534 docker-cli scans these paths, when it searches for plugins:
Unix-like OSes:
$HOME/.docker/cli-plugins
/usr/local/lib/docker/cli-plugins
&/usr/local/libexec/docker/cli-plugins
/usr/lib/docker/cli-plugins
&/usr/libexec/docker/cli-plugins
I think it's reasonable to keep Docker CLI's plugins, that brew manages, in the similar structure.
Note, Docker Desktop seems to amend the default list of the paths and bundles several cli-plugins into the distribution itself (e.g. buildx and compose live inside the docker desktop's bundle).
Formula/d/docker-buildx.rb
Outdated
@@ -32,11 +33,18 @@ def install | |||
generate_completions_from_executable(bin/"docker-buildx", "completion") | |||
end | |||
|
|||
def post_install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably replace this post_install
entirely by doing something like
(lib/"docker/cli-plugins").install_symlink bin/"docker-buildx"
Any reason why we're not doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's indeed simpler; I updated the changes. PHAL
Signed-off-by: Vladimir Varankin <[email protected]>
0b5ba03
to
3860e91
Compare
@narqo thanks for your first contribution to homebrew-core! 🎉 |
🤖 An automated task has requested bottles to be published to this PR. |
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?The PR updates the existing formula with
docker-buildx
Docker plugin, adding a post install step, that links the plugin into#{HOMEBREW_PREFIX}/lib/docker/cli-plugins
directory. Users are expected to add the directory to the list ofcliPluginsExtraDirs
in their docker-cli's config.E.g. this is how it looks in my config.json
I find this behaviour much more in-line with how managed packages should work. That is, as a user I find it more natural to update my local config, to point to a managed directory with plugins, than symlinking the managed plugin into my home directory (i.e. the previous suggestion from "caveats").
Note on Docker's
cliPluginsExtraDirs
, this parameter was added to docker-cli more than 4 years ago, but the fields of the config aren't very well documented it seems.