Bug 1728381

This place is to propose package to be reviewed, not to post request. If you want to get this into Fedora, please provide a SPEC file for it.

Created attachment 1590276 [details]
mdevctl.spec

There's an mdevctl.spec.in in the source tree and an mdevctl.spec target in the Makefile.  Attaching the resulting spec file for the current tree.

I know the project is quite small so far and anything beyond a Makefile may seem overkill, but once it gets to packaging across multiple distros it really helps having a standard build system, packagers don't need to worry about if distro cflags and ldflags are getting into the correct places, or install directories are correct, etc.

I recommend you take a look at meson. As an example you can check out ksmtuned usage of meson which is shipping a lot of similar files to mdevctl. The meson build config file is pretty intuitive

https://github.com/ksmtuned/ksmtuned
https://github.com/ksmtuned/ksmtuned/blob/master/meson.build

I'm not sure if that follows meson best practices or whatever but it got the job done for me.

Configure with: meson some-build-dir
Build with: ninja -C some-build-dir

I thought this project was actually building C code but I see now that it's just using shell, my bad. That makes it less 'necessary' to use meson or something standard but it's still a good idea IMO. Not a blocker for this though

Thanks Cole.  I'll try to rewind to proper procedure here, let me know if you'd prefer to start over in a new bz:

Spec URL: http://people.redhat.com/~alwillia/mdevctl/mdevctl.spec
SRPM URL: http://people.redhat.com/~alwillia/mdevctl/mdevctl-0.46-1.fc29.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=36279832

We're largely leveraging (blatant copying) driverctl for build and packaging.  If make is ok for now, maybe we can think about doing something with meson later, but as you've discovered the packaging is the only thing we're "building".  Thanks

Spec URL: http://people.redhat.com/~alwillia/mdevctl/mdevctl.spec
SRPM URL: http://people.redhat.com/~alwillia/mdevctl/mdevctl-0.47-1.fc29.src.rpm

Fixed the "probably wrong" url for Source0 in the spec file, this one works correctly.

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


- 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


- 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.


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

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!

(In reply to Alex Williamson from comment #8)
> 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.
>  

I don't think there's any way to control the github directory structure.
There is a way to get a .tar.gz per commit, using some github URL magic.
https://github.com/mdevctl/mdevctl/archive/e2bda0996bd37d12da6292d8dc6d4a938f657e86/mdevctl-e2bda0996bd37d12da6292d8dc6d4a938f657e86.tar.gz


> 
> > - 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.

Dealing with changelogs across upstream hosted spec and downstream is a pain.
Most projects I work on just don't include a %changelog upstream. But whatever
works for you as long as the format is appropriate for Fedora.


The new version looks good, but the archive still doesn't match the Source
content. Please update the src.rpm with the archive from the Source link:
https://github.com/mdevctl/mdevctl/archive/0.49/mdevctl-0.49.tar.gz

After that I'll approve the review

(In reply to Cole Robinson from comment #9)
> (In reply to Alex Williamson from comment #8)
> > (In reply to Cole Robinson from comment #7)
> > > 
> > > 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.
> >  
> 
> I don't think there's any way to control the github directory structure.
> There is a way to get a .tar.gz per commit, using some github URL magic.
> https://github.com/mdevctl/mdevctl/archive/
> e2bda0996bd37d12da6292d8dc6d4a938f657e86/mdevctl-
> e2bda0996bd37d12da6292d8dc6d4a938f657e86.tar.gz

Right, github will make an archive from any valid refspec.  I was trying to use both the tag and commit id since tags could theoretically be removed and recreated to point at something different, but I don't see an easy way to do that.  Using the commit id should be less forgeable, but then it's only the spec file correlating the tagged version to a commit id.  If we trust upstream not to change tags, what we have should be fine.

> > > - 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.
> 
> Dealing with changelogs across upstream hosted spec and downstream is a pain.
> Most projects I work on just don't include a %changelog upstream. But
> whatever
> works for you as long as the format is appropriate for Fedora.

Would it be considered bad practice in Fedora if the changelog is rewritten between releases?  For instance if the upstream auto-generation changes the formatting or contents for previous releases (as I've done in 0.50), how much, if any effort should we make in the Fedora package to retain released changelog contents as-is, versus simply maintaining compliant formatting?  Same question for the Fedora specific changelog entries.  Would it be considered required or just best-effort to maintain, for example, a mass rebuild 0.49-2 changelog entry when I upload 0.50?

Clearly the approach I'm taking assumes this upstream package isn't going to have more than a handful of commits between releases and I hope that merge tools will handle much of this for each release.

> The new version looks good, but the archive still doesn't match the Source
> content. Please update the src.rpm with the archive from the Source link:
> https://github.com/mdevctl/mdevctl/archive/0.49/mdevctl-0.49.tar.gz
> 
> After that I'll approve the review

Ok, yes, the previous srpm was generated from the upstream Makefile with a local archive rather than directly using the github link.  The contents are the same, but I assume you're looking at md5sum between the two.  I hadn't really figured out this part of the process yet.  For the version uploaded below, I'm using 'spectool -g -R mdevctl.spec' to fetch the upstream source and 'rpmbuild -bs --rmsource mdevctl.spec' to generate the srpm and cleanup the upstream source tarball.  The only other change here is the changelog format, which I hope doesn't churn your stomach or violate Fedora standards.  Thanks!

http://people.redhat.com/~alwillia/mdevctl/mdevctl.spec
http://people.redhat.com/~alwillia/mdevctl/mdevctl-0.50-1.fc29.src.rpm

(In reply to Alex Williamson from comment #10)
> 
> > > > - 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.
> > 
> > Dealing with changelogs across upstream hosted spec and downstream is a pain.
> > Most projects I work on just don't include a %changelog upstream. But
> > whatever
> > works for you as long as the format is appropriate for Fedora.
> 
> Would it be considered bad practice in Fedora if the changelog is rewritten
> between releases?  For instance if the upstream auto-generation changes the
> formatting or contents for previous releases (as I've done in 0.50), how
> much, if any effort should we make in the Fedora package to retain released
> changelog contents as-is, versus simply maintaining compliant formatting? 
> Same question for the Fedora specific changelog entries.  Would it be
> considered required or just best-effort to maintain, for example, a mass
> rebuild 0.49-2 changelog entry when I upload 0.50?
> 

Fedora guidelines say to have one changelog entry per Fedora build. However
plenty of packages also trim changelogs after a certain time, rather than
have lots of historical data there. Nothing enforces it, but if it can be
helped I would try to not throw it away.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs

> 
> Ok, yes, the previous srpm was generated from the upstream Makefile with a
> local archive rather than directly using the github link.  The contents are
> the same, but I assume you're looking at md5sum between the two.  I hadn't
> really figured out this part of the process yet.  For the version uploaded
> below, I'm using 'spectool -g -R mdevctl.spec' to fetch the upstream source
> and 'rpmbuild -bs --rmsource mdevctl.spec' to generate the srpm and cleanup
> the upstream source tarball.

Yes, the fedora-review tool does an md5 comparison. For packages I own
upstream, I will upload the dist on release, then download it back and feed it
to the package build, just to be sure.

The only other change here is the changelog
> format, which I hope doesn't churn your stomach or violate Fedora standards.
> Thanks!
> 

Haven't seen entries like that before but I think it's fine.
Setting fedora-review+ . I will sponsor you too if needed

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/mdevctl

This has been built for f30+