https://bugzilla.redhat.com/show_bug.cgi?id=1728381 Alex Williamson <alex.williamson@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(crobinso@redhat.c | |om) --- Comment #8 from Alex Williamson <alex.williamson@xxxxxxxxxx> --- Thanks for the review Cole, sorry for the delay in getting back to this. (In reply to Cole Robinson from comment #7) > FWIW this packages is quite similar to driverctl which is already in Fedora. > driverctl review bug: https://bugzilla.redhat.com/show_bug.cgi?id=1372670 > > Trimmed output from fedora-review: > > > - Sources used to build the package match the upstream source, as provided > in the spec URL. > See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > > The source URL looks acceptable per the docs, but looks like the archive in > the SRPM is not the same content as github generates. Please fix that Fixed, I don't know how to control the directory structure github uses in the tarball, so I dropped the sha1 from the local build to match github. > - systemd_post is invoked in %post, systemd_preun in %preun, and > systemd_postun in %postun for Systemd service files. > See: > https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/ > #_scriptlets > > The guidelines don't list anything about systemd parameterized units (ones > with @ in > the name), which is what mdevctl is using. driverctl doesn't use the macros, > but openvpn > _does_ use them for its parameterized units, like this: > > %post > %systemd_post openvpn-client@\*.service > %systemd_post openvpn-server@\*.service > > %preun > %systemd_preun openvpn-client@\*.service > %systemd_preun openvpn-server@\*.service > > %postun > %systemd_postun_with_restart openvpn-client@\*.service > %systemd_postun_with_restart openvpn-server@\*.service > %systemd_postun_with_restart openvpn@\*.service > > I don't really understand enough about how all this works to say whether > this package > should be using those macros. aw I'll leave it up to you to decide. Can > always > be fixed after import if necessary Like driverctl, our parameterized systemd unit is triggered by the udev rules, we don't need an enable/disable when installed or removed, it's all dynamic. > - Changelog in prescribed format. > > Changelog lines should be individually prefixed with '-' and contain a > version string > at the end. > > Your changelog there looks more like it should be a NEWS.md file which you > can ship > as %doc. Using that is better for upstream too IMO because other distros > won't want a .spec file to be the canonical release notes. > > For Fedora spec the changelog should be the package version history so all > of those > entries should be trimmed except the most recent one basically. Fixed. What's present now is still entirely auto-generated from the git log, as I think that is our canonical release notes. However, the formatting now matches the Fedora requirements and we're rolling together all the commit subjects between tags. I think this will allow me to merge the upstream auto-generated spec file with the Fedora maintained one fairly automatically, assuming it's good practice to maintain the logs for Fedora specific rebuilds. > mdevctl.noarch: W: empty-%post > mdevctl.noarch: W: empty-%postun > > Looks like on all current Fedora versions, udev_rules_update is an empty > macro, > because file triggers are used to make udev update itself. > https://github.com/systemd/systemd/commit/ > 32a00a9c097cf04ec2b0fcbf9b73eba188318424 > > So you can remove those sections entirely. If you need those for another > distro > like rhel, please wrap them in a conditional to make it clear they don't > apply > to Fedora Removed for now, will make them conditional if they need to be re-added for other distros. Please review current status: http://people.redhat.com/~alwillia/mdevctl/mdevctl.spec http://people.redhat.com/~alwillia/mdevctl/mdevctl-0.49-1.fc29.src.rpm Thanks! -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-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/package-review@xxxxxxxxxxxxxxxxxxxxxxx
