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

ceph.spec.in: add BuildRequires: systemd #6692

Merged
merged 2 commits into from Dec 15, 2015
Merged

Conversation

smithfarm
Copy link
Contributor

http://tracker.ceph.com/issues/13860 Fixes: #13860

Signed-off-by: Nathan Cutler ncutler@suse.com

http://tracker.ceph.com/issues/13860 Fixes: ceph#13860

Signed-off-by: Nathan Cutler <ncutler@suse.com>
@ktdreyer
Copy link
Member

I'm not a fan of running systemd-tmpfiles in %post (versus listing the temp location with %files) but iirc this is a SUSE vs Fedora convention thing, and we might as well continue with the current implementation, so 👍 if this helps OBS

@smithfarm
Copy link
Contributor Author

@ktdreyer I think I see what you mean. We could include the files in %files and drop systemd-tmpfiles from %post. Their inclusion in %files would ensure that they are created when the package is installed. After the next reboot they will disappear, but systemd-tmpfiles is run at boot and will re-create them. Correct?

@ktdreyer
Copy link
Member

You've got it exactly!

@smithfarm
Copy link
Contributor Author

@ktdreyer I asked the openSUSE maintainers about this, and they are pushing back:

The extra directories/files make rpm slower than it already is (its database is
really horribly laid out), and when you are in a rescue system, `rpm -V` will
needlessy complain about the missing file if you did not mark it %ghost,
%noverify, or some combination thereof.

Even the remotely legitimate case of removing /var/run/xyz directories on
  (a) package removal, and
  (b) on ancient system where /var/run is not yet a tmpfs
can be rejected as well, because the program may create any number of run files
which rpm definitely is not tracking and therefore won't remove.

IOW, save yourself the trouble of listing /var/run/xyz.

Note: we already had the files in %files before (with %ghost IIRC), and we switched to running systemd-tmpfiles in %post . . . if we want to go back to that, we would most likely have to conditionalize it so it doesn't apply to openSUSE, and I would try to avoid that if possible.

@ktdreyer
Copy link
Member

Thanks for asking the maintainers about this. I think using %ghost is preferred in Fedora, but this is such a small issue, let's just continue down the path and BR: systemd here.

@smithfarm smithfarm changed the title ceph.spec.in: add BuildRequires: systemd [DNM] ceph.spec.in: add BuildRequires: systemd Nov 25, 2015
@smithfarm
Copy link
Contributor Author

Added a patch to satisfy openSUSE maintenance policy.

@ktdreyer
Copy link
Member

Can we just define %tmpfiles_create if it is undefined, and avoid the distro conditionals? I'm guessing it's something like this:

%{!?tmpfiles_create: %global tmpfiles_create %(systemd-tmpfiles --create)}

@smithfarm
Copy link
Contributor Author

In openSUSE it is defined in systemd-rpm-macros like so:

%tmpfiles_create() \
[ -x /usr/bin/systemd-tmpfiles ] && \
    /usr/bin/systemd-tmpfiles --create %{?*} || : \

Presumably it is not defined in Fedora, etc. so your idea should work. The only issue I see is that AFAICT I'm supposed to provide %{_tmpfilesdir}/ceph-common.conf as an argument to systemd-tmpfiles

Thanks for reviewing! I will re-do the commit accordingly.

Ensure the macro is defined, and use it to placate the relevant openSUSE
RPMLINT check.

Drop --prefix=/run/ceph (introduced by 477bb06) because we now supply the
configuration file on the systemd-tmpfiles command line. (We don't need to run
systemd-tmpfiles on all files in /usr/lib/tmpfiles.d/ - just on ours.)

Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor Author

How does this look?

@smithfarm
Copy link
Contributor Author

This is working nicely in the OBS. Removing [DNM].

@smithfarm smithfarm changed the title [DNM] ceph.spec.in: add BuildRequires: systemd ceph.spec.in: add BuildRequires: systemd Dec 1, 2015
@smithfarm
Copy link
Contributor Author

@ktdreyer Ping?

@ktdreyer
Copy link
Member

LGTM, thanks

@ktdreyer ktdreyer merged commit 014e2f0 into ceph:jewel Dec 15, 2015
ktdreyer added a commit that referenced this pull request Dec 15, 2015
ceph.spec.in: add BuildRequires: systemd

Reviewed-by: Ken Dreyer <kdreyer@redhat.com>
@smithfarm smithfarm deleted the wip-13860 branch February 7, 2016 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants