https://bugzilla.redhat.com/show_bug.cgi?id=2385991
--- Comment #18 from Dridi Boukelmoune <[email protected]> --- Thanks a lot for this initial review, I'll reply to the items with notes now, and I will later work on a new spec/SRPM submission, using this reply as a task summary for my future self. > [!]: License file installed when any subpackage combination is installed. > NOTE: Not entirely. See the above note. Good catch. I must admit that I followed upstream's licensing model with MPL2 for the standard libraries and GPL3 for programs, looking at SPDX comments for those only, not checking every single source file. I'll look at the details and bring this topic upstream if I find inconsistencies. In particular, I'm surprised to see more than one MPL2 variant. Regarding the RPM macros and my choice of MIT, I actually went with Fedora's licensing for spec files and packaging-related deliverables. If there are also MIT licensed sources in Hare, I will of course take that into account in license tags, but I'll have my closer look offline. > [x]: Requires correct, justified where necessary. > NOTE: Where does the tzdata requirement come from? Can you add a comment > above it? > Does it need to be repeated in both hare and hare-stdlib? This one is tricky, and I have not tried to automate it yet, but I probably should. It should be required at least by hare-stdlib, because the time::date Hare module needs it. The hare package can get it indirectly from hare-stdlib and frankly I don't remember why I put it in both, maybe I forgot to remove it from the hare package. Now, any program using the time::date module should include this as a dependency. This is something I'd like to automate, but it's unrealistic to generalize to other modules loading specific system-wide resources in the future... I'll probably add a comment for starters, and something to the packaging guidelines draft. > [!]: Rpmlint is run on all rpms the build produces. > Note: There are rpmlint messages (see attachment). > NOTE: "name-repeated-in-summary Hare" should be fixed (i.e., remove > "hare" from the package Summary). This is what my local hare.rpmlintrc file looks like: # Empty READMEs allow empty modules to have submodules addFilter("zero-length /usr/src/hare/stdlib/.*/README") # Empty "tagged" source files can override "default" files addFilter("zero-length /usr/src/hare/stdlib/.*[.].*") # Multiple tags may need the same sources addFilter("files-duplicate /usr/src/hare/stdlib/.*") # False positive for hare-rpm-macros addFilter("only-non-binary-in-usr-lib") # This warning should be case-sensitive addFilter("name-repeated-in-summary Hare") I didn't see a point in "name-repeated-in-summary Hare" especially when it's the capitalized form. Regarding empty READMEs, I think that meanwhile this has become a non-issue as all modules got at least a one-liner description at the beginning of the README. > [!]: Latest version is packaged. > NOTE: Please update to 0.26.0.1 TIL! > [!]: Patches link to upstream bugs/comments/lists or are otherwise > justified. > NOTE: A short comment above the Patch invocation would be suitable. This patch comes from the master branch, so I didn't feel the need for a comment. In fact, it's the very first patch that landed after the 0.26.0 release, but it did not make it in 0.26.0.1. > [!]: Packages should try to preserve timestamps of original installed > files. > NOTE: It'd be preferable to sed or patch the Makefile so -p is passed to > install to preserve timestamps. I think I will submit a patch upstream and add it to the spec. This time there will be a comment with a link to the mailing list thread, as there was with previous iterations of this package submission ;-) I already upstreamed changes to the build driver to support automatic dependency generators. > [!]: Spec file according to URL is the same as in SRPM. > Note: Bad spec filename: > /home/gotmax/dev/packaging/reviews/hare-0.26.0/built/hare/srpm- > unpacked/hare.spec > See: (this test has no URL) This looks like a side effect of patching fedora-review to feed it the SRPM's spec instead of a separate one. I'm not sure my own patch had this problem, I don't remember. > - I see there's a `# XXX: HARETEST_INCLUDE=slow` comment. What's the story > there? I didn't want to waste time waiting for tests feeding GBs of zeros to hash functions while I was iterating, so I kept a reminder. That would be something like '%make_build check HARETEST_INCLUDE=slow', or maybe via the environment. > - Definitely not a blocker, but I prefer the var:method(...) syntax in lua > over the type.method(var, ...) syntax (e.g., macros.hare_arches:gmatch(...) > instead of string.gmatch(macros.hare_arches, ...)) Thanks for the tip. I literally have no preference, this is my very first lua scripting in a spec file. I'm a total noob in this area. > https://git.sr.ht/~sircmpwn/hare/archive/0.26.0.tar.gz#/hare-0.26.0.tar.gz : > CHECKSUM(SHA256) this package : > 72a0f05e172523df333b2cce4c3bf8181a05d71a62531b89565e9913b2430cae > CHECKSUM(SHA256) upstream package : > 525ee699bdbade390eb1e35525ab9b5b3032c5f42cddce49d9176bbdc9172d04 > However, diff -r shows no differences I didn't repack the source archive, so something must have changed on sourcehut (like gzip compression level in zlib or other). This will become a moot point since my next submission will target the 0.26.0.1 release. > hare.spec: W: patch-not-applied Patch0: > 0001-hare-parse-fix-crash-on-invalid-input.patch > (snip) > Provides > -------- > (snip) > hare-stdlib: > hare-stdlib This is very wrong, hare-stdlib should provide hare_mod(...) for each module, and the patch is here to prevent a crash happening during dependency generation. I probably have local changes I made since the last submission. -- 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=2385991 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202385991%23c18 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://forge.fedoraproject.org/infra/tickets/issues/new
