https://bugzilla.redhat.com/show_bug.cgi?id=2075850 --- Comment #9 from Marco Rizzi <marcor@xxxxxx> --- Sorry Davide for the long delay, I made some changes, let's give fedora-review another run to see what's still pending. Spec URL: https://download.copr.fedorainfracloud.org/results/marcor/zeek/fedora-rawhide-x86_64/04366466-zeek/zeek.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/marcor/zeek/fedora-rawhide-x86_64/04366466-zeek/zeek-4.2.0-1.fc37.src.rpm Here are my comments: > - No %config files under /usr. > Note: %config /usr/share/zeek/site/local.zeek > [!]: %config files are marked noreplace or the reason is justified. > Note: No (noreplace) in %config /usr/share/zeek/site/local.zeek >Is this actually a config file, meaning something the user is supposed/expected to edit? If it is, we should figure out if we can move it under /etc (and mark it as %config(noreplace)); if it isn't, it shouldn't be marked as %confg Looking at the zeek documentation, this is classified as a script file, so I have removed the %config reference: https://docs.zeek.org/en/master/quickstart.html#filesystem-walkthrough >- Large documentation must go in a -doc subpackage. > >I think this is a false positive? The only actual documentation I see is three text files, that doesn't warrant a dedicated package IMO. I have removed the "CHANGES" text file from the %doc statement, that file alone is more than 1MB. >[!]: License field in the package spec file matches the actual license. >[!]: Package contains no bundled libraries without FPC exception. >These two are related; basically this thing is bundling a bunch of external libraries like rapidjson, which is generally frowned upon. Long term, we should figure out if we can unbundle these, but for now please add bundled() provides for them per https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling and updated the License: field accordingly. I have added "bundled()" for the short term >[!]: Package requires other packages for directories it uses. > Note: No known owner of /usr/lib64/zeek, /var/log/zeek >[!]: Package must own all directories that it creates. > Note: Directories without known owners: /usr/lib/sysusers.d, > /var/log/zeek, /usr/lib64/zeek > >Add %dir entries for /var/log/zeek and /usr/lib64/zeek and add a Requires: systemd for /usr/lib/sysusers.d done that >[!]: Patches link to upstream bugs/comments/lists or are otherwise > justified. > >Add a comment before the patch with the URL it comes from done >[-]: %check is present and all tests pass. > >Not a blocker, but if this package has tests it can run at build time, it'd be good to add a %check section to the spec (for cmake-based projects, usually it's enough to call %ctest if they're rigged up properly). I don't think I see tests that can run at build time. >[!]: Large data in /usr/share should live in a noarch subpackage if package > is arched. > >Consider making a zeek-data noarch subpackage for the stuff under /usr/share/zeek I'm not sure if that's going to be possible, different packages write to /usr/share/zeek/ -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2075850 _______________________________________________ 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 on the list, report it: https://pagure.io/fedora-infrastructure
