https://bugzilla.redhat.com/show_bug.cgi?id=1728381
Alex Williamson <alex.williamson(a)redhat.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Flags| |needinfo?(crobinso(a)redhat.c
| |om)
--- Comment #8 from Alex Williamson <alex.williamson(a)redhat.com> ---
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(a)\*.service
%systemd_post openvpn-server(a)\*.service
%preun
%systemd_preun openvpn-client(a)\*.service
%systemd_preun openvpn-server(a)\*.service
%postun
%systemd_postun_with_restart openvpn-client(a)\*.service
%systemd_postun_with_restart openvpn-server(a)\*.service
%systemd_postun_with_restart openvpn(a)\*.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