Add HelperBinariesDir field to engine config #758

Luap99 · 2021-09-08T11:12:02Z

This field contains a list of directories which should be used to store
some helper binaries, e.g. gvproxy.

Also add a FindHelperBinary method to the config struct to get the full
path to a helper binary.

Signed-off-by: Paul Holzinger [email protected]

openshift-ci · 2021-09-08T11:12:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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]

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

Luap99 · 2021-09-08T11:16:00Z

@baude @rhatdan @vrothberg @mheon @ashley-cui PTAL
I think this is the best solution for the the gvproxy path problem. I think having this in the containers.conf file makes the most sense, given that it is likely that we need more helper binaries in the future (new network stack and also maybe rootlessport).

vrothberg

Two mini nits but LGTM

Nice idea!

vrothberg · 2021-09-08T11:40:42Z

pkg/config/containers.conf

+# A is a list of directories which are used to search for helper binaries.
+#
+#helper_binaries_dir = [
+#  "/usr/local/libexec/podman",


Indentation is off in the first entry.

This is intentional, check the other config options in the file.

vrothberg · 2021-09-08T11:41:12Z

pkg/config/config_darwin.go

@@ -15,3 +15,11 @@ func customConfigFile() (string, error) {
 func ifRootlessConfigPath() (string, error) {
 	return rootlessConfigPath()
 }
+
+var defaultHelperBinariesDir = []string{
+	// FIXME: What paths can we use on mac?


/usr/libexec seems to exist on Mac OS, so we could make that the first entry.

Yes I have no idea about what paths can be used for mac or where homebrew install this ATM. That's why I marked this as draft.

At the moment, Homebrew installs gvproxy into /usr/local/bin or /opt/homebrew/bin (depending... long story), but the general consensus seems that we no longer want gvproxy to be in the executable path. The next-best option seems to be to have Homebrew leave it in-key at ${HOMEBREW_PREFIX}/Cellar/podman/${version}/libexec, but you can see that that version in the middle of the keg means we'll just need to put out a containers.conf file with the configured directory as part of the Homebrew install anyway. So, for the moment, I'd not worry about the Mac for the default list -- what's here LGTM for defaults.

Which leads me to the search path for containers.conf ... reviewing that now.

I reread (again) the Homebrew Formula Cookbook and discovered "the opt prefix". If we add /usr/local/opt/podman/libexec, /opt/homebrew/bin, and /opt/homebrew/opt/podman/lib exec to the defaults list, we should be okay.

@Luap99 Are you good to add the 3 directories above or shall I file a PR against your repo?

I will add them, thanks for the list.

jonpspri · 2021-09-08T12:42:56Z

We're getting there, but I still have some concerns about hard-coded paths. Looking at

common/pkg/config/config.go

Lines 21 to 31 in 0be74e4

    
           const ( 
        
           	// _configPath is the path to the containers/containers.conf 
        
           	// inside a given config directory. 
        
           	_configPath = "containers/containers.conf" 
        
           	// DefaultContainersConfig holds the default containers config path 
        
           	DefaultContainersConfig = "/usr/share/" + _configPath 
        
           	// OverrideContainersConfig holds the default config path overridden by the root user 
        
           	OverrideContainersConfig = "/etc/" + _configPath 
        
           	// UserOverrideContainersConfig holds the containers config path overridden by the rootless user 
        
           	UserOverrideContainersConfig = ".config/" + _configPath 
        
           )

, it seems the System and Override containers.conf locations are hard-coded and do not respect install prefixes. While we could just punt and say the environment variable CONTAINERS_CONF needs to be set on the Mac, I'd prefer to be able to create system configuration files and install them from Homebrew and other package management systems.

Heaven forefend we have to do a patch during that build :)

Luap99 · 2021-09-08T12:51:17Z

@jonpspri Can we just put the config file in ~/.config/containers/containers.conf?

jonpspri · 2021-09-08T12:53:43Z

Homebrew spec says it should got to ${HOMEBREW_PREFIX}/usr/share/containers/containers.conf and to ${HOMEBREW_PREFIX}/etc/containers/containers.conf, which is what I think the installer will support. I need to run out now... I'll do some checking later. I still think we should use a configurable prefix, even if we reverse-engineer it based on where the binary is installed.

jonpspri · 2021-09-08T23:15:43Z

Given that we can work with the path list in the code (for now), I suggest we defer the "where do the configurations live" conversation to a separate issue.

jonpspri

Just a problem with /usr/local/bin needing to be there for the current x86_64 Homebrew install. I think it disappeared through a lack of clarity in my comment.

pkg/config/config_darwin.go

docs/containers.conf.5.md

This field contains a list of directories which should be used to store some helper binaries, e.g. gvproxy. Also add a FindHelperBinary method to the config struct to get the full path to a helper binary. Signed-off-by: Paul Holzinger <[email protected]>

Luap99 · 2021-09-10T11:39:59Z

@jonpspri updated with /usr/local/bin

rhatdan · 2021-09-10T11:56:38Z

LGTM

rhatdan · 2021-09-11T08:48:42Z

/lgtm

simnalamburt · 2021-09-11T08:50:36Z

Once we release a new version of containers/common, both podman main branch and v3.3 branch will be ready to support macOS Apple silicon

rhatdan · 2021-09-13T14:33:31Z

https://github.com/containers/common/releases/tag/v0.44.0

openshift-ci bot added the do-not-merge/work-in-progress label Sep 8, 2021

openshift-ci bot added the approved label Sep 8, 2021

Luap99 mentioned this pull request Sep 8, 2021

Use Make GVPROXY_PATH to locate gvproxy containers/podman#11480

Closed

vrothberg reviewed Sep 8, 2021

View reviewed changes

jonpspri mentioned this pull request Sep 9, 2021

Make gvproxy path configurable with containers.conf containers/podman#11449

Merged

Luap99 force-pushed the helperBinDir branch from b178e13 to 5ee26cc Compare September 10, 2021 08:02

Luap99 marked this pull request as ready for review September 10, 2021 08:03

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 10, 2021

jonpspri suggested changes Sep 10, 2021

View reviewed changes

pkg/config/config_darwin.go Show resolved Hide resolved

docs/containers.conf.5.md Show resolved Hide resolved

Luap99 force-pushed the helperBinDir branch from 5ee26cc to 04a59d4 Compare September 10, 2021 11:39

simnalamburt mentioned this pull request Sep 10, 2021

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

Closed

18 tasks

openshift-ci bot assigned rhatdan Sep 11, 2021

openshift-ci bot added the lgtm label Sep 11, 2021

openshift-merge-robot merged commit 2d46695 into containers:main Sep 11, 2021

Luap99 deleted the helperBinDir branch September 11, 2021 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HelperBinariesDir field to engine config #758

Add HelperBinariesDir field to engine config #758

Luap99 commented Sep 8, 2021

openshift-ci bot commented Sep 8, 2021

Luap99 commented Sep 8, 2021

vrothberg left a comment

vrothberg Sep 8, 2021

Luap99 Sep 8, 2021

vrothberg Sep 8, 2021

Luap99 Sep 8, 2021

jonpspri Sep 8, 2021 •

edited

Loading

jonpspri Sep 8, 2021 •

edited

Loading

jonpspri Sep 9, 2021

Luap99 Sep 9, 2021

jonpspri commented Sep 8, 2021 •

edited

Loading

Luap99 commented Sep 8, 2021

jonpspri commented Sep 8, 2021

jonpspri commented Sep 8, 2021

jonpspri left a comment

Luap99 commented Sep 10, 2021

rhatdan commented Sep 10, 2021

rhatdan commented Sep 11, 2021

simnalamburt commented Sep 11, 2021

rhatdan commented Sep 13, 2021

Add HelperBinariesDir field to engine config #758

Add HelperBinariesDir field to engine config #758

Conversation

Luap99 commented Sep 8, 2021

openshift-ci bot commented Sep 8, 2021

Luap99 commented Sep 8, 2021

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Sep 8, 2021

Choose a reason for hiding this comment

Luap99 Sep 8, 2021

Choose a reason for hiding this comment

vrothberg Sep 8, 2021

Choose a reason for hiding this comment

Luap99 Sep 8, 2021

Choose a reason for hiding this comment

jonpspri Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

jonpspri Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

jonpspri Sep 9, 2021

Choose a reason for hiding this comment

Luap99 Sep 9, 2021

Choose a reason for hiding this comment

jonpspri commented Sep 8, 2021 • edited Loading

Luap99 commented Sep 8, 2021

jonpspri commented Sep 8, 2021

jonpspri commented Sep 8, 2021

jonpspri left a comment

Choose a reason for hiding this comment

Luap99 commented Sep 10, 2021

rhatdan commented Sep 10, 2021

rhatdan commented Sep 11, 2021

simnalamburt commented Sep 11, 2021

rhatdan commented Sep 13, 2021

jonpspri Sep 8, 2021 •

edited

Loading

jonpspri Sep 8, 2021 •

edited

Loading

jonpspri commented Sep 8, 2021 •

edited

Loading