Make gvproxy path configurable with containers.conf #11449

simnalamburt · 2021-09-04T16:43:47Z

macOS does not have /usr/libexec/ so we look in the executable paths first.

Closes #11226
Closes #11531

Blocked by containers/common#758

NOTE: This PR is identical with #11234. Since #11226 still exist in the main branch, I'm sending the PR again.

pkg/machine/qemu/machine.go

jonpspri · 2021-09-05T14:06:29Z

I've been reading through the Issue and PR history of this particular PR and two things strike me:

This is partially a Homebrew issue -- the preference of the Podman team is to have gvproxy installed outside off the exec path, and...
The install path prefix in Podman should not be hardcoded but should be a build-time or run-time configuration. That decision could percolate throughout the code base.

Having said that, we may still need the quick fix to fall back to exec.LookPath while the new approach to gvproxy is percolating through homebrew and podman.

Conan-Kudo · 2021-09-05T14:40:39Z

macOS does not have /usr/libexec/ so we look in the executable paths first.

It definitely does, though...

jonpspri · 2021-09-05T14:53:09Z

@Conan-Kudo It does, but that directory not user-writable on recent OS releases, and even if it were, installing to System directories flies in the face of Homebrew's philosophy. I think longer-term, podman-machine at least will need to support configurable (install) prefixes and/or some search path capability.

baude · 2021-09-05T15:35:41Z

@simnalamburt create different search methods by OS (i.e. Linux and Darwin) and we can have a different approach in each. I personally see no issue with the code in 3.3; it just didnt get into main branch yet. Good luck, ill try to check back but im on PTO.

jonpspri · 2021-09-06T13:17:43Z

Perhaps we should consider something like this: jonpspri@562c720

Rather than do backflips in the code, the build process can set the path as a variable. I think in 95% of cases, the target should be known at build time. For example, I updated my Homebrew install to do make GVPROXY_PATH=/opt/homebrew/bin/gvproxy podman-remote-darwin.

Otherwise, I expect we should start looking to an entry in ~/.config/podman. Dunno what the configuration libraries are...

Thoughts?

(p.s. In the next revision, I'll likely use $(shell grep -m1 ^module ./go.mod | cut -f 2 ) to extract the module into a make variable and reduce some unsightly hardcoding in the Makefile.

jonpspri · 2021-09-06T14:44:24Z

I also read up on the Homebrew standards for installs. Homebrew reserves libexec for its own purposes, hence installing gvproxy into ${HOMEBREW_PREFIX}/bin.

pharaujo · 2021-09-06T17:26:03Z

I also read up on the Homebrew standards for installs. Homebrew reserves libexec for its own purposes, hence installing gvproxy into ${HOMEBREW_PREFIX}/bin.

From my reading of the docs, the libexec directory can definitely be used by formulas, but the binaries placed there just won't be symlinked to HOMEBREW_PREFIX (e.g. /usr/local/libexec). I do think it makes sense to put gvproxy in the formula's libexec directory and have a configurable GVPROXY_PATH set appropriately (e.g. pointing to /usr/local/opt/podman/libexec) at build time.

jonpspri · 2021-09-07T11:09:33Z

@pharaujo I like it. Install into the Keg libexec and use a build-time configuration to direct podman to it. So the make becomes

make GVPROXY_PATH=/opt/homebrew/Cellar/podman/<version>/libexec/gvproxy podman-remote-darwin

I've tested with my formula at https://github.com/jonpspri/homebrew-core/blob/master/Formula/podman.rb while we sort out Pull Requests.

jonpspri · 2021-09-07T21:57:01Z

@ashley-cui ^

Presented as an alternative to PR containers#11449 Rather than do backflips in the code to locate `gvproxy`, use a build-time variable to set the location. That variable can default to `/usr/libexec` but other build packages (_e.g._ Homebrew) can set it to something of their liking. I'll take no offense if the consensus is that we do not want to pollute the build, but we should likely add a runtime configuration parameter as an alternative in that case. Fixes: containers#11226 Signed-off-by: Jonathan Springer <[email protected]>

simnalamburt · 2021-09-09T16:33:39Z

We need a consensus.

Current options:

Look up $PATH first and then /usr/libexec/podman/gvproxy → 38dcd6e
Look up /usr/libexec/podman/gvproxy first and then $PATH → https://github.com/containers/podman/pull/11449/files#r702401547
In Darwin, look up $PATH. Otherwise, use /usr/libexec/podman/gvproxy → https://github.com/containers/podman/pull/11449/files#r702401547, Make gvproxy path configurable with containers.conf #11449 (comment)
Make it build-time configurable → Use Make GVPROXY_PATH to locate gvproxy #11480
Make it run-time configurable → @Luap99 @vrothberg Use Make GVPROXY_PATH to locate gvproxy #11480 (comment)

Which option do you prefer? I'd prefer option 5.

jonpspri · 2021-09-09T16:35:17Z

@simnalamburt I'm also on board for option 5 (as done over in containers/common#758 ).

simnalamburt · 2021-09-09T16:40:46Z

Oh I didn't notice the progress of over there. Knowing that it is set to option 5, I will find out what I can do to help.

jonpspri · 2021-09-09T17:37:06Z

@simnalamburt One thing you could whip out if you have time is that even once containers/common#758 is merged, we'll still need to refer to the config item in the code this PR addresses. Could you put that together in this PR?

simnalamburt · 2021-09-09T17:40:36Z

Sure I’ll watch for that PR! 2021년 9월 10일 (금) 02:37, Jonathan Springer ***@***.***>님이 작성:

…

@simnalamburt <https://github.com/simnalamburt> One thing you could whip out if you have time is that even once containers/common#758 <containers/common#758> is merged, we'll still need to refer to the config item in the code this PR addresses. Could you put that together in this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11449 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB235JYU7DMATM45QB4Y6LUBDWE5ANCNFSM5DNOLQMA> .

simnalamburt · 2021-09-11T09:42:31Z

NOTE: This PR is waiting for the new release of https://github.com/containers/common

Luap99 · 2021-09-13T08:44:50Z

@simnalamburt You can vendor in the main branch.
go get github.com/containers/common@main && make vendor

Luap99 · 2021-09-13T18:23:05Z

The new code is merged into podman #11552. If you rebase you should be able to use config.FindHelperBinary()

baude · 2021-09-13T18:31:07Z

@simnalamburt thanks for your contribution ... can you pleasse rebase this PR and updated with the new method call?

simnalamburt · 2021-09-13T18:32:02Z

Ok I will

baude · 2021-09-13T19:26:47Z

/approve

baude · 2021-09-13T19:27:21Z

Ok I will

and then of course, we will merge it

simnalamburt · 2021-09-13T19:31:40Z

Sorry for the delay, it’s 04:30 AM in KST. I’ll do it in few hours later 2021년 9월 14일 (화) 04:27, Brent Baude ***@***.***>님이 작성:

…

Ok I will and then of course, we will merge it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11449 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB235LKWDBUYU2N2IV3U7TUBZGCJANCNFSM5DNOLQMA> .

simnalamburt · 2021-09-14T08:04:08Z

I've updated and rebased the PR.

BTW, this is the last piece that is needed for podman's Apple Silicon support by default. Yay!

pkg/machine/qemu/machine.go

Luap99 · 2021-09-14T08:08:46Z

@simnalamburt You also have to add [NO TESTS NEEDED] to your commit message.

Closes containers#11531 [NO TESTS NEEDED] Signed-off-by: Hyeon Kim <[email protected]>

Luap99

LGTM

rhatdan · 2021-09-14T08:47:17Z

/approve
/lgtm
/hold

openshift-ci · 2021-09-14T08:47:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, Luap99, rhatdan, simnalamburt

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~~ [Luap99,baude,rhatdan]

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

baude · 2021-09-14T11:07:17Z

/hold cancel

rhatdan reviewed Sep 5, 2021

View reviewed changes

pkg/machine/qemu/machine.go Outdated Show resolved Hide resolved

simnalamburt mentioned this pull request Sep 5, 2021

[macos] podman machine start on Apple M1 uses qemu with hvf which is not yet supported by qemu on aarch64 #10577

Closed

scottschreckengaust mentioned this pull request Sep 6, 2021

Adding -cpu host for qemu for MacOS #11442

Merged

jonpspri mentioned this pull request Sep 8, 2021

Use Make GVPROXY_PATH to locate gvproxy #11480

Closed

simnalamburt mentioned this pull request Sep 10, 2021

Upstream podman's Apple Silicon support status simnalamburt/podman#5

Closed

18 tasks

simnalamburt changed the title ~~Fix gvproxy path search for macos~~ Make gvproxy path configurable with containers.conf Sep 10, 2021

simnalamburt marked this pull request as draft September 10, 2021 20:37

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2021

simnalamburt force-pushed the gvproxy-path branch 2 times, most recently from 98418aa to 13609ff Compare September 14, 2021 07:51

simnalamburt marked this pull request as ready for review September 14, 2021 07:51

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2021

simnalamburt mentioned this pull request Sep 14, 2021

Use "cni" as a default rootless_networking method at least in macOS simnalamburt/podman#4

Open

simnalamburt force-pushed the gvproxy-path branch from 13609ff to 107f06e Compare September 14, 2021 07:57

Luap99 reviewed Sep 14, 2021

View reviewed changes

pkg/machine/qemu/machine.go Outdated Show resolved Hide resolved

Search gvproxy with config.FindHelperBinary()

072b061

Closes containers#11531 [NO TESTS NEEDED] Signed-off-by: Hyeon Kim <[email protected]>

simnalamburt force-pushed the gvproxy-path branch from 107f06e to 072b061 Compare September 14, 2021 08:15

Luap99 approved these changes Sep 14, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2021

openshift-ci bot assigned rhatdan Sep 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2021

openshift-merge-robot merged commit ad26684 into containers:main Sep 14, 2021

StephenWithPH mentioned this pull request Oct 8, 2021

podman machine VM fails to start on MacOS due to gvproxy runtime path resolution NixOS/nixpkgs#141041

Closed

MikeNicholls mentioned this pull request Oct 14, 2021

podman 3.4.0 breaks support for MacPorts' gvproxy location #11960

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make gvproxy path configurable with containers.conf #11449

Make gvproxy path configurable with containers.conf #11449

simnalamburt commented Sep 4, 2021 •

edited

Loading

jonpspri commented Sep 5, 2021

Conan-Kudo commented Sep 5, 2021

jonpspri commented Sep 5, 2021

baude commented Sep 5, 2021

jonpspri commented Sep 6, 2021 •

edited

Loading

jonpspri commented Sep 6, 2021

pharaujo commented Sep 6, 2021

jonpspri commented Sep 7, 2021

jonpspri commented Sep 7, 2021

simnalamburt commented Sep 9, 2021

jonpspri commented Sep 9, 2021 •

edited

Loading

simnalamburt commented Sep 9, 2021

jonpspri commented Sep 9, 2021

simnalamburt commented Sep 9, 2021 via email

simnalamburt commented Sep 11, 2021

Luap99 commented Sep 13, 2021

Luap99 commented Sep 13, 2021

baude commented Sep 13, 2021

simnalamburt commented Sep 13, 2021

baude commented Sep 13, 2021

baude commented Sep 13, 2021

simnalamburt commented Sep 13, 2021 via email

simnalamburt commented Sep 14, 2021

Luap99 commented Sep 14, 2021 •

edited

Loading

Luap99 left a comment

rhatdan commented Sep 14, 2021

openshift-ci bot commented Sep 14, 2021

baude commented Sep 14, 2021

Make gvproxy path configurable with containers.conf #11449

Make gvproxy path configurable with containers.conf #11449

Conversation

simnalamburt commented Sep 4, 2021 • edited Loading

jonpspri commented Sep 5, 2021

Conan-Kudo commented Sep 5, 2021

jonpspri commented Sep 5, 2021

baude commented Sep 5, 2021

jonpspri commented Sep 6, 2021 • edited Loading

jonpspri commented Sep 6, 2021

pharaujo commented Sep 6, 2021

jonpspri commented Sep 7, 2021

jonpspri commented Sep 7, 2021

simnalamburt commented Sep 9, 2021

jonpspri commented Sep 9, 2021 • edited Loading

simnalamburt commented Sep 9, 2021

jonpspri commented Sep 9, 2021

simnalamburt commented Sep 9, 2021 via email

simnalamburt commented Sep 11, 2021

Luap99 commented Sep 13, 2021

Luap99 commented Sep 13, 2021

baude commented Sep 13, 2021

simnalamburt commented Sep 13, 2021

baude commented Sep 13, 2021

baude commented Sep 13, 2021

simnalamburt commented Sep 13, 2021 via email

simnalamburt commented Sep 14, 2021

Luap99 commented Sep 14, 2021 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

rhatdan commented Sep 14, 2021

openshift-ci bot commented Sep 14, 2021

baude commented Sep 14, 2021

simnalamburt commented Sep 4, 2021 •

edited

Loading

jonpspri commented Sep 6, 2021 •

edited

Loading

jonpspri commented Sep 9, 2021 •

edited

Loading

Luap99 commented Sep 14, 2021 •

edited

Loading