https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Bug ID: 1559786 Summary: Review Request: libvirt-dbus - This package provides integration between libvirt and the DBus Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: phrdina@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://rosnicka.net/libvirt-dbus.spec SRPM URL: https://rosnicka.net/libvirt-dbus-0.0.1-1.fc27.src.rpm
Description: This package provides integration between libvirt and the DBus
This is my first package I'm submitting and I need sponsor. I'm upstream maintainer of this package.
Fedora Account System Username: phrdina
Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=25900028
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Pavel Hrdina phrdina@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Iwicki Artur fedora@svgames.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fedora@svgames.pl
--- Comment #1 from Iwicki Artur fedora@svgames.pl ---
Group: Development/Libraries
Drop this.
%clean
And this.
%install rm -rf $RPM_BUILD_ROOT
And this. https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
%__make %{?_smp_mflags}
Macro forms of system executables are discouraged. https://fedoraproject.org/wiki/Packaging:Guidelines#Macros
%__make install DESTDIR=$RPM_BUILD_ROOT
Use "%make_install" instead.
%doc README.md HACKING.md COPYING AUTHORS NEWS
COPYING should be marked as %license.
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
--- Comment #2 from Pavel Hrdina phrdina@redhat.com --- Thanks a lot for the review, I've fixed all the issues and I've also changed "%setup -q" into "%autosetup".
The Spec URL and SRPM URL now points to the fixed version.
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Cole Robinson crobinso@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |crobinso@redhat.com
--- Comment #3 from Cole Robinson crobinso@redhat.com --- Independent of this review:
$ wget https://rosnicka.net/libvirt-dbus-0.0.1-1.fc27.src.rpm --2018-03-23 12:11:15-- https://rosnicka.net/libvirt-dbus-0.0.1-1.fc27.src.rpm Resolving rosnicka.net (rosnicka.net)... 89.221.209.56 Connecting to rosnicka.net (rosnicka.net)|89.221.209.56|:443... connected. ERROR: The certificate of ‘rosnicka.net’ is not trusted. ERROR: The certificate of ‘rosnicka.net’ hasn't got a known issuer.
choked up fedora-review too. firefox doesn't complain though
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
--- Comment #4 from Cole Robinson crobinso@redhat.com --- fedora-review is still running, but some bits from the spec:
* drop %defattr : https://pagure.io/packaging-committee/issue/77
* I think you can drop %{?extra_release}, that's an old leftover from danbp's autobuild project and I don't think anything uses it anymore. Can probably be removed from libvirt projects as well
* description: s/the D-Bus/D-Bus/. But instead maybe something like: This package provides a D-Bus API for libvirt
* light upstream suggestion: drop AUTHORS, IMO very redundant in the age of git
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
--- Comment #5 from Pavel Hrdina phrdina@redhat.com --- Sigh, it uses Let's Encrypt. Anyway, http:// should work.
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Cole Robinson crobinso@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |crobinso@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
--- Comment #6 from Cole Robinson crobinso@redhat.com --- Trimmed fedora-review output below, just some comments on the interesting stuff
Issues: ======= - Header files in -devel subpackage, if present. Note: libvirt-dbus-debugsource: ....
Seems like a fedora-review error, not knowing about debugsource maybe
- Sources used to build the package match the upstream source, as provided in the spec URL. Note: Upstream MD5sum check error, diff is in /home/crobinso/libvirt- dbus/diff.txt See: http://fedoraproject.org/wiki/Packaging/SourceURL
I didn't confirm this, but maybe you used a manual 'make dist' archive and not the published one?
- All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. Note: These BR are not needed: gcc See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
Another fedora-review issue, BR gcc is correct for latest fedora
===== MUST items =====
Generic: [ ]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/dbus-1/system- services, /usr/share/dbus-1, /usr/share/dbus-1/services, /etc/polkit-1/rules.d, /usr/share/dbus-1/system.d, /usr/share/dbus-1/interfaces, /etc/polkit-1
These are dbus and polkit owned dirs. Nothing else owns them on my system. Maybe Requires: dbus and Requires: polkit make this go away but I don't think it's interesting
[ ]: Changelog in prescribed format.
Add one :)
===== SHOULD items =====
[ ]: Spec use %global instead of %define unless justified. Note: %define requiring justification: %define glib2_version 2.44.0, %define libvirt_version 1.2.8, %define libvirt_glib_version 0.0.7, %define system_user libvirtdbus
Needs fixing
[x]: Uses parallel make %{?_smp_mflags} macro.
I think there's also a %make_build macro that does this but I haven't used it.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment).
I looked them over, nothing interesting IMO
After those minor tweaks I'll re-review, should be good. I can sponsor as well
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
--- Comment #7 from Pavel Hrdina phrdina@redhat.com --- Thanks for the review, I've fixed all the issues and updated the files on server, the links in Comment 1 should point to the fixed files.
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #8 from Neal Gompa ngompa13@gmail.com --- (In reply to Iwicki Artur from comment #1)
%__make %{?_smp_mflags}
Macro forms of system executables are discouraged. https://fedoraproject.org/wiki/Packaging:Guidelines#Macros
Note, %make_build is preferred too.
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Cole Robinson crobinso@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841 (FE-NEEDSPONSOR) | Flags| |fedora-review+
--- Comment #9 from Cole Robinson crobinso@redhat.com --- Looks good to me, setting fedora-review+
I can sponsor Pavel, so removing NEEDSPONSOR
Admins, please also add virt-maint@lists.fedoraproject.org to the package CC list, that was pkgdb group::virtmaint-sig but I have no idea if that changed in pagure world
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
--- Comment #10 from Cole Robinson crobinso@redhat.com --- I've added/sponsored Pavel for the packagers group now
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
--- Comment #11 from Gwyn Ciesla limburgher@gmail.com --- (fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/libvirt-dbus
https://bugzilla.redhat.com/show_bug.cgi?id=1559786
Pavel Hrdina phrdina@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |NEXTRELEASE Last Closed| |2018-03-27 07:23:04
--- Comment #12 from Pavel Hrdina phrdina@redhat.com --- Thanks, the package was successfully built for Fedora Rawhide.
https://koji.fedoraproject.org/koji/taskinfo?taskID=25997814
package-review@lists.fedoraproject.org