Skip to content
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

Respect $PREFIX for config files paths #1298

Closed
wants to merge 1 commit into from

Conversation

mateuszkwiatkowski
Copy link
Contributor

Handling $PREFIX for binaries and documentation was fixed in
#1168.
This commit puts configuration files to $PREFIX/etc/containers.

Fixes #787

Handling $PREFIX for binaries and documentation was fixed in
containers#1168.
This commit puts configuration files to $PREFIX/etc/containers.

Fixes containers#787
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I am a bit worried about overriding the paths unconditionally. If we run into the hypothetical situation of changing defaults in c/image, Skopeo would continue using the defaults from its Makefile.

@mtrmac WDYT?

@mateuszkwiatkowski
Copy link
Contributor Author

@vrothberg I agree this is not ideal solution and it already bitten homebrew maintainers as described in #787. The good thing is it's now consistent with other paths, the bad is it has to reach that deep to vendored package. I'm open to other solutions of this problem but unfortunately I'm not proficient in Go build system so help would be needed. :-)

@mtrmac
Copy link
Contributor

mtrmac commented May 27, 2021

I am a bit worried about overriding the paths unconditionally. If we run into the hypothetical situation of changing defaults in c/image, Skopeo would continue using the defaults from its Makefile.

Well, that’s not really hypothetical at all:

const builtinRegistriesConfPath = "/etc/containers/registries.conf"

vs. this PR which would use /usr/local/etc/containers by default.

There are three aspects to this:


#787 is a very valid request; that does not, AFAICS, require changing any file locations. The -X … arguments can be added without moving to /usr/local/etc, so that’s what I think this PR should do, and we can discuss the move to /usr/local/etc (and compatibility impact), separately. (Users can override the make variables in any case.)


WRT unconditionally overriding — if Skopeo is already installing default files in the paths decided in its own Makefile, we need to keep Skopeo’s Makefile and c/image in sync already, both for Skopeo to work and for Buildah/Podman to work. That’s not a new concern, in that sense.

It would still be very nice to add an automated tests for this, of some kind, maybe very vaguely (probably doesn’t work at all):

# use this everywhere
POLICY_JSON_PATH := ${CONTAINERSCONFDIR}/policy.json

test-path-consistency:
    somehow_with_empty_environment_and_no_overrides $(MAKE) test-path-consistency-internal

# or maybe just a script in hack/* that gets passed POLICY_JSON_PATH and all the others?
test-path-consistency-internal: 
    # Actually parse as Go, or run some kind of a helper program??
    # This is not a correct Go parser, but works well enough to highlight the need to keep consistency.
    grep -q 'const builtinDefaultPolicyPath = "$(POLICY_JSON_PATH)"' vendor/github.com/containers/image/v5/signature/*.go

… if someone had time to make that actually work. (An architecture-astronaut approach would be to maintain the mapping of package+variable name vs. make variable name only once, and apply it both for the -X flags and this check, but that seems way more trouble than is warranted.)

(I don’t think this automated test needs to be be a blocker for this PR.)

@rhatdan
Copy link
Member

rhatdan commented Jun 25, 2021

What do we want to do with this PR?

@mtrmac
Copy link
Contributor

mtrmac commented Jun 28, 2021

  • Definitely drop the parts that change the default path from /etc/containers to /usr/local/etc/containers
  • Maybe the automated test as suggested above. I don’t think it’s a blocker but it would certainly give us peace of mind.

@lsm5
Copy link
Member

lsm5 commented Jul 20, 2021

ping @mateuszkwiatkowski

@rhatdan
Copy link
Member

rhatdan commented Jul 30, 2021

Most of the time when we install we set PREFIX=/usr, which would mean the config files would end up in /usr/etc/containers, which is clearly broken.

@mateuszkwiatkowski Unless we can work around that problem, I say we close this PR.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@lsm5
Copy link
Member

lsm5 commented Aug 30, 2021

Closing until we hear further from @mateuszkwiatkowski or anyone willing to take this PR

@mateuszkwiatkowski
Copy link
Contributor Author

I'm sorry, I didn't look into project involved with skopeo since then. I understand your concerns and currently don't have resources to work on this further. I see that work continue at #1507.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help for overriding paths during build
5 participants