Prioritize addtionalHelperBinariesDir over default dirs

[ashley-cui]

When addtionalHelperBinariesDir is set, first search that path for helper binaries, then the default locations.

Closes: containers/podman#20808

[ashley-cui]

Tested by building with HELPER_BINARIES_DIR="/opt/podman/qemu/bin"

With this fix:

acui@Ashleys-Mini podman % ps aux | grep qemu
acui             12489   0.2  0.3 412252816  22240 s002  S     1:51PM   1:05.48 /opt/podman/qemu/bin/qemu-system-aarch64

Without this fix:

acui@Ashleys-Mini podman % ps aux | grep qemu  
acui             13480   2.0  0.2 412195168  18208 s002  S     2:06PM   1:03.42 /opt/homebrew/bin/qemu-system-aarch64--

[ashley-cui]

@benoitf PTAL

[rhatdan]

LGTM
/approve
@containers/podman-maintainers PTAL
This should be back ported

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, 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:

• OWNERS [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

@cfergeau PTAL

[mheon]

Sure, LGTM

[cfergeau]

I would also change

 var defaultHelperBinariesDir = []string{
+       // Relative to the binary directory
+       "$BINDIR/../libexec/podman",
        // Homebrew install paths
        "/usr/local/opt/podman/libexec/podman",
        "/opt/homebrew/opt/podman/libexec/podman",
@@ -42,6 +44,4 @@ var defaultHelperBinariesDir = []string{
        "/usr/local/lib/podman",
        "/usr/libexec/podman",
        "/usr/lib/podman",
-       // Relative to the binary directory
-       "$BINDIR/../libexec/podman",
 }

but also LGTM

[benoitf]

hello, I patched the file in podman vendor/github.com/containers/common/pkg/config/default.go and I did create the installer but it does work only for gvproxy in my case

I used

make ARCH=aarch64 NO_CODESIGN=1 pkginstaller

then installed it

if I run it, now I can see that it's using gvproxy from the /opt/podman/bin folder but qemu is still launched from brew

benoitf          50363  37.9  2.0 441821360 1987296 s002  S    10:55AM   0:26.68 /opt/homebrew/bin/qemu-system-aarch64 -m 30895 -smp 5 -fw_cfg ....
benoitf          50362   0.2  0.0 409236784  31904 s002  S    10:55AM   0:00.11 /opt/podman/qemu/bin/gvproxy -debug -mtu 1500 ....

also I'm not sure to follow why it's using /opt/podman/qemu/bin/gvproxy and not /opt/podman/bin/gvproxy but at least it's using one binary from the installer and not from the system

[benoitf]

update: it works with a fresh machine, but not with an existing machine so somehow the path to qemu is persisted in the machine configuration

I don't know if it's a feature or a bug but I would expect that it should not store the path to the qemu binary

[ashley-cui]

@cfergeau Fixed, thanks!

@benoitf Currently we do store the path to the binary in the config. I think finding the binary on start time and not init time is something we should consider, but that work is in podman anyway, so it wouldn't be fixed by this PR. Maybe we open an issue on that?

[cfergeau]

/lgtm

[openshift-ci]

@cfergeau: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[baude]

I think finding the binary on start time and not init time is something we should consider,

this will be part of podman 5; i dont intend to change the behavior of the config files for podman 4.x. while this seems obvious to do this not that PD is part of the equation, the fully qualified binary path was done to protect against the opposite of this problem ... that is to say, a new qemu binary is found somewhere and chosen over the known working one.

[baude]

/lgtm

[benoitf]

I'm fine to not have that in 4.x but if it's taken into account for 5.x

[benoitf]

@ashley-cui do you know when it'll land into podman main branch (because it's using an old version of containers/common there)

[baude]

@ashley-cui do you know when it'll land into podman main branch (because it's using an old version of containers/common there)

it can be vendored whenever you want ... the main branch is podman 5 now

[ashley-cui]

@benoitf containers/podman#20836

[benoitf]

@baude yes I'm using podman main branch this week (5.0.0-dev) 👍 (just I think it's more convenient when the patch is already included and not having to include it by myself)

@ashley-cui thanks

[benoitf]

hello it does not seem to have been backported to 4.8.x branch

[baude]

was it asked for ?

[baude]

actually history suggestions you were fine with that ??

[benoitf]

it has been asked there
#1754 (comment)

I replied that I was OK for the storage of qemu binary location in the machine definition for 4.x and that it could wait for 5.x

[Luap99]

/cherry-pick v0.57