Unit files: Use actual installed path for podman #11886
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago 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 |
openshift-ci
bot
added
the
approved
edsantiago
force-pushed
the
unitfile_path
branch
from
8f9b1ca to
fe8cc21
Compare
|
Are we going to need to do any packaging work to get this to work? @lsm5 |
|
The RPM task is blowing up, suggesting that we will |
|
Sigh. This is the rathole I was trying to avoid. This means that |
|
I wonder if podman/contrib/spec/podman.spec.in Line 438 in 703db8e |
edsantiago
force-pushed
the
unitfile_path
branch
from
fe8cc21 to
96d79d1
Compare
|
Probably better check the Clarification: This tarball is produced for each of the 'Build for *' tasks (Fedora and Ubuntu). You'll find the tarball in the Cirrus WebUI under the |
|
Update: I examined the F34 tarball (in this PR) and surprisingly, the paths look correct (i.e. they don't reference the temp. dir). |
|
I give up. If someone more rpm-savvy wants to take this over, please feel free. |
In case it helps at all, the I should think if it's possible with a simple tarball...surely rpmbuild can be made to do the right thing. Though I too am very much not an expert nor savvy. |
|
I've just spent way more hours than I should've trying to debug this. Conclusion: our Makefile is very badly broken. It is doing nasty things with |
edsantiago
force-pushed
the
unitfile_path
branch
from
96d79d1 to
ff01a2d
Compare
|
Looks like you got it! |
contrib/spec/podman.spec.in
Outdated
| @@ -435,7 +435,7 @@ BUILDTAGS=$BUILDTAGS make binaries | |||
| %install | |||
| install -dp %{buildroot}%{_unitdir} | |||
| install -dp %{buildroot}%{_usr}/lib/systemd/user | |||
| PODMAN_VERSION=%{version} %{__make} PREFIX=%{buildroot}%{_prefix} ETCDIR=%{buildroot}%{_sysconfdir} \ | |||
| PODMAN_VERSION=%{version} %{__make} DESTDIR=%{buildroot} PREFIX=/usr ETCDIR=%{_sysconfdir} \ | |||
There was a problem hiding this comment.
@lsm5 PTAVCL (Very Close Look) at this change. I'm highly confident that it is the right thing to do, and that this fixes something that has been broken from Day One... but when I get "confident" is exactly when I need to ask for extra sets of eyes. I don't know who/what else uses this specfile, or what cascading effects this might have.TIA.
There was a problem hiding this comment.
@edsantiago can you remind me what exactly was/in broken?
There was a problem hiding this comment.
mostly LGTM on this change, that's how it should be. I guess last time we didn't have a DESTDIR variable in makefile or something, is that correct?
If you could change to DESTDIR=%{buildroot} PREFIX=%{_prefix}, even better. %{_prefix} by default evaluates to /usr
There was a problem hiding this comment.
Why is PREFIX=/usr ? Currently make install installs in /usr/local. The exact problem is, that make install installs in /usr/local/bin/podman, but the service files looked at /usr/bin/podman wihtout /local.
There was a problem hiding this comment.
can you remind me what exactly was/in broken?
If a user installs into /usr/local for some reason, the systemd unit files will still refer to /usr/bin/podman. See #10787.
last time we didn't have a DESTDIR variable in makefile or something, is that correct?
I doubt it; I think DESTDIR dates back pretty far. I don't know why this was the way it was. It was broken from the beginning, I suspect it was copied from something else.
If you could change to DESTDIR=%{buildroot} PREFIX=%{_prefix}, even better
Done. Thanks for the hint.
There was a problem hiding this comment.
Why is PREFIX=/usr ?
Because this is a specfile. Specfiles are for installing in a distribution. Distributions install into /usr, never ever ever into /usr/local.
|
@tobwen @dilyanpalauzov PTAL |
This makes no sense. DESTDIR shall not influence the content of the .service files. |
|
https://www.gnu.org/prep/standards/html_node/DESTDIR.html - DESTDIR purpose. |
edsantiago
force-pushed
the
unitfile_path
branch
from
ff01a2d to
bd763cd
Compare
|
You're right, I meant PREFIX (which in turn affects BINDIR). I've updated the commit message, rebased, and pushed. Thank you |
Don't hardcode /usr/bin/podman in unit files: instead, use template files with a path replaced at install time. Because 'make' can be invoked repeatedly, with different PREFIX, do not leave the generated files behind in our work directory: wipe them immediately after install. To get this to work, fix a longstanding bug in podman.spec.in, a PREFIX that should've been DESTDIR. Side note: containers#7023 made contrib/systemd/user a symlink to .../system but did not update paths in Makefile. The unrelated-looking path change you see here is a belated correction for that. Fixes: containers#10787 Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
unitfile_path
branch
from
bd763cd to
69b6659
Compare
|
@containers/podman-maintainers I'm calling this ready to merge |
|
I am not really particularly qualified to review makefile changes, but from my limited knowledge, this LGTM |
|
/lgtm |
|
Thanks for sticking it out through merge @edsantiago If not for your work, this probably wouldn't have been fixed at all, or worse, fixed in a very bad/broken way. It takes courage to pick something so frustrating like this, back up into play and over the goal line. |
|
💯 ! |
github-actions
bot
added
the
locked - please file new issue/PR
Don't hardcode /usr/bin/podman in unit files: instead, use
template files with a path replaced at install time.
Because 'make' can be invoked repeatedly, with different
PREFIX, do not leave the generated files behind in our
work directory: wipe them immediately after install.
Side note: #7023 made contrib/systemd/user a symlink
to .../system but did not update paths in Makefile.
The unrelated-looking path change you see here is
a belated correction for that.
Fixes: #10787
Signed-off-by: Ed Santiago [email protected]