https://bugzilla.redhat.com/show_bug.cgi?id=587978 Otto Urpelainen <oturpe@xxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(rebus@xxxxxxxxx) --- Comment #31 from Otto Urpelainen <oturpe@xxxxxx> --- Reviewed, here are my findings. Some are clear and some just things I could not immediately understand. Fix the parts that you agree with and explain what is going on with others. I checked everything already, so after these are all addressed, the review should be clear. 1. What is the intent of fragment part #/%{name}-%{version}.tar.gz? It seems to me that it is simply ignored. If the idea is to indicate the name of downloaded file, a comment would do better? Also, the macros in the fragment do not resolve to the real filename from Content-Disposition http header at the moment, since %{name} != %{gitname} > Source0: https://github.com/%{gituser}/%{gitname}/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz 2. Better %{_bindir}/ruby, since that is how rubypick package provides this > Requires: /usr/bin/ruby References: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_effect_of_the_usrmove_fedora_feature https://src.fedoraproject.org/rpms/rubypick/blob/rawhide/f/rubypick.spec 3. Fixing env shebangs should not be required in Fedora anymore. If this is still needed for some reason (RHEL perhaps?), comment should be updated to match > # Fedora using Rubypick > sed -i -e 's|#!/usr/bin/env ruby|#!/usr/bin/ruby|; s|#!/bin/env ruby|#!/usr/bin/ruby|;' \ whatweb plugin-development/find-common-stuff plugin-development/get-pattern Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/Ruby/#_shebang_lines 4. I do not understand this. Is this an issue with upstream man pages? If so, a fix or an issue should be submitted and referenced from the specfile. > # Unknown macros in manpage > sed -i -e 's|^.ni||; s|^\./plugins-disabled|+\./plugins-disabled|' whatweb.1 5. Is this still needed? PR282 has been merged before 0.5.5 was released, so it should be ok. Again, if this is an upstream issue, a bug report or pull request should be referenced from here. If Fedora-specific, the situation should be explained. > # Add the whatweb shared directory + PR282 > sed -i -e "s|expand_path(__dir__)), '.')|expand_path(__dir__)), '%{_datadir}/%{name}')|" whatweb 6. Are both this and the earlier sed call that commnents off 'bundle install' needed? Can bundler be disable with only one method? If this is really needed here, a comment explaining what is going on would be good. > alias bundle='echo' 7. This is not wrong, but could be handled with a single row %{_datadir}/%{name}/addons, which would own the directory and include it and all its content in one statement. The same goes for lib, plugins, my-plugins, plugin-development and plugin-disabled folders. Also, I wonder if a simple '%{_datadir}/%{name}' would correctly handle all this, is there something in there that you do not want to own & include? > %dir %{_datadir}/%{name}/addons > ... > %{_datadir}/%{name}/addons/* 8. There are tests in the source, but no %check in specfile. Tests should be run. If is it too difficult to get them run inside the buildsystem, then perhaps a %check section with commented off attempt and a comment explaining why they cannot be run? Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites 9. my-plugins and plugin-development folders look like material for plugin writing. Are they really needed at runtime? If not, they should not installed. 10. What about the shell scripts in addons folder? Are the intended to be run by the user? If so, they should be installed to %{_bindir}. If not, and are not otherwise needed at runtime, they should be dropped or perhaps moved to documentation. 11. Maybe a comment here explaining what is going on. Is it just that RHEL does not support Recommends? > %if 0%{?rhel} && 0%{?rhel} <= 8 > Requires: rubygem-bson > Requires: rubygem-mongo > %else > Recommends: rubygem-bson > Recommends: rubygem-mongo > %endif 12. Is the license really GPLv2 or is it GPLv2+? License is listed as such in upstream home page, but many (not all!) files like lib/logging.rb contain a notice that also allows any later version. You should probably contact upstream to clarify the issue. -- You are receiving this mail because: You are always notified about changes to this product and component _______________________________________________ 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
