Spec URL: http://subversion.city-fan.org/repos/cfo-repo/ansible-collection-community-libvirt/trunk/ansible-collection-community-libvirt.spec SRPM URL: http://www.city-fan.org/~paul/extras/ansible-collection-community-libvirt/ansible-collection-community-libvirt-1.1.0-1.fc37.src.rpm Description: Manages virtual machines supported by libvirt. Fedora Account System Username: pghmcfc I couldn't see any packaging guidelines for ansible collections so I based the spec on that for the community.mysql collection (https://src.fedoraproject.org/rpms/ansible-collection-community-mysql/blob/rawhide/f/ansible-collection-community-mysql.spec).
As one of the Fedora ansible maintainers, I figured I'd take this review. If you could add me (@gotmax23) as an admin to the package when you import it, I would appreciate it. I'm re-pasting the URLs, because your link to the ansible-collection-community-mysql specfile throws off fedora-review. Spec URL: http://subversion.city-fan.org/repos/cfo-repo/ansible-collection-community-libvirt/trunk/ansible-collection-community-libvirt.spec SRPM URL: http://www.city-fan.org/~paul/extras/ansible-collection-community-libvirt/ansible-collection-community-libvirt-1.1.0-1.fc37.src.rpm
> # Avoid inclusion of package note file > %undefine _package_note_file I don't think this is necessary. There are no binaries built. I would recommend mentioning that it's an ansible collection somewhere in the `Summary:`/`%description`. You can take a look at https://src.fedoraproject.org/rpms/ansible-collection-community-docker/blob/rawhide/f/ansible-collection-community-docker.spec which I maintain for an example. This is admittedly a bit of a nitpick. I also attached a patch to change the way that the specfile handles removing unnecessary files and enables unit tests. I would like to eventually add a macro to ansible-packaging that automates running unit tests for ansible collections. I will look into writing packaging guidelines, as well.
Created attachment 1879895 [details] Review suggestions
Hi Maxwell, thanks for the suggestions. The package note thing is strange: if I do a local mock build I end up with a package note file, but not from a koji build. Here's an rpmdiff between a local and koji build: $ rpmdiff ~/ansible-collection-community-libvirt-1.1.0-1.fc37.noarch.rpm ansible-collection-community-libvirt-1.1.0-1.fc37.noarch.rpm ..........T /usr/share/ansible/collections/ansible_collections/community ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt removed /usr/share/ansible/collections/ansible_collections/community/libvirt/.package_note-ansible-collection-community-libvirt-1.1.0-1.fc37.x86_64.ld ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/.pyproject-builddir ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/CHANGELOG.rst ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/CONTRIBUTING.md ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/COPYING S.5.......T /usr/share/ansible/collections/ansible_collections/community/libvirt/FILES.json ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/LICENSE ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/MAINTAINERS ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/MAINTAINING.md ..5.......T /usr/share/ansible/collections/ansible_collections/community/libvirt/MANIFEST.json ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/README.md ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/REVIEW_CHECKLIST.md ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/changelogs ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/changelogs/changelog.yaml ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/changelogs/config.yaml ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/meta ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/meta/runtime.yml ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/connection ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/connection/libvirt_lxc.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/connection/libvirt_qemu.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/doc_fragments ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/doc_fragments/__init__.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/doc_fragments/requirements.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/doc_fragments/virt.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/inventory ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/inventory/__init__.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/inventory/libvirt.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/modules ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/modules/virt.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/modules/virt_net.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/plugins/modules/virt_pool.py ..........T /usr/share/ansible/collections/ansible_collections/community/libvirt/shippable.yml ..........T /usr/share/doc/ansible-collection-community-libvirt ..........T /usr/share/licenses/ansible-collection-community-libvirt Any idea why that should happen? I also end up getting a directory /usr/share/ansible/collections/ansible_collections/community/libvirt/.pyproject-builddir in the package; can we get rid of that too?
Another thing: %{ansible_collection_url} isn't expanded in EPEL-9 (at least with local builds): $ rpm -qip ~/ansible-collection-community-libvirt-1.1.0-2.el9.src.rpm Name : ansible-collection-community-libvirt Version : 1.1.0 Release : 2.el9 Architecture: noarch Install Date: (not installed) Group : Unspecified Size : 67384 License : GPLv3+ Signature : (none) Source RPM : (none) Build Date : Mon 16 May 2022 10:54:35 BST Build Host : tycho.city-fan.org URL : %{ansible_collection_url} Summary : Manages virtual machines supported by libvirt Description : Manages virtual machines supported by libvirt.
Updated packages: Spec URL: http://subversion.city-fan.org/repos/cfo-repo/ansible-collection-community-libvirt/trunk/ansible-collection-community-libvirt.spec SRPM URL: http://www.city-fan.org/~paul/extras/ansible-collection-community-libvirt/ansible-collection-community-libvirt-1.1.0-2.fc37.src.rpm
(In reply to Paul Howarth from comment #4) > Hi Maxwell, thanks for the suggestions. > Any idea why that should happen? I'm not sure why this would happen. There might be some package update that's related to this in the koji buildroot but not your mock chroot. You can pass `--enablerepo=local` to mock to pull in the koji buildroot packages and see if that fixes the problem. > I also end up getting a directory > /usr/share/ansible/collections/ansible_collections/community/libvirt/. > pyproject-builddir in the package; can we get rid of that too? That's a good point. I also need to fix this in one of my collections. Yes, you can add `- .pyproject-builddir` to the galaxy.yml `build_ignore:` list. ansible-galaxy will add all files to the built collection that you don't explicitly delete or exclude. I think there was some talk upstream about fixing this arguably broken behavior (i.e. we should make adding extra files not actually run by ansible opt-in instead of opt-out), but I don't think it went anywhere. (In reply to Paul Howarth from comment #5) > Another thing: %{ansible_collection_url} isn't expanded in EPEL-9 (at least > with local builds): Do you have `ansible-packaging` installed? I don't think it's part of the default buildroot. I would revert the change you made to replace %{ansible_collection_url}.
> %pyproject_buildrequires -N %{python3_sitelib}/ansible_test/_data/requirements/units.txt\ It looks there's an extra space after the `-N`. This might have been my fault. It's purely cosmetic, but I noticed it and figured I'd point it out.
(In reply to Maxwell G from comment #7) > (In reply to Paul Howarth from comment #4) > > Hi Maxwell, thanks for the suggestions. > > Any idea why that should happen? > > I'm not sure why this would happen. There might be some package update > that's related to this in the koji buildroot but not your mock chroot. You > can pass `--enablerepo=local` to mock to pull in the koji buildroot packages > and see if that fixes the problem. It didn't help :-( > (In reply to Paul Howarth from comment #5) > > Another thing: %{ansible_collection_url} isn't expanded in EPEL-9 (at least > > with local builds): > > Do you have `ansible-packaging` installed? I don't think it's part of the > default buildroot. I would revert the change you made to replace > %{ansible_collection_url}. I do have ansible-packaging installed but I don't think that's related because I can rebuild (in mock) the same SRPM for Fedora and EPEL-9 (using a CentOS Stream buildroot) and the latter doesn't get %{ansible_collection_url} expanded in the resulting rebuilt SRPM but the former does. Hence I'd rather keep the URL as it is until we can figure out how to fix that. (In reply to Maxwell G from comment #8) > > %pyproject_buildrequires -N %{python3_sitelib}/ansible_test/_data/requirements/units.txt\ > > It looks there's an extra space after the `-N`. This might have been my > fault. It's purely cosmetic, but I noticed it and figured I'd point it out. I'll fix that next time I update the spec.
(In reply to Paul Howarth from comment #9) > (In reply to Maxwell G from comment #7) > > (In reply to Paul Howarth from comment #4) > > > Hi Maxwell, thanks for the suggestions. > > > Any idea why that should happen? > > > > I'm not sure why this would happen. There might be some package update > > that's related to this in the koji buildroot but not your mock chroot. You > > can pass `--enablerepo=local` to mock to pull in the koji buildroot packages > > and see if that fixes the problem. > > It didn't help :-( I'm not sure then. We can manually disable/exclude the package note file for now. I believe this @zbyszek's area of expertise, so maybe you can ask him. > > (In reply to Paul Howarth from comment #5) > > > Another thing: %{ansible_collection_url} isn't expanded in EPEL-9 (at least > > > with local builds): > > > > Do you have `ansible-packaging` installed? I don't think it's part of the > > default buildroot. I would revert the change you made to replace > > %{ansible_collection_url}. > > I do have ansible-packaging installed but I don't think that's related > because I can rebuild (in mock) the same SRPM for Fedora and EPEL-9 (using a > CentOS Stream buildroot) and the latter doesn't get > %{ansible_collection_url} expanded in the resulting rebuilt SRPM but the > former does. Hence I'd rather keep the URL as it is until we can figure out > how to fix that. That's fine with me. I will try to look into it. > (In reply to Maxwell G from comment #8) > > > %pyproject_buildrequires -N %{python3_sitelib}/ansible_test/_data/requirements/units.txt\ > > > > It looks there's an extra space after the `-N`. This might have been my > > fault. It's purely cosmetic, but I noticed it and figured I'd point it out. > > I'll fix that next time I update the spec. +1
This looks good to me besides the licensing issue. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: ======= - If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. Note: License file COPYING is not marked as %license See: https://docs.fedoraproject.org/en-US/packaging- guidelines/LicensingGuidelines/#_license_text It seems like there are two copies of the GPLv3. I think we should include both of them and then query upstream to remove one instead of doing so ourselves. What do you think? ===== MUST items ===== Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU General Public License v3.0 or later", "*No copyright* GNU General Public License v3.0 or later". 88 files have unknown license. Detailed output of licensecheck in /home/gotmax/Sync/git- repos/packaging/fedora_rpms/review.repos/2086299-ansible-collection- community-libvirt/licensecheck.txt [x]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/share/ansible/collections/ansible_collections/community(ansible- collection-community-docker, ansible-collection-community-general, ansible-collection-community-rabbitmq, ansible-collection-community- kubernetes, ansible-collection-community-mysql) It is fine for packages that don't depend on eachother to co-own directories. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. Note that ansible-packaging generates the dependency on ansible(-core) itself. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 3 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. Yay! [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/ansible-collections/community.libvirt/archive/1.1.0/ansible-collection-community-libvirt-1.1.0.tar.gz : CHECKSUM(SHA256) this package : a3a998cf28889edb73fc300c4b6d5ed505cc2d733e570bb44ef5214cf4420e40 CHECKSUM(SHA256) upstream package : a3a998cf28889edb73fc300c4b6d5ed505cc2d733e570bb44ef5214cf4420e40 Requires -------- ansible-collection-community-libvirt (rpmlib, GLIBC filtered): (ansible-core or (ansible < 2.10.0 with ansible >= 2.9.10)) Provides -------- ansible-collection-community-libvirt: ansible-collection(community.libvirt) ansible-collection-community-libvirt Generated by fedora-review 0.8.0 (e988316) last change: 2022-04-07 Command line :/usr/bin/fedora-review -b 2086299 Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, Generic Disabled plugins: Python, Ocaml, Haskell, fonts, Java, R, Perl, SugarActivity, C/C++, PHP Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
(In reply to Maxwell G from comment #11) > This looks good to me besides the licensing issue. ... > It seems like there are two copies of the GPLv3. I think we should include > both of them and then query upstream to remove one instead of doing so > ourselves. What do you think? Done. https://github.com/ansible-collections/community.libvirt/issues/122 I figured out what was going on with the %{ansible_collection_url} on the SRPM for the EPEL-9 build. The SRPM is normally generated using the minimal buildroot, which does not contain ansible-packaging and hence a definition of %{ansible_collection_url}. The macro gets expanded for the Fedora build because it uses dynamic buildrequires and so the SRPM is regenerated with ansible-packaging present. The normal way to address this would be to have an ansible-srpm-macros package that contained the definition of %{ansible_collection_url} and have redhat-rpm-config (and the equivalent package for EPEL) require ansible-srpm-macros, which would pull it into the minimal buildroot for the SRPM build. For now though I worked around it by using %generate_buildrequires unconditionally. If there's ever a need to support EPEL prior to 9, that could be done in a different way in the appropriate branch. Updated packages: Spec URL: http://subversion.city-fan.org/repos/cfo-repo/ansible-collection-community-libvirt/trunk/ansible-collection-community-libvirt.spec SRPM URL: http://www.city-fan.org/~paul/extras/ansible-collection-community-libvirt/ansible-collection-community-libvirt-1.1.0-3.fc37.src.rpm
(In reply to Paul Howarth from comment #12) > I figured out what was going on with the %{ansible_collection_url} on the > SRPM for the EPEL-9 build. The SRPM is normally generated using the minimal > buildroot, which does not contain ansible-packaging and hence a definition > of %{ansible_collection_url}. The macro gets expanded for the Fedora build > because it uses dynamic buildrequires and so the SRPM is regenerated with > ansible-packaging present. The normal way to address this would be to have > an ansible-srpm-macros package that contained the definition of > %{ansible_collection_url} and have redhat-rpm-config (and the equivalent > package for EPEL) require ansible-srpm-macros, which would pull it into the > minimal buildroot for the SRPM build. For now though I worked around it by > using %generate_buildrequires unconditionally. If there's ever a need to > support EPEL prior to 9, that could be done in a different way in the > appropriate branch. Thanks for figuring this out! I have made the appropriate changes to ansible-packaging to split out that macro into ansible-srpm-macros and am waiting on my PRs to redhat-rpm-config and epel-rpm-macros that adds that new package to the buildroot to be approved. I would like this package in epel8, but we shouldn't need this workaround for too long. If you could add me (@gotmax23) as an admin to the package when you import it, I would appreciate it. Anyways, this package is approved!
Thanks for the review. I'll add you as admin once the repo is created. $ fedpkg request-repo ansible-collection-community-libvirt 2086299 https://pagure.io/releng/fedora-scm-requests/issue/44493 $ fedpkg request-branch --repo ansible-collection-community-libvirt f36 https://pagure.io/releng/fedora-scm-requests/issue/44494 $ fedpkg request-branch --repo ansible-collection-community-libvirt f35 https://pagure.io/releng/fedora-scm-requests/issue/44495 $ fedpkg request-branch --repo ansible-collection-community-libvirt epel9 https://pagure.io/releng/fedora-scm-requests/issue/44496 $ fedpkg request-branch --repo ansible-collection-community-libvirt epel8 https://pagure.io/releng/fedora-scm-requests/issue/44497
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/ansible-collection-community-libvirt
FEDORA-2022-7ca54eca48 has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-7ca54eca48
FEDORA-2022-d21a504070 has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2022-d21a504070
FEDORA-EPEL-2022-2d1296b5f2 has been submitted as an update to Fedora EPEL 9. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-2d1296b5f2
FEDORA-EPEL-2022-a9ccbc0012 has been submitted as an update to Fedora EPEL 8. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-a9ccbc0012
FEDORA-2022-d21a504070 has been pushed to the Fedora 35 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2022-d21a504070 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-d21a504070 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-7ca54eca48 has been pushed to the Fedora 36 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2022-7ca54eca48 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-7ca54eca48 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2022-2d1296b5f2 has been pushed to the Fedora EPEL 9 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-2d1296b5f2 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2022-a9ccbc0012 has been pushed to the Fedora EPEL 8 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-a9ccbc0012 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-7ca54eca48 has been pushed to the Fedora 36 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-d21a504070 has been pushed to the Fedora 35 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-EPEL-2022-2d1296b5f2 has been pushed to the Fedora EPEL 9 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-EPEL-2022-a9ccbc0012 has been pushed to the Fedora EPEL 8 stable repository. If problem still persists, please make note of it in this bug report.