https://bugzilla.redhat.com/show_bug.cgi?id=2106611 --- Comment #8 from Stefano Brivio <sbrivio@xxxxxxxxxx> --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2106611 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
