Spec URL: https://passt.top/passt/tree/contrib/fedora/passt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/srpm-builds/04600401/passt-0.git.2022_07_06.27aec59-0.fc35.src.rpm Description: passt implements a translation layer between a Layer-2 network interface and native Layer-4 sockets (TCP, UDP, ICMP/ICMPv6 echo) on a host. It doesn't require any capabilities or privileges, and it can be used as a simple replacement for Slirp. pasta (same binary as passt, different command) offers equivalent functionality, for network namespaces: traffic is forwarded using a tap interface inside the namespace, without the need to create further interfaces on the host, hence not requiring any capabilities or privileges. Fedora Account System Username: sbrivio Copr link: https://copr.fedorainfracloud.org/coprs/sbrivio/passt/
This is my first Fedora package and I need sponsorship. I'm the upstream maintainer and author.
Ensure files can be downloaded Spec URL: https://passt.top/passt/plain/contrib/fedora/passt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04600401-passt/passt-0.git.2022_07_06.27aec59-0.fc37.src.rpm
Spec URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04600401-passt/passt.spec
Unofficial Review: Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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. [!]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GNU Affero General Public License v3.0", "BSD 3-Clause License GNU Affero General Public License v3.0", "GNU Affero General Public License v3.0 [generated file]", "BSD 3-Clause License", "Unknown or generated", "*No copyright* GNU Affero General Public License v3.0", "*No copyright* Apache License 2.0". 13 files have unknown license. Detailed output of licensecheck in /home/FedoraPackaging/passt/2106611-passt/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [?]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [?]: Package requires other packages for directories it uses. Note: No known owner of /usr/share/selinux/packages/passt [?]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/selinux/packages, /usr/share/selinux, /usr/share/selinux/packages/passt [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [!]: 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. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [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 61440 bytes in 2 files. [?]: 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]: 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. [x]: Package does not own files or directories owned by other packages. [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: [-]: 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). [?]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in passt- selinux [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Scriptlets must be sane, if used. [-]: 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. [!]: %check is present and all tests pass. [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 debuginfo package(s). Note: There are rpmlint messages (see attachment). [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://passt.top/passt/snapshot/passt-HEAD.tar.xz : CHECKSUM(SHA256) this package : 3adf86bbac914edf760d74f0f0c4decb6e24fa030b c972a0417af7345dbf3230 CHECKSUM(SHA256) upstream package : 3adf86bbac914edf760d74f0f0c4decb6e24fa030b c972a0417af7345dbf3230 Requires -------- passt (rpmlib, GLIBC filtered): libc.so.6()(64bit) rtld(GNU_HASH) passt-selinux (rpmlib, GLIBC filtered): /bin/sh passt policycoreutils passt-debuginfo (rpmlib, GLIBC filtered): passt-debugsource (rpmlib, GLIBC filtered): Provides -------- passt: passt passt(x86-64) passt-selinux: passt-selinux passt-selinux(x86-64) passt-debuginfo: debuginfo(build-id) passt-debuginfo passt-debuginfo(x86-64) passt-debugsource: passt-debugsource passt-debugsource(x86-64) Generated by fedora-review 0.8.0 (e988316) last change: 2022-04-07 Command line :/usr/bin/fedora-review -b 2106611 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, C/C++, Shell-api Disabled plugins: Python, Java, fonts, PHP, SugarActivity, R, Ocaml, Haskell, Perl Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH Comments: a) Consider packaging demo.sh with the documentation as it is a useful getting started example b) Can any of the tests run without direct network access? If so they should be used. c) {{{ git_dir_changelog }}} does not appear in https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04600401-passt/passt.spec d) The README.md file has many scripts that would not work in the terminal. Maybe these should be removed? Generating an HTML file for use on a webpage from a markdown file would improve maintainability. e) Packaging specific commits is reasonable, though specifying releases or tagging some commits as releases would also help with maintainability and knowing when to update, especially if updates will also be done by people who are not upstream developers f) The files passt-HEAD/contrib/kata-containers/0001-virtcontainers-agent-Add-passt-networking-model-and-.patch passt-HEAD/contrib/podman/0001-libpod-Add-pasta-networking-mode.patch contain Apache 2.0 licenses. Is it worth packaging these? g) Should header files be in a devel package? h) It may be helpful to explain the mixed licensing policy
Just a remark: This source-URL: Source: https://passt.top/passt/snapshot/passt-HEAD.tar.xz is non-deterministic and therefore not acceptable in Fedora.
Benson, Ralf, Thanks a lot for the unofficial review and comments! I'll get back to you within a couple of days.
> Spec URL: https://passt.top/passt/tree/contrib/fedora/passt.spec 1. The links you post as part of a Review Request should point to "plain"/"raw" files, not HTML renditions. 2. The spec file should match the one used to build the linked SRPM, i.e. it cannot be a template that needs filling out. > Release: 0%{?dist} This must start at 1, not 0. https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_simple_versioning > Group: System Environment/Daemons > VCS: git://passt.top/passt Not used in Fedora. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections > Source: https://passt.top/passt/snapshot/passt-HEAD.tar.xz This link points to the latest version of passt, hence the file will change over time. You need to use a "static" link referring to a specific commit or git tag. > %package selinux This should probably be marked as "BuildArch: noarch", otherwise you end up with a separate passt-selinux package for each architecture. > %build > export CFLAGS="%{optflags}" This is not needed. Starting with Fedora 36, this is done automatically at start of %build and %check. If you insist on doing this, the proper way is to use the "%set_build_flags" macro, which will set up not only CFLAGS, but also LDFLAGS. > %{_mandir}/man1/passt.1.* > %{_mandir}/man1/pasta.1.* > %{_mandir}/man1/qrap.1.* This wildcard can match any compression method, but will fail if the man pages are *not compressed*. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages
Thanks for all the reviews! I made a number of changes, and I finally have an updated spec files hopefully addressing all the pending comments. Spec URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04752883-passt/passt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04752883-passt/passt-0.git.2022_08_21.7b71094-1.fc38.src.rpm Benson, (In reply to Benson Muite from comment #4) > > [...] > > Comments: > a) Consider packaging demo.sh with the documentation as it is a useful > getting started example The existing demo.sh was largely outdated as I pretty much forgot about it. I rewrote it, and it's included in the package now: %doc %{_docdir}/passt/demo.sh ...I had a look around, it seems that /usr/share/doc/ is the most common place for this kind of stuff. > b) Can any of the tests run without direct network access? If so they should > be used. Unfortunately, not yet. I looked into it, it's definitely doable, but it involves a significant amount of changes which would take even longer. I'll keep this in mind, though, and try to get there in a near future. > c) {{{ git_dir_changelog }}} does not appear in > https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora- > rawhide-x86_64/04600401-passt/passt.spec Fixed. > d) The README.md file has many scripts that would not work in the terminal. > Maybe these should be removed? Generating an HTML file for use on a webpage > from a markdown file would improve maintainability. The package now includes a "plain" Markdown version, that's automatically sourced from the web-oriented README.md. That should be clean enough, even though it's not an ideal solution in the long term. I think it would be better to build the web-oriented README.md from a plain version (that is, the other way around). However, the project homepage is based on cgit, and cgit needs the file to be in the tree as it is. I still need to look into a solution for this. > e) Packaging specific commits is reasonable, though specifying releases or > tagging some commits as releases would also help with maintainability and > knowing when to update, especially if updates will also be done by people > who are not upstream developers I started tagging specific commits on push, before I trigger Copr builds. The format of the tag is DATE.SHORT_SHA, with date being the ISO 8601 representation of the commit date of HEAD, and SHORT_SHA its 7-digit commit SHA. The rpkg macros can then operate on those tags. > f) The files > passt-HEAD/contrib/kata-containers/0001-virtcontainers-agent-Add-passt- > networking-model-and-.patch > passt-HEAD/contrib/podman/0001-libpod-Add-pasta-networking-mode.patch > contain Apache 2.0 licenses. Is it worth packaging these? I don't think so. I licensed both as Apache 2.0 to avoid licensing compatibility issues with the projects at hand (both Kata Containers and Podman use Apache 2.0 licenses). However, my plan is to submit the Podman integration directly to the Podman maintainers once packages are widely available. As to Kata Containers, that patch is outdated now and I doubt it would actually benefit users like that. > g) Should header files be in a devel package? I guess not, passt doesn't export any API and can't be used as a library. > h) It may be helpful to explain the mixed licensing policy Actually, there's no mixed licensing -- the project license is AGPL 3.0. I originally took one function (in checksum.c) from the BESS ("Berkeley Extensible Software Switch") project, distributed under the terms of the 3-Clause BSD license, which is compatible with the AGPL 3.0 (https://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses). My understanding is that the "License:" tag of the spec file needs to include all the licenses of files that are used for the binary build (https://docs.fedoraproject.org/en-US/legal/license-field/#_conjunctive_and_licensing). Does that look correct to you? -- Ralf, (In reply to Ralf Corsepius from comment #5) > Just a remark: This source-URL: > Source: https://passt.top/passt/snapshot/passt-HEAD.tar.xz > is non-deterministic and therefore not acceptable in Fedora. Thanks for pointing this out. I fixed that, the current Source: tag points to: https://passt.top/passt/snapshot/passt-7b710946b152fabab0f3c838e5518576beb9020c.tar.xz -- Artur, (In reply to Artur Frenszek-Iwicki from comment #7) > > Spec URL: https://passt.top/passt/tree/contrib/fedora/passt.spec > 1. The links you post as part of a Review Request should point to > "plain"/"raw" files, not HTML renditions. I totally forgot, I now posted links to plain files. > 2. The spec file should match the one used to build the linked SRPM, i.e. it > cannot be a template that needs filling out. Right, I posted a link to the actual spec file. > > Release: 0%{?dist} > This must start at 1, not 0. > https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ > #_simple_versioning Fixed. > > Group: System Environment/Daemons > > VCS: git://passt.top/passt > Not used in Fedora. > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections Dropped. > > Source: https://passt.top/passt/snapshot/passt-HEAD.tar.xz > This link points to the latest version of passt, hence the file will change > over time. > You need to use a "static" link referring to a specific commit or git tag. Fixed, right now it points to: https://passt.top/passt/snapshot/passt-7b710946b152fabab0f3c838e5518576beb9020c.tar.xz > > %package selinux > This should probably be marked as "BuildArch: noarch", otherwise you end up > with a separate passt-selinux package for each architecture. Changed. > > %build > > export CFLAGS="%{optflags}" > This is not needed. Starting with Fedora 36, this is done automatically at > start of %build and %check. > If you insist on doing this, the proper way is to use the "%set_build_flags" > macro, > which will set up not only CFLAGS, but also LDFLAGS. Thanks for the pointer, I switched it to %set_build_flags, as the same spec file is currently used for other RPM-based distributions as well. > > %{_mandir}/man1/passt.1.* > > %{_mandir}/man1/pasta.1.* > > %{_mandir}/man1/qrap.1.* > This wildcard can match any compression method, but will fail if the man > pages are *not compressed*. > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages Fixed.
(In reply to Stefano Brivio from comment #8) > Thanks for all the reviews! I made a number of changes, and I finally have > an updated spec files hopefully addressing all the pending comments. > > Spec URL: > https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora- > rawhide-x86_64/04752883-passt/passt.spec > SRPM URL: > https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora- > rawhide-x86_64/04752883-passt/passt-0.git.2022_08_21.7b71094-1.fc38.src.rpm This versioning scheme doesn't match documented rules IMHO: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots My interpretation of the rules is that it is expected to look like: 0^20220821.git7b71094 ('git' can be reduced to just 'g' if desired) In the first line of the spec it says > # SPDX-License-Identifier: AGPL-3.0-or-later While it is allowed, it is really very unusual to put an explicit license on the spec file. Every spec I've ever looked at has no license and is thus considered to be MIT licensed https://fedoraproject.org/wiki/Licensing:Main#License_of_Fedora_SPEC_Files I think that it is preferrable to stick with normal Fedora practice, given that parts of the spec file are commonly cargo cult copied across packages when new rules are applied. > # contrib/fedora/passt.spec - Example spec file for fedora This line can go, it isn't an example spec, it is the actual official spec. > Source: https://passt.top/passt/snapshot/passt-7b710946b152fabab0f3c838e5518576beb9020c.tar.xz This commit hash is used in multiple places, so best practice would be to define this at the top of the file: %global git_hash 7b710946b152fabab0f3c838e5518576beb9020c %global git_shorthash 7b71094 and then reference %{git_hash} or %{git_shorthash} throughout the file. > %package selinux > BuildArch: noarch > Summary: SELinux support for passt and pasta > Requires: %{name} = %{version} This Requires should be fully versioned, ie Requires: %{name} = %{version}.%{release} See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package > %if 0%{?suse_version} > 910 > %make_install DESTDIR=%{buildroot} prefix=%{_prefix} docdir=%{_prefix}/share/doc/packages/passt > %else > %make_install DESTDIR=%{buildroot} prefix=%{_prefix} > %endif Fedora specs would generally only be expected to contain conditionals for Fedora or RHEL, not any other RPM based distro, since Fedora & RHEL share the same packaging guidelines, but other distros do their own thing. Also setting prefix only affects one of the many variables your makefile uses: prefix ?= /usr/local exec_prefix ?= $(prefix) bindir ?= $(exec_prefix)/bin datarootdir ?= $(prefix)/share docdir ?= $(datarootdir)/doc/passt mandir ?= $(datarootdir)/man man1dir ?= $(mandir)/man1 I'd expect to override the ones relevant to 'install' functionality at least, even if Makefile's current defaults happen to match Fedora's %make_install prefix=%{_prefix} bindir=%{_bindir} mandir=%{_mandir} docdir=%{_docdir}/passt > %changelog > * Sun Aug 21 2022 Stefano Brivio <sbrivio> - 0.git.2022_08_21.7b71094-0 > - Use more GNU-style directory variables, explicit docdir for OpenSUSE > ...snip... Consider if you wish to use 'Release: %autorelease' and %autochangelog which populates from git commit messages.
(In reply to Daniel Berrangé from comment #9) > (In reply to Stefano Brivio from comment #8) > > Thanks for all the reviews! I made a number of changes, and I finally have > > an updated spec files hopefully addressing all the pending comments. > > > > Spec URL: > > https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora- > > rawhide-x86_64/04752883-passt/passt.spec > > SRPM URL: > > https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora- > > rawhide-x86_64/04752883-passt/passt-0.git.2022_08_21.7b71094-1.fc38.src.rpm > > This versioning scheme doesn't match documented rules IMHO: > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ > #_snapshots > > My interpretation of the rules is that it is expected to look like: > > 0^20220821.git7b71094 > > ('git' can be reduced to just 'g' if desired) I see now -- I didn't consider the "Snapshots" guidelines and observed the "Simple versioning" rules, but upstream git tags don't really qualify as releases I guess. Patch posted upstream. > In the first line of the spec it says > > > # SPDX-License-Identifier: AGPL-3.0-or-later > > While it is allowed, it is really very unusual to put an explicit license on > the spec file. Every spec I've ever looked at has no license and is thus > considered to be MIT licensed > > https://fedoraproject.org/wiki/Licensing:Main#License_of_Fedora_SPEC_Files > > I think that it is preferrable to stick with normal Fedora practice, given > that parts of the spec file are commonly cargo cult copied across packages > when new rules are applied. Dropped (patch posted upstream). This is something I originally added to make the project compliant with the REUSE specification (https://reuse.software/spec/), but there are now a few other files where adding explicit licensing information isn't really practical. I guess I'll try again later on with that. > > # contrib/fedora/passt.spec - Example spec file for fedora > > This line can go, it isn't an example spec, it is the actual official spec. > > > > Source: https://passt.top/passt/snapshot/passt-7b710946b152fabab0f3c838e5518576beb9020c.tar.xz > > This commit hash is used in multiple places, so best practice would be to > define this at the top of the file: > > %global git_hash 7b710946b152fabab0f3c838e5518576beb9020c > %global git_shorthash 7b71094 > > and then reference %{git_hash} or %{git_shorthash} throughout the file. Patch posted, defining %{git_hash}, not %{git_shorthash} because it appears only once, and it's actually populated by an rpkg macro that outputs the full version, so splitting that out looks a bit awkward. > > %package selinux > > BuildArch: noarch > > Summary: SELinux support for passt and pasta > > Requires: %{name} = %{version} > > This Requires should be fully versioned, ie > > Requires: %{name} = %{version}.%{release} > > See > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_requiring_base_package Fixed (with "%{version}-%{release}"), patch posted upstream. > > %if 0%{?suse_version} > 910 > > %make_install DESTDIR=%{buildroot} prefix=%{_prefix} docdir=%{_prefix}/share/doc/packages/passt > > %else > > %make_install DESTDIR=%{buildroot} prefix=%{_prefix} > > %endif > > Fedora specs would generally only be expected to contain conditionals for > Fedora or RHEL, not any other RPM based distro, since Fedora & RHEL share > the same packaging guidelines, but other distros do their own thing. I'm using the same spec file on Copr also for CentOS, Mageia and OpenSUSE Tumbleweed builds, and I find that really convenient, but sure, I see your point. In any case, I dropped that, because: > Also setting prefix only affects one of the many variables your makefile > uses: > > prefix ?= /usr/local > exec_prefix ?= $(prefix) > bindir ?= $(exec_prefix)/bin > datarootdir ?= $(prefix)/share > docdir ?= $(datarootdir)/doc/passt > mandir ?= $(datarootdir)/man > man1dir ?= $(mandir)/man1 > > I'd expect to override the ones relevant to 'install' functionality at > least, even if Makefile's current defaults happen to match Fedora's > > %make_install prefix=%{_prefix} bindir=%{_bindir} mandir=%{_mandir} > docdir=%{_docdir}/passt ...this suggestion should actually fix the issue I had with OpenSUSE Tumbleweed as well. Patch pending upstream now. > > %changelog > > * Sun Aug 21 2022 Stefano Brivio <sbrivio> - 0.git.2022_08_21.7b71094-0 > > - Use more GNU-style directory variables, explicit docdir for OpenSUSE > > ...snip... > > Consider if you wish to use 'Release: %autorelease' and %autochangelog > which populates from git commit messages. I didn't know about those, thanks for mentioning them. %autochangelog, however, is a bit limited compared to the current rpkg macro I'm using: most importantly, it doesn't generate links to upstream changes, which, I think, are a nice addition. I left this part as it was.
FYI, I'm volunteering to sponsor Stefano after this package review completes.
(In reply to Stefano Brivio from comment #10) > (In reply to Daniel Berrangé from comment #9) > > > > [...] > > > > I'd expect to override the ones relevant to 'install' functionality at > > least, even if Makefile's current defaults happen to match Fedora's > > > > %make_install prefix=%{_prefix} bindir=%{_bindir} mandir=%{_mandir} > > docdir=%{_docdir}/passt > > ...this suggestion should actually fix the issue I had with OpenSUSE > Tumbleweed as well. Patch pending upstream now. ...and it did! Thanks for the suggestion, it's cleaner now. Updated spec file and SRPM: Spec URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04801146-passt/passt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04801146-passt/passt-0%5E20220830.g0cb795e-1.fc38.src.rpm
Just three pretty minor problems left to address: Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== ISSUES ===== [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/share/selinux/packages/passt, /usr/share/doc/passt => Needs %dir entries for these two [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/selinux/packages/passt, /usr/share/doc/passt, /usr/share/selinux, /usr/share/selinux/packages => Needs a Requires: selinux-policy [!]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in passt- selinux => Needs %{?_isa} bit adding ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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: "GNU Affero General Public License v3.0", "BSD 3-Clause License GNU Affero General Public License v3.0", "GNU Affero General Public License v3.0 [generated file]", "BSD 3-Clause License", "Unknown or generated", "*No copyright* GNU Affero General Public License v3.0", "*No copyright* Apache License 2.0". 15 files have unknown license. Detailed output of licensecheck in /home/berrange/2106611-passt/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/share/selinux/packages/passt, /usr/share/doc/passt [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/selinux/packages/passt, /usr/share/doc/passt, /usr/share/selinux, /usr/share/selinux/packages [x]: %build honors applicable compiler flags or justifies otherwise. [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. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [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 30720 bytes in 2 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]: 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. [x]: Package does not own files or directories owned by other packages. [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: [-]: 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). [!]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in passt- selinux [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Scriptlets must be sane, if used. [-]: 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. [-]: %check is present and all tests pass. [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 debuginfo package(s). Note: There are rpmlint messages (see attachment). [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Cannot parse rpmlint output: Rpmlint (debuginfo) ------------------- Cannot parse rpmlint output: Rpmlint (installed packages) ---------------------------- /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) ============================ rpmlint session starts ============================ rpmlint: 2.2.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 4 passt-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/.dwz/passt-0^20220830.g0cb795e-1.fc38.x86_64 passt-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/passt-0^20220830.g0cb795e-1.fc38.x86_64.debug passt-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/passt.avx2-0^20220830.g0cb795e-1.fc38.x86_64.debug passt-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/qrap-0^20220830.g0cb795e-1.fc38.x86_64.debug passt-debuginfo.x86_64: E: statically-linked-binary /usr/lib/debug/.dwz/passt-0^20220830.g0cb795e-1.fc38.x86_64 passt-selinux.noarch: W: no-documentation passt-debuginfo.x86_64: W: no-documentation passt-debugsource.x86_64: W: no-documentation passt-debuginfo.x86_64: E: missing-PT_GNU_STACK-section /usr/lib/debug/.dwz/passt-0^20220830.g0cb795e-1.fc38.x86_64 passt-debuginfo.x86_64: E: ldd-failed /usr/lib/debug/usr/bin/passt-0^20220830.g0cb795e-1.fc38.x86_64.debug /usr/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) ldd: warning: you do not have execution permission for `/usr/lib/debug/usr/bin/passt-0^20220830.g0cb795e-1.fc38.x86_64.debug' passt-debuginfo.x86_64: E: ldd-failed /usr/lib/debug/usr/bin/passt.avx2-0^20220830.g0cb795e-1.fc38.x86_64.debug /usr/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) ldd: warning: you do not have execution permission for `/usr/lib/debug/usr/bin/passt.avx2-0^20220830.g0cb795e-1.fc38.x86_64.debug' passt-debuginfo.x86_64: E: ldd-failed /usr/lib/debug/usr/bin/qrap-0^20220830.g0cb795e-1.fc38.x86_64.debug /usr/bin/bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8) ldd: warning: you do not have execution permission for `/usr/lib/debug/usr/bin/qrap-0^20220830.g0cb795e-1.fc38.x86_64.debug' passt-debuginfo.x86_64: W: hidden-file-or-dir /usr/lib/debug/.dwz passt-debuginfo.x86_64: W: hidden-file-or-dir /usr/lib/debug/.dwz passt-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/7c/7a0b0dd868bfcebbf5f1ba59ca50c145121688 ../../../.build-id/7c/7a0b0dd868bfcebbf5f1ba59ca50c145121688 passt-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/a7/d90f323134d1b902de6d129afa589cee43a4d7 ../../../.build-id/a7/d90f323134d1b902de6d129afa589cee43a4d7 passt-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/f8/588fadfb4f4f32aeeba8431d3f6fda3a26451e ../../../.build-id/f8/588fadfb4f4f32aeeba8431d3f6fda3a26451e 4 packages and 0 specfiles checked; 5 errors, 12 warnings, 5 badness; has taken 1.3 s Source checksums ---------------- https://passt.top/passt/snapshot/passt-0cb795e432f6b318b07a0e4878e041233e7d7f7c.tar.xz : CHECKSUM(SHA256) this package : 599217bc9a218f2e5c0840d9810db9853a795bf67e7b5c9f0fc989a1e191312c CHECKSUM(SHA256) upstream package : 599217bc9a218f2e5c0840d9810db9853a795bf67e7b5c9f0fc989a1e191312c Requires -------- passt (rpmlib, GLIBC filtered): libc.so.6()(64bit) rtld(GNU_HASH) passt-selinux (rpmlib, GLIBC filtered): /bin/sh passt policycoreutils passt-debuginfo (rpmlib, GLIBC filtered): passt-debugsource (rpmlib, GLIBC filtered): Provides -------- passt: passt passt(x86-64) passt-selinux: passt-selinux passt-debuginfo: debuginfo(build-id) passt-debuginfo passt-debuginfo(x86-64) passt-debugsource: passt-debugsource passt-debugsource(x86-64) Generated by fedora-review 0.9.0 (6761b6c) last change: 2022-08-23 Command line :/usr/bin/fedora-review -b 2106611 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: R, Python, PHP, Ocaml, SugarActivity, fonts, Java, Haskell, Perl Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
> I didn't know about those, thanks for mentioning them. %autochangelog, however, is a bit limited compared to the current rpkg macro I'm using: most importantly, it doesn't generate links to upstream changes, which, I think, are a nice addition. I left this part as it was. Just FYI, the rpkg macros are not available in Fedora's build system (only in COPR) and they are no longer maintained. If you want to automate Release / %changelog, then rpmautospec (%autorelease / %autochangelog) is currently the only officially supported way to do that.
(In reply to Daniel Berrangé from comment #13) > ===== ISSUES ===== > > [!]: Package requires other packages for directories it uses. > Note: No known owner of /usr/share/selinux/packages/passt, > /usr/share/doc/passt > > => Needs %dir entries for these two Added. > [!]: Package must own all directories that it creates. > Note: Directories without known owners: > /usr/share/selinux/packages/passt, /usr/share/doc/passt, > /usr/share/selinux, /usr/share/selinux/packages > > => Needs a Requires: selinux-policy Added. > [!]: Fully versioned dependency in subpackages if applicable. > Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in passt- > selinux > > => Needs %{?_isa} bit adding Left as it was: the SELinux policy subpackage is actually a "noarch" package, and the guidelines say: "When a subpackage requires the base package, it must do so using a fully versioned arch-specific (for non-noarch packages) dependency" ...which makes sense I suppose: even if %{?_isa} is "noarch", or empty, so that should work in any case. I guess fedora-review doesn't catch this corner case. --- Fabio, (In reply to Fabio Valentini from comment #14) > > Just FYI, the rpkg macros are not available in Fedora's build system (only > in COPR) and they are no longer maintained. Thanks for mentioning this, I didn't realise they are no longer maintained. > If you want to automate Release / %changelog, then rpmautospec (%autorelease > / %autochangelog) is currently the only officially supported way to do that. I'm not actually using the {{{ git_dir_changelog }}} rpkg-utils macro, I wrote a different one for this purpose, because git_dir_changelog would report a line for every single upstream commit (with possibility of editing), which clearly conflicts with packaging guidelines. If one day Copr stops allowing macro usage altogether, I would rather call my macro (a rather simple shell function) from the upstream Makefile, I guess. As to the unavailability on Fedora's build system, yes, I already planned for that -- I would simply copy the spec file as processed by rpkg, and keep it updated in Fedora's git. --- Updated spec file and SRPM: Spec URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04805487-passt/passt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/sbrivio/passt/fedora-rawhide-x86_64/04805487-passt/passt-0^20220902.g7ce9fd1-1.fc38.src.rpm
Marking as approved. I have also sponsored your account for the packager group. You can now continue with the remaining steps to import and build the package, as detailed at: https://docs.fedoraproject.org/en-US/package-maintainers/Package_Review_Process/#_contributor Ping me if you need assistance with any parts of it.
(In reply to Stefano Brivio from comment #15) > I'm not actually using the {{{ git_dir_changelog }}} rpkg-utils macro, I > wrote a different one for this purpose, because git_dir_changelog would > report a line for every single upstream commit (with possibility of > editing), which clearly conflicts with packaging guidelines. If one day Copr > stops allowing macro usage altogether, I would rather call my macro (a > rather simple shell function) from the upstream Makefile, I guess. > > As to the unavailability on Fedora's build system, yes, I already planned > for that -- I would simply copy the spec file as processed by rpkg, and keep > it updated in Fedora's git. Note that the canonical location for the .spec file for a package is supposed to be the Fedora dist-git repository: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_maintenance_and_canonicity i.e. you are not allowed to override changes that are made to the Fedora spec file with things from upstream.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/passt
FEDORA-2022-252b744afb has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2022-252b744afb
FEDORA-2022-c3236d381e has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-c3236d381e
FEDORA-2022-8a734e1c9c has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2022-8a734e1c9c
BTW you should avoid using macros in the %changelog: rpmbuild expands them! See for example https://koji.fedoraproject.org/koji/buildinfo?buildID=2058896 eg you could write instead %%set_build_flags or set_build_flags
FEDORA-2022-8a734e1c9c has been pushed to the Fedora 37 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-8a734e1c9c \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-8a734e1c9c See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
(In reply to Jens Petersen from comment #22) > BTW you should avoid using macros in the %changelog: rpmbuild expands them! > > See for example https://koji.fedoraproject.org/koji/buildinfo?buildID=2058896 > > eg you could write instead %%set_build_flags or set_build_flags Right, I just noticed, thanks for explaining where that mess came from! I was about to prepare an update for a failed annocheck test ("FAIL: bind-now test because not linked with -Wl,-z,now"), I'll change that as well.
FEDORA-2022-252b744afb 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 --refresh --advisory=FEDORA-2022-252b744afb \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-252b744afb See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-c3236d381e 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 --refresh --advisory=FEDORA-2022-c3236d381e \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-c3236d381e See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-a9152a0bbe has been pushed to the Fedora 35 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-a9152a0bbe` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-a9152a0bbe See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-70d28f3e7e has been pushed to the Fedora 36 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-70d28f3e7e` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-70d28f3e7e See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-4d3eb959e7 has been pushed to the Fedora 37 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-4d3eb959e7` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-4d3eb959e7 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-4d3eb959e7 has been pushed to the Fedora 37 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-70d28f3e7e has been pushed to the Fedora 36 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-a9152a0bbe has been pushed to the Fedora 35 stable repository. If problem still persists, please make note of it in this bug report.
(In reply to Daniel Berrangé from comment #16) > Marking as approved. I have also sponsored your account for the packager > group. You can now continue with the remaining steps to import and build the > package, as detailed at: > > https://docs.fedoraproject.org/en-US/package-maintainers/ > Package_Review_Process/#_contributor > > Ping me if you need assistance with any parts of it. The package is now in stable repositories for Fedora 35, 36 and 37. Thanks a lot for all the support!