Bug 1728381 - Review Request: mdevctl - A mediated device persistence and management utility
Summary: Review Request: mdevctl - A mediated device persistence and management utility
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Cole Robinson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-07-09 19:59 UTC by Alex Williamson
Modified: 2020-04-07 18:37 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-04-07 18:37:51 UTC
Type: Bug
crobinso: fedora-review+


Attachments (Terms of Use)
mdevctl.spec (5.64 KB, text/plain)
2019-07-13 15:38 UTC, Alex Williamson
no flags Details

Description Alex Williamson 2019-07-09 19:59:59 UTC
Description of problem:

https://github.com/mdevctl/mdevctl

The mdevctl utility fills a gap in the management of devices within Linux's mediated device interface.  Mediated devices are currently used by Intel vGPU (GVT-g) and NVIDIA vGPU (GRID) on x86_64 platforms, as well as CCW and AP devices interfaces on S390 platforms, to provide VFIO-based device assignment to virtual machines.  Further mdev use cases are under development, such as support for Intel Scalable IOV devices and non-VFIO virtual devices in the networking space.

The mdevctl utility aims to provide command line management of these devices as well as persistence of these devices between boots.

Please provide comments and support towards the inclusion of this package into the next available Fedora release.

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.

Actual results:


Expected results:


Additional info:

Comment 1 Robert-André Mauchin 2019-07-13 15:21:22 UTC
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.

Comment 2 Alex Williamson 2019-07-13 15:38:48 UTC
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.

Comment 3 Cole Robinson 2019-07-15 16:33:02 UTC
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

Comment 4 Cole Robinson 2019-07-15 20:02:16 UTC
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

Comment 5 Alex Williamson 2019-07-15 22:51:07 UTC
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

Comment 6 Alex Williamson 2019-07-17 20:28:11 UTC
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.

Comment 7 Cole Robinson 2019-07-29 00:20:34 UTC
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

Comment 8 Alex Williamson 2019-10-01 19:40:11 UTC
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!

Comment 9 Cole Robinson 2019-10-02 18:18:58 UTC
(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

Comment 10 Alex Williamson 2019-10-02 19:59:19 UTC
(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

Comment 11 Cole Robinson 2019-10-02 22:35:46 UTC
(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

Comment 12 Gwyn Ciesla 2019-10-04 16:17:23 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/mdevctl

Comment 13 Cole Robinson 2020-04-07 18:37:51 UTC
This has been built for f30+


Note You need to log in before you can comment on or make changes to this bug.