Remove symlinks for fedora-* services #127

deekej · 2017-09-19T13:53:13Z

These symlinks are no longer needed, because services enabled by default are now managed by fedora-release package.

Before merging this PR/releasing new version of initscripts, we should wait for necessary changes to be made in fedora-release.

lnykryn

Those patches should be merged to one commit. It is one change "going from static enable to presets" and separate commits could be confusing for others or during backports.

Good point. Commits squashed into one. :)

deekej · 2017-10-16T12:45:53Z

We're still waiting for this BZ to be resolved first:
https://bugzilla.redhat.com/show_bug.cgi?id=1493479

deekej · 2017-10-16T12:57:15Z

UPDATE: Pull-requst by @keszybz was merged into fedora-release package.

lnykryn

We also need to add systemd_post and frinds scriplets to spec file:
https://fedoraproject.org/wiki/Packaging:Scriptlets#Scriptlets

deekej · 2017-10-16T17:06:09Z

Thanks for spotting this, I wasnt't aware of this. I will update the commit. :)

deekej · 2017-10-16T18:04:33Z

I have updated the specfile, but I'm unsure about few things:

I have added the Requires of systemd-units for post, postun and preun phases.
I have added the BuildRequires for systemd-devel instead of systemd (as suggested in the wiki)

I was looking at httpd package on how it does in Rawhide, and it is different compared to the wiki. But IMHO that shouldn't be a problem for the build/upgrade/removal of initscripts.

I have also set the fedora-import-state.service and fedora-readonly.service to not be restarted during upgrade. These are oneshot type of services, and it doesn't make sense to me to run them during upgrade, it could break things. If I'm wrong, please correct me.

I'm updating the commit for RHEL7 in the same way.

Please, check if the requested changes are OK. Thank you.

keszybz · 2017-10-16T18:40:18Z

initscripts.spec

 Requires(preun): /sbin/chkconfig
-BuildRequires: glib2-devel popt-devel gettext pkgconfig systemd
+Requires(preun): systemd-units
+BuildRequires: glib2-devel popt-devel gettext pkgconfig systemd-devel


Please see the guidelines, they are up to date: https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd. systemd-units has been gone for many many years. You need systemd-devel only when linking to the systemd libraries.

And yes, using %systemd_postun (without restart) is correct.

OK, I will fix that. Just one more question - should I also add this line?

%{?systemd_requires}

I'm not sure about the purpose of that line...

It generated the required Requires(pre) and Requires(post) lines.

keszybz · 2017-10-16T18:40:39Z

initscripts.spec

@@ -57,19 +60,28 @@ touch %{buildroot}%{_sysconfdir}/rc.d/rc.local
 chmod 755 %{buildroot}%{_sysconfdir}/rc.d/rc.local

 %post
+%systemd_post fedora-import-state.service
+%systemd_post fedora-readonly.service


It's better to put both services on the same line, to save systemctl invocations.

keszybz · 2017-10-16T18:40:46Z

initscripts.spec

 /usr/sbin/chkconfig --add network > /dev/null 2>&1 || :
 /usr/sbin/chkconfig --add netconsole > /dev/null 2>&1 || :
 if [ $1 -eq 1 ]; then
  /usr/bin/systemctl daemon-reload > /dev/null 2>&1 || :
 fi

 %preun
+%systemd_preun fedora-import-state.service
+%systemd_preun fedora-readonly.service


keszybz · 2017-10-16T18:40:53Z

initscripts.spec

 if [ $1 = 0 ]; then
  /usr/sbin/chkconfig --del network > /dev/null 2>&1 || :
  /usr/sbin/chkconfig --del netconsole > /dev/null 2>&1 || :
 fi

 %postun
+%systemd_postun fedora-import-state.service
+%systemd_postun fedora-readonly.service


keszybz · 2017-10-16T18:42:17Z

systemd/system/fedora-import-state.service

@@ -12,3 +12,6 @@ ExecStart=/lib/systemd/fedora-import-state
 Type=oneshot
 TimeoutSec=0
 RemainAfterExit=yes
+
+[Install]
+WantedBy=local-fs.target


I think this should be WantedBy=basic.target.

This one will have to answer @lnykryn, I'm no expert on systemd. I have just used the targets as they were before (created by symlinks that we're removing).

I would put sysinit.target there, basic target seems a bit late to me.
@deekej just for FYI this is nicely documented in bootup manpage.

After looking at the systemd bootup process details (and in context how it was done before and what it does), I agree with @lnykryn. The basic.taget can be already expecting that fedora-import-state.service has finished.

If you do not want the local-fs.target for that, then sysinit.target seems to be a way to go. :)

You're right, sysinit.target is better.

keszybz · 2017-10-16T18:43:35Z

systemd/system/fedora-loadmodules.service

@@ -11,3 +11,6 @@ ExecStart=/lib/systemd/fedora-loadmodules
 Type=oneshot
 TimeoutSec=0
 RemainAfterExit=yes
+
+[Install]
+WantedBy=basic.target


Hmm, most likely WantedBy=sysinit.target.

Same here. @lnykryn?

Yep we can do that.

Updated commit pushed. Please re-check, thanks!

lnykryn · 2017-10-17T11:31:52Z

initscripts.spec

+%systemd_post fedora-import-state.service \
+              fedora-loadmodules.service \
+              fedora-readonly.service
+
 /usr/sbin/chkconfig --add network > /dev/null 2>&1 || :
 /usr/sbin/chkconfig --add netconsole > /dev/null 2>&1 || :
 if [ $1 -eq 1 ]; then


I think that we can also drop the daemon-reload calls from the spec file.
https://fedoraproject.org/wiki/Changes/systemd_file_triggers

No, please keep them as written in the guidelines. They evaluate to nothing if not necessary, but having the macro leaves the window open to change the implementation in systemd later on if necessary.

keszybz · 2017-10-17T13:38:35Z

systemd/system/fedora-import-state.service

@@ -12,3 +12,6 @@ ExecStart=/lib/systemd/fedora-import-state
 Type=oneshot
 TimeoutSec=0
 RemainAfterExit=yes
+


OK, but then you need Before=sysinit.target, not Before=basic.target.

keszybz · 2017-10-17T13:39:10Z

systemd/system/fedora-readonly.service

@@ -10,3 +10,6 @@ ExecStart=/lib/systemd/fedora-readonly
 Type=oneshot


/usr/lib/...

keszybz · 2017-10-17T13:56:29Z

initscripts.spec

+                fedora-readonly.service
+
+# This should be removed in Rawhide for Fedora 29:
+%triggerin -- initscripts > 9.77


I don't think this can work. This will get triggered every time this package is installed or upgraded.

Maybe something like this:

%triggerpostun -- initscripts <= 9.77 echo "Manually enabling fedora-import-state.service and fedora-readonly.service on upgrade" systemctl enable fedora-import-state.service fedora-readonly.service

(untested)

Yes, we were discussing this with @lnykryn, because we were unsure about it. This will be changed.

I have updated this part. I need to make just one more test, but it seems promising.

Another round of improvements, please re-check.

The symlinks are no longer needed. Enablement of default services is now managed by 'fedora-release' package.

keszybz · 2017-10-18T14:37:20Z

LGTM.

deekej · 2017-10-19T09:29:18Z

Unfortunately, we're still waiting on Dennis to make a new release of fedora-release package... :-/

lnykryn previously requested changes Sep 20, 2017

View reviewed changes

deekej self-assigned this Sep 20, 2017

deekej added bugfix Fedora labels Sep 20, 2017

deekej added this to the initscripts-10.0 milestone Sep 20, 2017

deekej added the dont-merge label Oct 16, 2017

lnykryn previously requested changes Oct 16, 2017

View reviewed changes

keszybz previously requested changes Oct 16, 2017

View reviewed changes

lnykryn reviewed Oct 17, 2017

View reviewed changes

keszybz previously requested changes Oct 17, 2017

View reviewed changes

systemd/system: symlinks for fedora-* services removed

848d083

The symlinks are no longer needed. Enablement of default services is now managed by 'fedora-release' package.

keszybz approved these changes Oct 18, 2017

View reviewed changes

deekej merged commit 5829c81 into fedora-sysv:master Oct 19, 2017

deekej deleted the remove-systemd-target-symlinks branch October 19, 2017 09:22

deekej removed the dont-merge label Nov 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove symlinks for fedora-* services #127

Remove symlinks for fedora-* services #127

deekej commented Sep 19, 2017 •

edited

lnykryn left a comment

deekej commented Oct 16, 2017

deekej commented Oct 16, 2017 •

edited

lnykryn left a comment

deekej commented Oct 16, 2017

deekej commented Oct 16, 2017 •

edited

keszybz Oct 16, 2017

keszybz Oct 16, 2017

deekej Oct 16, 2017

keszybz Oct 17, 2017

keszybz Oct 16, 2017

keszybz Oct 16, 2017

keszybz Oct 16, 2017

keszybz Oct 16, 2017

deekej Oct 16, 2017

lnykryn Oct 17, 2017

deekej Oct 17, 2017

keszybz Oct 17, 2017

keszybz Oct 16, 2017

deekej Oct 16, 2017

lnykryn Oct 17, 2017

lnykryn Oct 17, 2017

keszybz Oct 17, 2017

keszybz Oct 17, 2017

keszybz Oct 17, 2017

keszybz Oct 17, 2017

deekej Oct 17, 2017

deekej Oct 18, 2017

keszybz commented Oct 18, 2017

deekej commented Oct 19, 2017

		@@ -10,3 +10,6 @@ ExecStart=/lib/systemd/fedora-readonly
		Type=oneshot

Remove symlinks for fedora-* services #127

Remove symlinks for fedora-* services #127

Conversation

deekej commented Sep 19, 2017 • edited

lnykryn left a comment

Choose a reason for hiding this comment

deekej commented Oct 16, 2017

deekej commented Oct 16, 2017 • edited

lnykryn left a comment

Choose a reason for hiding this comment

deekej commented Oct 16, 2017

deekej commented Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keszybz commented Oct 18, 2017

deekej commented Oct 19, 2017

deekej commented Sep 19, 2017 •

edited

deekej commented Oct 16, 2017 •

edited

deekej commented Oct 16, 2017 •

edited