On Tue, Jul 25, 2023 at 10:45:30AM -0400, Andrea Bolognani wrote: > On Tue, Jul 25, 2023 at 10:51:04AM +0000, Zbigniew Jędrzejewski-Szmek wrote: > > I don't think Fedora would reject those changes, because in general > > we don't reject things unless they really break stuff for other people. > > Nevertheless, I agree with what Daniel P. Berrangé wrote in another reply: > > > > > My concern is about where the solution is applied and divergance from > > > Fedora guidelines. > > > > > > The long term direction of Fedora / RPM has been to reduce the number > > > of scriptlets required to be explicitly listed in package specfiles, by > > > having RPM globally apply script logic for all content in certain given > > > directories. If we're using standard Fedora macros, then we'd not expect > > > to see problems if the macros get changed to adapt to new approaches, > > > but if we're going our own way all bets are off. > > > > > > The current macros we're using are specified in the Fedora packaging > > > guidelines: > > > > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_systemd > > > > > > and their impl is provided by systemd upstream itself: > > > > > > https://github.com/systemd/systemd/blob/main/src/rpm/macros.systemd.in > > > > > > The problems described do not appear to be things unique to libvirt. > > > > > > IOW, I think the problem needs to be raised & addressed in context of > > > the Fedora and systemd communities, rather than having libvirt diverge > > > from normal Feora packaging practice. > > > > You're putting in quite a lot of work to create some very specific > > machinery. From the POV of the individual package this may look like > > the most efficient solution, but from the POV of the distro, such > > unique per-package solutions have the downside that they require special > > knowledge and make it harder for other maintainers to understand the > > package and contribute to it. > > No disagreement from me here :) > > > I think it'd be more effiecient to go with the (slightly ugly, no > > disagreement here) seven line %trigger scriptlet. And and then work > > with upstream to implement a generic solution that can be used by all > > packages. It seems to me that the work put into implementing a custom > > solution in libvirt would be wasted if you want to work with upstream > > on a completely different general solution soon after. > > Another way to look at it is that I *already* put all that work in, > and the solution I've proposed for libvirt has been tested quite > thoroughly in a number of install and upgrade scenarios. In order to > adopt the %trigger based solution, I will have to spend more time on > developing and testing another implementation. That's fine. I only saw some links to emails, and it didn't know that those have already been applied and tested. My goal was to minimize overall effort, not to increase it ;) > I'll give it a shot, but as I mentioned time is not on our side with > this one. My primary concern is having a working solution in place so > that users' deployment don't break on upgrade. > > > > Note that systemd-update-helper also doesn't currently support > > > marking services as needs-reload, only needs-restart, so that would > > > have to be added. > > > > So far, the assumption was that almost everything would be restarted > > (because we want the new code to be running). Stuff that cannot be > > restarted is effectively ignored by the packaging macros, we have > > %systemd_postun == %nil. In the general case, for services that do > > not support restarting, it's not clear that they should be reloaded. > > They old code might not support the new configuration format, or some > > of the auxiliary files might not exist in the new version, or even if > > the reload is supported, it's not clear why it should happen. > > > > So packages are generally expected to do the reload, if appropriate, > > on their own. There is really no harm in having > > %postun > > systemctl try-reload foobar.service || : > > > > We *could* wrap this in a macro, but it's trivial and package-specific > > anyway. > > I feel that having packages call systemctl directly instead of going > through the %systemd_* macros is an anti-pattern that shouldn't be > encouraged. That's a valid point. > The current set of macros already includes both %systemd_postun and > %systemd_postun_with_restart, so adding %systemd_postun_with_reload > or something along those lines doesn't seem like a stretch. → https://github.com/systemd/systemd/pull/28521 > > > > > +%post -n kdump-tools > > > > > # Initial installation > > > > > %systemd_post kdump.service > > > > > > > > This one is tricky. %systemd_post presets the service on "first installation", > > > > which is actually the first the time package is installed. I.e. it unfortunately > > > > also would execute the preset on upgrades that split out a subpackage, because > > > > as far as rpm is concerned, this is the initial installation of that subpackage. > > > > > > > > The righteous way to solve this would be something like this: > > > > > > > > %triggerprein -n kdump-tools -- kexec-tools < 2.0.26-8 > > > > touch %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset > > > > > > > > %post -n kdump-tools > > > > rm %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset 2>/dev/null && return 0 > > > > # Initial installation > > > > %systemd_post kdump.service > > > > > > > > A bit of a bother, but at least nobody will be suprised by > > > > kdump.service changing state. > > Looking at this again, I can rely on the fact that %triggerprein will > run before %post, right? It looks like that based on what I know > about scriptlets execution order and your example, but I want to be > sure. Yes, I think so. https://rpm-software-management.github.io/rpm/manual/triggers.html#order-of-script-execution seems to say so too. > What version number should I use in my case? The package split > happened in 9.1.0-1, but this handling is going to land in 9.6.0-1 at > the earliest so I think think using this latter version number makes > more sense, right? If you have somehow already upgraded to, say, > 9.3.0-1 in the meantime, then your deployment is already broken and > there's not much that we can do about it anyway. Yes. > > > The beauty of the "check at %pre time whether the unit already exists > > > on disk" approach is that it should work out of the box in all > > > scenarios, with no further information needed. Do you see a problem > > > with it that I have failed to consider? > > > > I think it's an interesting approach and it sounds like it should > > work. Nevertheless, it's a big departure from what we used before, so > > there might be some corner cases I'm not seeing. > > That's already encouraging :) > > > I wonder if it would be possible to invert the approach, and have a > > %transfiletriggerun that'd mark various using to not require a preset, > > and then just do the preset for anything that was added later. > > I'm not sure I quite understand what you're suggesting. > > Wouldn't that part be under systemd's control? AFAICT the current > implementation uses %transfiletriggerpostun to restart services that > have been marked as needing a restart, but the task of marking them > as such still falls on the package. > > For presets, we don't have a --marked option that would allow > applying everything in one go. I actually don't think that we would > want that to happen anyway: in the case of libvirt, for example, we > need presets for virtqemud.socket to be applied before presets for > virtqemud.service, because virtqemud.socket is disabled as per the > system-wide default and virtqemud.service is explicitly enabled, but > at the same time virtqemud.service has Also=virtqemud.socket in its > [Install] section and we want the result to be both being enabled. > > In general, wouldn't this kind of approach require marking units with > preset-already-applied (or similar) and carrying that annotation for > the lifetime of the system? That seems worse than marking units as > preset-needed (or similar) for just the short time between %pre and > %posttrans. My idea was that we'd preset all units. The guidelines say that units should be enabled through presets, so effectively, all units should either use presets or have no enablement, in which case the preset has no effect. But looking at this again, I don't think it's worth the trouble, because we'd still need some of the other scriptlets, so the spec files wouldn't become much simpler. Zbyszek _______________________________________________ devel mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
