config: Allow specifying helper dirs with $BINDIR as base directory #1127
Conversation
pkg/config/config.go
Outdated
| if path == bindirPrefix || strings.HasPrefix(path, bindirPrefix+string(filepath.Separator)) { | ||
| // Calculate the path to the executable first time we encounter a $BINDIR prefix. | ||
| if bindirPath == "" && bindirErr == nil { | ||
| podman, bindirErr := os.Executable() |
There was a problem hiding this comment.
nit: This should not be called podman, since this library is used for other tools like buildah.
Just call it executable
There was a problem hiding this comment.
Sure that makes sense, I'm using execPath now.
(It's a bit odd though, since config_darwin.go in this same package uses libexec/podman for the helper directory, but I guess that's perhaps expected since the helpers are somewhat shared between both?)
|
Thanks @filbranden |
pkg/config/config.go
Outdated
| @@ -1261,6 +1285,9 @@ func (c *Config) FindHelperBinary(name string, searchPATH bool) (string, error) | |||
| return exec.LookPath(name) | |||
| } | |||
| configHint := "To resolve this error, set the helper_binaries_dir key in the `[engine]` section of containers.conf to the directory containing your helper binaries." | |||
| if bindirErr != nil { | |||
| configHint = fmt.Sprintf("Error looking up path relative to %s: [%s] %s", bindirPrefix, bindirErr, configHint) | |||
There was a problem hiding this comment.
Should this be a logrus.Warnf instead of a print?
There was a problem hiding this comment.
Oh yes indeed logging is much nicer!
By not having to track the error result, I ended up refactoring the code considerably, moving bindirPath calculation to a standalone function.
In the new code, I also track whether os.Executable() (or resolving symlinks from it) ever failed, in order to only log about it once.
I hope you still like how the new code looks, let me know if you'd like me to make any changes to it.
|
Thanks @filbranden ! A nit from Dan and a nit from me, but other than that, LGTM! |
|
Updated the PR. One case I had previously missed was that the called binary is actually a symbolic link, now I added a call to Also split it into two separate commits, one with the feature to support the PTAL. Thanks! |
filbranden
force-pushed
the
bindir1
branch
3 times, most recently
from
3b63438 to
8f28dc8
Compare
This should make it easier to locate helper binaries relative to where the main binary was installed, which should be useful in installations such as Homebrew which install packages under a versioned directory. Use a `$BINDIR` magic token as a prefix in the helper path to indicate it should be relative to the directory where the binary is located. This is somewhat familiar to the syntax used in the shell and Makefile and is still quite explicit about the behavior (as opposed to, say, making all relative paths be relative to the directory of the binary.) Tested: After updating the Darwin config to include a `$BINDIR/../libexec/podman` directory, updated vendored package in podman, built it with `podman-remote`, copied `gvproxy` to a `libexec/podman` at the same level as `bin/podman` and confirmed that `podman machine start` worked as expected. Also confirmed that having the `podman` in search path be a symlink to a binary elsewhere works as expected, the searched `../libexec/podman` directory is relative to the actual binary and not the symlink (which matches the Homebrew use case.) Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
Look for helpers such as `gvproxy` under `$BINDIR/../libexec/podman`, which helps this work on distributions such as Homebrew without the need to hardcode paths into the binary, which makes a pre-built binary distribution work regardless of the installation prefix. Tested: Updated vendored package in podman, built it with `podman-remote`, copied `gvproxy` to a `libexec/podman` at the same level as `bin/podman` and confirmed that `podman machine start` worked as expected. Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: filbranden, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…helpers relative to the main binary PR containers/common Homebrew#1127 (containers/common#1127) introduced a new feature by which podman is now capable of locating its helpers (such as `gvproxy`) relative to the main binary location, and by default enables that feature for MacOS builds. Apply the patch on top of the current Homebrew tree and enable the feature. Move `gvproxy` from a bare `libexec` to a `libexec/podman`, since that's the default path to the binary used in podman. For Linux builds, use the environment variable consumed by the Makefile to inject the same binary-relative path to the default helper search list. Since this change makes this keg relocatable again, drop the `pour_bottle? only_if:` gate that was causing it to build from source whenever using a non-standard prefix. Bump up revision number. Installing from bottles should now work fine, even on a non-standard prefix. Tested: Rebuilt from source with `brew reinstall -s podman` and confirmed that `podman machine start` works (that's the operation that depends on the location of the `gvproxy` binary.) Tested a different directory location to test (on a Mac) that the Linux change should work as exepcted.
…helpers relative to the main binary PR containers/common #1127 (containers/common#1127) introduced a new feature by which podman is now capable of locating its helpers (such as `gvproxy`) relative to the main binary location, and by default enables that feature for MacOS builds. Apply the patch on top of the current Homebrew tree and enable the feature. Move `gvproxy` from a bare `libexec` to a `libexec/podman`, since that's the default path to the binary used in podman. For Linux builds, use the environment variable consumed by the Makefile to inject the same binary-relative path to the default helper search list. Since this change makes this keg relocatable again, drop the `pour_bottle? only_if:` gate that was causing it to build from source whenever using a non-standard prefix. Bump up revision number. Installing from bottles should now work fine, even on a non-standard prefix. Tested: Rebuilt from source with `brew reinstall -s podman` and confirmed that `podman machine start` works (that's the operation that depends on the location of the `gvproxy` binary.) Tested a different directory location to test (on a Mac) that the Linux change should work as exepcted. Closes #109241. Signed-off-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com> Signed-off-by: BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>
This should make it easier to locate helper binaries relative to where the main binary was installed, which should be useful in installations such as Homebrew which install packages under a versioned directory.
Use a
$BINDIRmagic token as a prefix in the helper path to indicate it should be relative to the directory where the binary is located. This is somewhat familiar to the syntax used in the shell and Makefile and is still quite explicit about the behavior (as opposed to, say, making all relative paths be relative to the directory of the binary.)Update
podmanconfig on Darwin to look for helpers such asgvproxyunder$BINDIR/../libexec/podman, which is the ultimate objective of this code change.Tested: Updated vendored package in podman, built it with
podman-remote, copiedgvproxyto alibexec/podmanat the same level asbin/podmanand confirmed thatpodman machine startworked as expected.Fixes: containers/podman#12161
Related: PR #946 (cc @ashley-cui)
Signed-off-by: Filipe Brandenburger filbranden@gmail.com