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

Makefile: do not prefix /etc #18257

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Conversation

vrothberg
Copy link
Member

Revert commit 3d0e08f. /etc/ does not need a prefix and can be customized with the ETCDIR env variable.

Fixes: #18250

Does this PR introduce a user-facing change?

None

Revert commit 3d0e08f.
`/etc/` does not need a prefix and can be customized
with the `ETCDIR` env variable.

Fixes: containers#18250
Signed-off-by: Valentin Rothberg <[email protected]>
@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 18, 2023
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@edsantiago
Copy link
Member

@tobwen PTAL, this reverts your PR #10786. I never understood the point of that PR, but if you have a compelling reason for insisting that it remain in place, please speak now.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vrothberg
Copy link
Member Author

@tobwen PTAL, this reverts your PR #10786. I never understood the point of that PR, but if you have a compelling reason for insisting that it remain in place, please speak now.

Thanks for pulling in @tobwen, Ed!

@tobwen
Copy link
Contributor

tobwen commented Apr 18, 2023

IMHO, in the GNU Makefile Conventions, there is nothing like $ETCDIR. The closest thing to it would be sysconfdir, but: [...] This directory should normally be /usr/local/etc, but write it as $(prefix)/etc. (If you are using Autoconf, write it as ‘@sysconfdir@’.)

I haven't seen an application in the last years that doesn't automatically push etc/ under ${prefix}. I'm using GNU Stow to version my self-built software, so I'm expecting to have my stuff under /usr/local/stow/podman-x.xx. In many applications, make install DESTDIR= has site-effects, since the target directory need to be known before building (for example, when scripts are written).

However, I do not want to stand in the way of your project. And if $ETCDIR is to be used, I have to take it into account when building in the future (if necessary, an entry should also be made in INSTALL.md, so that when building and installing as root, /etc/ doesn't get overwritten unexpectedly, as it was my case.

@edsantiago
Copy link
Member

ping, to let everyone know that @tobwen has responded (but did so via an edit, not a post, so it didn't go to anyone's email). Read above.

Also, I'm waffling a lot on this PR. I believe we need to keep $PREFIX/etc because there are an unknown number of users out there, with no root access but permissions to install into /usr/local, and for those it would be impossible to install into /etc. But then: will podman read /usr/local/etc/ on those systems? I don't think so, because this is just make, not ./configure && make, and I see no mechanism for overriding /etc in podman itself. So then removing $PREFIX is the correct thing to do.

Out of time. This can continue tomorrow.

@vrothberg
Copy link
Member Author

But then: will podman read /usr/local/etc/ on those systems?

Not consistently, no. It may work for containers.conf but for this config only. But the docker.in is sensitive to it.

Since commit 1edada4 we no longer install 87-podman-bridge.conflist, so we can scratch that off the list. I don't think anything is installed to /etc anymore.

With respect to docker.in: Wouldn't PREFIX=/foo ETCDIR=/foo/etc make ... get the desired behavior? I wonder whether this path needs to configurable at all. Which brings me to the next question: Do we still need a configurable /etc?

@Luap99
Copy link
Member

Luap99 commented Apr 19, 2023

Whenever we need a customizable ETCDIR should not be the discussion here. The binaries never respected it anyway. The original problem as @vrothberg points out is fixed we no longer install anything into /etc so this change should not cause problems.

@vrothberg
Copy link
Member Author

Whenever we need a customizable ETCDIR should not be the discussion here. The binaries never respected it anyway.

I am not that sure. The behavior of docker.in is sensitive to this setting.

The binaries never respected it anyway.

Podman did to some degree. containers.conf is the only one AFAICS.

I am not advertising for doing it but I also don't want this conversation to be shot down too quickly.

@Luap99
Copy link
Member

Luap99 commented Apr 19, 2023

I am not saying we shouldn't have this discussion, but this PR is not the place to have it IMO.
We caused a regression this fixes it. The docker.in is the only file using ETCDIR so I see no problem with this patch.
The alternative would be to revert the docker.in change which is IMO much worse.

Podman did to some degree. containers.conf is the only one AFAICS.

containers.conf is defined in c/common. The podman build command sets X github.com/containers/podman/v4/libpod/config._etcDir=/usr/local/etc which does not exist. So I do not see how any config file ever used this path. Maybe the libpod.conf, this was before my time?

@tobwen
Copy link
Contributor

tobwen commented Apr 19, 2023

Dar fellas, as I said, feel free to discard my old PR. I don't want to block a PR, nor do I want to start trouble.

@vrothberg
Copy link
Member Author

@rhatdan PTAL

@vrothberg
Copy link
Member Author

So, can we merge?

@Luap99
Copy link
Member

Luap99 commented Apr 24, 2023

@containers/podman-maintainers PTAL

@edsantiago
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2023
@openshift-ci openshift-ci bot merged commit 3c53621 into containers:main Apr 24, 2023
@github-actions 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 Aug 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Podman looks for config files in /usr/etc as opposed to /etc
4 participants