https://bugzilla.redhat.com/show_bug.cgi?id=2417806
--- Comment #26 from Andreas Haupt <[email protected]> --- Thanks for all the comments! new build: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/fedora-rawhide-x86_64/10207168-lesspipe/lesspipe-2.22-8.fc45.src.rpm (In reply to Cristian Le from comment #24) > Hi Andreas, a quick note about Fedora review process, please continue to use > the format with `spec` and `srpm` as you did in the first few comments. That > triggers the Fedora review bot to submit its own copr builds including the > review.txt. Ah! Thanks for the hint! Now it hopefully works again. > Will leave the proper review for Pter since he seems to have better grasp of > the package. Just noting some generic package review points: > - Please add some comments on why some of the non-obvious dependencies > including weak ones are being added. bat, pandoc, and libreoffice are very > weird to be on that list Those were mentioned by the author Wolfgang Friebel as "nice to have" for most users. I can change that if desired. > - Similarly would be good for the BuildRequires with maybe some hints on > where they were found added a comment that these BuildRequires are needed for test.sh > - Heredoc files in the spec file can be quite bug prone. I would recommend a > separate source file and using a `sed` to expand the macros instead Put into %{SOURCE{0,1}} > - `%dir %{bindir}` is unnecessary, that would always be owned by something > at the very core of the system. Generally if a hard dependency owns the > directories already, co-owning the folder does not do anything for you %{bindir} is /usr/libexec/lesspipe atm. This is the reason I put the directory into the rpm. I want this directory to be removed in case the rpm is uninstalled. > - And the inverse issue with `%{bash_completion}` should be more explicitly > a `%dir` (to avoid any surprises that a file became a directory and > vice-versa) or packaging the files in it more explicitly Done. %dir %{bash_completion} as well as %{bash_completion}/* are now used. > - Using the git archives is generally preferred. You know the libxz fiasco > and all that right? I'm still in contact with the author Wolfgang Friebel to sort out issues. The current lesspipe-2.22 used is actually a "git archive" snapshot taken two days ago. In case all is fine with this review, I would ask the author to create a new release (2.23 most likely). But what you are saying is: I should change the %Source: from https://lesspipe.org/archive/lesspipe-%{version}.tar.gz to https://github.com/wofr06/lesspipe/archive/refs/tags/v%{version}.tar.gz right? > - For the GPL-2.0-only license, have you confirmed with upstream that that > is their intent? It is taken from: https://github.com/wofr06/lesspipe/blob/lesspipe/LICENSE > Usually it is not clear from the license text alone if that > is the case > - Licensecheck for this would be fun. See this gem [1] in the vimcolor script > > [1]: > https://github.com/wofr06/lesspipe/blob/ > f64d679cae63e86563802363a671cea14493d2c0/vimcolor#L12-L13 OK, will get back to the author to make this clear, thanks! -- 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=2417806 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202417806%23c26 -- _______________________________________________ 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
