RPM triggers: rewrite in shell #8550

cgwalters · 2018-03-22T13:06:53Z

This is compatible with rpm-ostree, which implements offline and
transactional updates by creating a new root, sandboxing each RPM script
in a bwrap container: coreos/rpm-ostree#749

Further, the "test for a directory and run a binary" pattern is significantly
shorter and more obvious in shell script.

Discussion in: #8090 (comment)

One counter was that this causes systemd to depend on /bin/sh, but that's
true today in Fedora at least, and I'd be extremely surprised if there were a
distribution where that wasn't the case. Down the line RPM could probably
learn to omit Requires for %transfiletrigger on /bin/sh.

Downstream: https://bugzilla.redhat.com/show_bug.cgi?id=1559141

Conan-Kudo

NAK.

You should really just implement a Lua subprocess thing in rpm-ostree.

poettering · 2018-03-22T18:19:21Z

src/core/triggers.systemd.in

+# upgraded. We care about the case where a package is initially
+# installed, because other cases are covered by the *un scriptlets,
+# so sometimes we will reload needlessly.
+if test -d /run/systemd/system; then %{_bindir}/systemctl daemon-reload; fi


is there any benefit in doing this one line? why not break this the way people would expect, i.e. in three lines?

I'm fine doing that if you prefer. I thought the one-liner emphasized just how much simpler shell is over Lua, but again I can easily change it.

we tend to prefer readability over density

Ok, updated to be multi-line ⬇️

poettering · 2018-03-22T18:20:43Z

src/core/triggers.systemd.in

+# This script will process files installed in @sysusersdir@ to create
+# specified users automatically. The priority is set such that it
+# will run before the tmpfiles file trigger.
+if test -d /run/systemd/system; then %{_bindir}/systemd-sysusers; fi


as above, why compress this into one line?

i mean, if you want to compress this into one line, why not do:

test -d … && …?

test -d … && …?

I actually did it that way to start, but then remember in shell that will result in exit 1 and hence RPM complaining about script failures... (and on a related note, in rpm-ostree by default script failures are fatal)

poettering · 2018-03-22T18:23:04Z

NAK.

You should really just implement a Lua subprocess thing in rpm-ostree.

Does rpm even have hooks for that?

Conan-Kudo · 2018-03-22T18:30:47Z

Does rpm even have hooks for that?

@poettering As rpm-ostree doesn't actually use librpm for most of its work, it's really up to @cgwalters to actually implement the functionality he's replacing.

I'm not inclined to entertain this because the rewrite to Lua was deliberate to make the scriptlet execution safer and more predictable.

cgwalters · 2018-03-22T19:52:19Z

scriptlet execution safer

If we're talking about safety I think rpm-ostree provides more value to the FOSS ecosystem than having scripts written in Lua. We've written and maintain a system that allows you to always safely pull the power while preparing a system update. And you can still install packages. We go to a lot of effort to have rpm-ostree install postgresql-server not touch your running system! Specifically again on this topic, we don't want that to invoke systemctl daemon-reload.

Anyways, at some point we probably will do the "lua as external subprocess", but bear in mind that things were working fine for Fedora Atomic {Host,Workstation} before this code landed in systemd.

poettering · 2018-03-22T19:56:37Z

src/core/triggers.systemd.in

+# installed, because other cases are covered by the *un scriptlets,
+# so sometimes we will reload needlessly.
+if test -d /run/systemd/system; then
+  %{_bindir}/systemctl daemon-reload


the other shell code uses 4ch indenting, no? please stick to the same across the whole file

Right, fixed ⬇️

poettering · 2018-03-22T19:56:50Z

src/core/triggers.systemd.in

+# specified users automatically. The priority is set such that it
+# will run before the tmpfiles file trigger.
+if test -d /run/systemd/system; then
+  %{_bindir}/systemd-sysusers


Conan-Kudo · 2018-03-22T20:02:47Z

If we're talking about safety I think rpm-ostree provides more value to the FOSS ecosystem than having scripts written in Lua.

I might consider that, if it weren't for the fact it can't handle large classes of RPMs as it is. Nothing with %verifyscript (bye SUSE systems!), nothing with Lua scriptlets (bye embedded systems and low-level packages in many Linux distributions), and nothing with %pretrans (though this might be a bit of a meh, since it just ignores %pretrans...).

This is compatible with rpm-ostree, which implements offline and transactional updates by creating a new root, sandboxing each RPM script in a bwrap container: coreos/rpm-ostree#749 Further, the "test for a directory and run a binary" pattern is significantly shorter and more obvious in shell script. Discussion in: systemd#8090 (comment) One counter was that this causes systemd to depend on `/bin/sh`, but that's true today in Fedora at least, and I'd be extremely surprised if there were a distribution where that wasn't the case. Down the line RPM could probably learn to omit `Requires` for `%transfiletrigger` on `/bin/sh`. Downstream: https://bugzilla.redhat.com/show_bug.cgi?id=1559141

Conan-Kudo · 2018-03-24T09:46:06Z

Down the line RPM could probably learn to omit Requires for %transfiletrigger on /bin/sh.

I didn't notice this immediately, but as I read this now, I realize that what you're asking for is nuts. You realize that I can technically set any scriptlet to any interpreter I want, right? I can even set it to Perl, Python, or any other heavyweight interpreters. I could even set it to PowerShell! It would be asinine to even consider not exporting the interpreter requires for scriptlets...

cgwalters · 2018-03-24T11:40:30Z

I didn't notice this immediately, ...

Do note the last two words in the (offhand) proposal: "on `/bin/sh".

keszybz · 2018-03-26T13:21:28Z

Hmm, so, I understand that the late landing of this feature in Fedora causes an issue for rpm-ostree. It would be reasonable to use your patch in Fedora for some time. (But note that the version in Fedora is already slightly different than the one upstream, since the upstream version does not support the split of systemd-udev subpackage).

I'm not convinced that we want to replace lua with shell. I think that since it's legal (and encouraged) to use lua in Fedora, there will be more and more scriptlets using lua. Right now there aren't that many, but there are some. And right now we have a bazillion useless scriptlets, so a single one is barely noticeable. But the efforts to prune the useless ones significantly have already begun, and then the remaining scriptlets will be more visible, and the benefit from rewriting them in lua to reduce the dependencies a bit will be stronger.

In the longer run, I expect that rpm-ostree will need to learn to run lua scriptlets too. And actually this shouldn't be too onerous: I know that the lua environment in rpm is a bit special, but afaik it differers primarily in the list of modules that are available, so it should be possible to recreate the same environment.

What about doing this just in Fedora for the time beeing?

cgwalters · 2018-03-26T13:54:09Z

What about doing this just in Fedora for the time beeing?

That's OK with me.

cgwalters · 2018-03-27T21:35:10Z

https://src.fedoraproject.org/rpms/systemd/c/8e6b39457b3e2660793821e0524855226e33e306?branch=master

dustymabe · 2018-03-27T21:37:47Z

https://src.fedoraproject.org/rpms/systemd/c/8e6b39457b3e2660793821e0524855226e33e306?branch=master

This was done with some urgency to un-break f28 beta for Atomic Host.

keszybz · 2018-03-28T06:45:07Z

Well, OK. Let's continue the discussion in the Fedora bugtracker.

This fixes compatibility with rpm-ostree.

cgwalters mentioned this pull request Mar 22, 2018

Add more file triggers to handle more aspects of systemd #8090

Merged

Conan-Kudo suggested changes Mar 22, 2018

View reviewed changes

poettering reviewed Mar 22, 2018

View reviewed changes

poettering added the rpm label Mar 22, 2018

cgwalters force-pushed the rpmscripts-shell branch from b806a6a to 0a458af Compare March 22, 2018 19:48

poettering reviewed Mar 22, 2018

View reviewed changes

cgwalters force-pushed the rpmscripts-shell branch from 0a458af to 9274509 Compare March 22, 2018 20:39

keszybz closed this Mar 28, 2018

yuwata pushed a commit to yuwata/systemd-fedora that referenced this pull request Apr 16, 2018

Use shell for triggers; see systemd/systemd#8550

8e6b394

This fixes compatibility with rpm-ostree.

yuwata pushed a commit to yuwata/systemd-fedora that referenced this pull request Apr 16, 2018

Use shell for triggers; see systemd/systemd#8550

dc46312

This fixes compatibility with rpm-ostree.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPM triggers: rewrite in shell #8550

RPM triggers: rewrite in shell #8550

cgwalters commented Mar 22, 2018

Conan-Kudo left a comment

poettering Mar 22, 2018

cgwalters Mar 22, 2018

poettering Mar 22, 2018

cgwalters Mar 22, 2018

poettering Mar 22, 2018

poettering Mar 22, 2018

cgwalters Mar 22, 2018

poettering Mar 22, 2018

poettering commented Mar 22, 2018

Conan-Kudo commented Mar 22, 2018

cgwalters commented Mar 22, 2018

poettering Mar 22, 2018

cgwalters Mar 22, 2018

poettering Mar 22, 2018

Conan-Kudo commented Mar 22, 2018 •

edited

Loading

Conan-Kudo commented Mar 24, 2018 •

edited

Loading

cgwalters commented Mar 24, 2018

keszybz commented Mar 26, 2018

cgwalters commented Mar 26, 2018

cgwalters commented Mar 27, 2018

dustymabe commented Mar 27, 2018

keszybz commented Mar 28, 2018

RPM triggers: rewrite in shell #8550

RPM triggers: rewrite in shell #8550

Conversation

cgwalters commented Mar 22, 2018

Conan-Kudo left a 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

poettering commented Mar 22, 2018

Conan-Kudo commented Mar 22, 2018

cgwalters commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conan-Kudo commented Mar 22, 2018 • edited Loading

Conan-Kudo commented Mar 24, 2018 • edited Loading

cgwalters commented Mar 24, 2018

keszybz commented Mar 26, 2018

cgwalters commented Mar 26, 2018

cgwalters commented Mar 27, 2018

dustymabe commented Mar 27, 2018

keszybz commented Mar 28, 2018

Conan-Kudo commented Mar 22, 2018 •

edited

Loading

Conan-Kudo commented Mar 24, 2018 •

edited

Loading