Bug 587978 (whatweb) - Review Request: whatweb - Web scanner to identify what websites are running
Summary: Review Request: whatweb - Web scanner to identify what websites are running
Keywords:
Status: CLOSED NOTABUG
Alias: whatweb
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Otto Urpelainen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 632917 632919
Blocks: FE-DEADREVIEW FE-SECLAB
TreeView+ depends on / blocked
 
Reported: 2010-05-01 23:00 UTC by Michal Ambroz
Modified: 2021-06-16 19:56 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-06-16 00:45:22 UTC
Type: ---


Attachments (Terms of Use)
License Check (218.00 KB, text/plain)
2012-08-28 06:13 UTC, Randy Berry
randyn3lrx: review?
Details
md5sum (253 bytes, text/plain)
2012-08-28 06:16 UTC, Randy Berry
no flags Details

Description Michal Ambroz 2010-05-01 23:00:15 UTC
SPEC URL: http://rebus.fedorapeople.org/12/SPECS/whatweb.spec
SRPM URL: http://rebus.fedorapeople.org/12/SRPMS/whatweb-0.4.2-1.fc12.src.rpm

Description:
Identify content management systems (CMS), blogging platforms, stats/analytic
packages, JavaScript libraries, servers and more. When you visit a website in
your browser the transaction includes many unseen hints about how the web-server
is set up and what software is delivering the web-page. Some of these hints are
obvious, ex. “Powered by XYZ” and others are more subtle. WhatWeb recognizes
these hints and reports what it finds.

Hello,
Please could you review the package whatweb?

Output from rpmlint:
$ rpmlint whatweb-0.4.2-1.fc12.noarch.rpm whatweb-0.4.2-1.fc12.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

Koji F12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2154469
Koji F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2154471

Comment 1 Terje Røsten 2010-05-02 18:18:25 UTC
Add dep on ruby:

$ whatweb
/usr/bin/env: ruby: No such file or directory

Comment 2 Mamoru TASAKA 2010-05-02 18:32:49 UTC
You should package anemone seperately:
http://anemone.rubyforge.org/

Comment 3 Michal Ambroz 2010-05-02 21:54:59 UTC
(In reply to comment #1)
> Add dep on ruby:
Strange - I wanted to add dependency by following guideline for ruby. Apparently it was not enough adding the:
Requires:       ruby(abi) = 1.8

Thank you Terje for noticing that - I will add file dependency for ruby binary as well.
SPEC URL: http://rebus.fedorapeople.org/12/SPECS/whatweb.spec
SRPM URL: http://rebus.fedorapeople.org/12/SRPMS/whatweb-0.4.2-2.fc12.src.rpm


(In reply to comment #2)
> You should package anemone seperately:
> http://anemone.rubyforge.org/    
Original anemone project has got some extra dependencies.
Version present in whatweb is standalone patched version of anemone without some dependencies. 

Best regards.
Michal Ambroz

Comment 4 Mamoru TASAKA 2010-05-03 04:20:22 UTC
(In reply to comment #3)
> (In reply to comment #1)

> (In reply to comment #2)
> > You should package anemone seperately:
> > http://anemone.rubyforge.org/    
> Original anemone project has got some extra dependencies.
> Version present in whatweb is standalone patched version of anemone without
> some dependencies. 

So please explain why you want to avoid additional dependency for
this package. I don't think that so huge additonal dependency will
be added. Using copy of external project is generally forbidden
on Fedora and it should be packaged:

https://fedoraproject.org/wiki/Packaging/Guidelines#Duplication_of_system_libraries
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Comment 5 Mamoru TASAKA 2010-05-03 04:26:07 UTC
As written on the above URLs, one of the biggest reasons is that
when a new version of the package is released for (security) bug fixes
or so, it gets very hard for us to track if such bug fixes are also
applied to ones internally bundled in other packages.

Comment 6 Michal Ambroz 2010-05-03 09:02:11 UTC
Hello Mamoru,
thank you for your comments. 
I will consider packing anemone and patching the whatweb to use sytem library of anemone.

(In reply to comment #4)
> https://fedoraproject.org/wiki/Packaging/Guidelines#Duplication_of_system_libraries
I believe this link is not relevant as there is currenlty no anemone library in Fedora.

>https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries  
However according this there probably should be one.

Best regards
Michal Ambroz

Comment 7 Michal Ambroz 2010-08-23 22:24:53 UTC
SPEC URL: http://rebus.fedorapeople.org/13/SPECS/whatweb.spec
SRPM URL: http://rebus.fedorapeople.org/13/SRPMS/whatweb-0.4.5-1.fc13.src.rpm

Rebuild of new version 0.4.5 of whatweb.
I still didn't manage to patch to separate anemone library from whatweb.

Michal Ambroz

Comment 8 Mamoru TASAKA 2010-08-30 18:05:07 UTC
(In reply to comment #7)
> I still didn't manage to patch to separate anemone library from whatweb.
> 

First please submit rubygem-anemome review request.
Then I guess replacing
------------------------------------------------
require 'lib/anemone/anemone.rb'
------------------------------------------------
in whatweb script to
------------------------------------------------
require 'rubygems'
require 'anemone'
------------------------------------------------
should work (if anemone 0.4.0 still supports anemone 0.2.0 API:
anemone bundled in whatweb 0.4.5 is 0.2.0, while the latest anemone
is 0.4.0)

Comment 9 Mamoru TASAKA 2010-09-11 16:15:00 UTC
ping?

Comment 10 Michal Ambroz 2010-09-11 20:52:46 UTC
U have submitted for review package rubygems-robots, which is needed for rubygems-anemone.
Bug 632912.

Comment 11 Michal Ambroz 2010-09-11 21:52:29 UTC
U have submitted for review package rubygems-shoulda, which is needed for rubygems-robots, which is needed for rubygems-anemone.
Bug 632917.

Comment 12 Michal Ambroz 2010-09-11 22:24:22 UTC
I have submitted for review package rubygems-anemone, which is needed for whatweb.
Bug 632919

Comment 13 Michal Ambroz 2010-09-11 22:53:08 UTC
SPEC URL: http://rebus.fedorapeople.org/SPECS/whatweb.spec
SRPM URL: http://rebus.fedorapeople.org/SRPMS/whatweb-0.4.5-2.fc13.src.rpm

Patch to use system-wide rubygems anemone library rather than local copy of anemone library.

Koji F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2462168

$ rpmlint whatweb-0.4.5-2.fc13.src.rpm whatweb-0.4.5-2.fc13.noarch.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

Best regards
Michal Ambroz

Comment 14 Michal Ambroz 2010-09-23 00:03:06 UTC
Whatweb doesn't work with the upstream anemone version.
Issue reported to whatweb developer, waiting for fix.

Comment 15 Bug Zapper 2010-11-03 15:52:32 UTC
This message is a reminder that Fedora 12 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 12.  It is Fedora's policy to close all
bug reports from releases that are no longer maintained.  At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '12'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 12's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that 
we may not be able to fix it before Fedora 12 is end of life.  If you 
would still like to see this bug fixed and are able to reproduce it 
against a later version of Fedora please change the 'version' of this 
bug to the applicable version.  If you are unable to change the version, 
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events.  Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

The process we are following is described here: 
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 16 Michal Ambroz 2011-03-27 23:16:46 UTC
New version of whatweb released.
This version still requires embedded old patched version of anemone.

http://rebus.fedorapeople.org/RPMS/whatweb-0.4.6-1.fc14.noarch.rpm
http://rebus.fedorapeople.org/SPECS/whatweb.spec
http://rebus.fedorapeople.org/SRPMS/whatweb-0.4.6-1.fc14.src.rpm

Michal Ambroz

Comment 17 Michal Ambroz 2011-04-05 22:27:47 UTC
New version of whatweb released.
This version requires embedded old patched version of anemone.

http://rebus.fedorapeople.org/SPECS/whatweb.spec
http://rebus.fedorapeople.org/RPMS/whatweb-0.4.7-1.fc14.noarch.rpm
http://rebus.fedorapeople.org/SRPMS/whatweb-0.4.7-1.fc14.src.rpm

Comment 18 Michal Ambroz 2012-04-17 01:10:54 UTC
I am sorry - It is not in my powers to separate the anemone library from the package. The package works for me as is and separation is not supported by the upstream.

I am stepping down from pushing this package to Fedora.
Best regards 

Michal Ambroz

Comment 19 Michal Ambroz 2012-07-08 16:12:41 UTC
Reopening the package review request.
It seems that the whatweb is in next upcoming version dropping the support for the anemone library as whole.

http://rebus.fedorapeople.org/SPECS/whatweb.spec
http://rebus.fedorapeople.org/RPMS/whatweb-0.4.8-0.git20120708.1.fc17.noarch.rpm 
http://rebus.fedorapeople.org/SRPMS/whatweb-0.4.8-0.git20120708.1.fc17.src.rpm

Michal Ambroz

Comment 20 Randy Berry 2012-08-28 06:12:51 UTC
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[ ]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[ ]: MUST Package contains no bundled libraries.
[ ]: MUST Changelog in prescribed format.
[ ]: MUST Sources contain only permissible code or content.
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
[ ]: MUST Macros in Summary, %description expandable at SRPM build time.
[ ]: MUST Package contains desktop file if it is a GUI application.
[ ]: MUST Development files must be in a -devel package
[ ]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[ ]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[ ]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install. 
     Note: rm -rf is only needed if supporting EPEL5. This is acceptable if you plan to package for EPEL5 
[ ]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL" For detailed output of licensecheck (see attachment)
[x]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[ ]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[ ]: MUST Package obeys FHS, except libexecdir and /usr/target.
[ ]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: MUST Package must own all directories that it creates.
[ ]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[ ]: MUST Package is not relocatable.
[ ]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
     Note: See below.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[ ]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[!]: SHOULD Buildroot is not present
     Note: Buildroot is not needed unless packager plans to package for EPEL5
[!]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL5
[ ]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[ ]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[ ]: SHOULD Package functions as described.
[ ]: SHOULD Latest version is packaged.
[ ]: SHOULD Package does not include license text files separate from
     upstream.
[ ]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: SHOULD SourceX tarball generation or download is documented.
[!]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: Source0 (urbanadventurer-WhatWeb-1dc2cff.tar.gz)
[x]: SHOULD SourceX is a working URL.
[ ]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[ ]: SHOULD %check is present and all tests pass.
[ ]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[!]: SHOULD Spec use %global instead of %define.
     Note: %define gituser urbanadventurer %define gitname WhatWeb %define
     gitversion 1dc2cff

Issues:
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
See: None
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
     Note: Upstream MD5sum check error.
See: http://fedoraproject.org/wiki/Packaging/SourceURL

Rpmlint
-------
Checking: whatweb-0.4.8-0.git20120708.1.fc16.src.rpm
          whatweb-0.4.8-0.git20120708.1.fc16.noarch.rpm
whatweb.src:15: W: macro-in-comment %{name}
whatweb.src:15: W: macro-in-comment %{version}
whatweb.src: W: file-size-mismatch urbanadventurer-WhatWeb-1dc2cff.tar.gz = 1238667, https://github.com/urbanadventurer/WhatWeb/tarball/master/urbanadventurer-WhatWeb-1dc2cff.tar.gz = 1254743
2 packages and 0 specfiles checked; 0 errors, 3 warnings.


Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:
Requires
--------
whatweb-0.4.8-0.git20120708.1.fc16.noarch.rpm (rpmlib, GLIBC filtered):
    
    /bin/bash  
    /bin/sh  
    /usr/bin/env  
    /usr/bin/ruby  
    ruby(abi) >= 1.8

Provides
--------
whatweb-0.4.8-0.git20120708.1.fc16.noarch.rpm:
    
    whatweb = 0.4.8-0.git20120708.1.fc16

MD5-sum check
-------------
https://github.com/urbanadventurer/WhatWeb/tarball/master/urbanadventurer-WhatWeb-1dc2cff.tar.gz :
  CHECKSUM(SHA256) this package     : fcedeb9373d104f9c09a23e66e2f2bdf892829d2a7b354112b190e6caabb0c4f
  CHECKSUM(SHA256) upstream package : 9efffec153c594c3a767dc81f0d97babdc62172d82d6bf69f4096d7744c19010
diff -r also reports differences


Generated by fedora-review 0.2.2 (9f8c0e5) last change: 2012-08-09
Command line :/usr/bin/fedora-review -b=587978
External plugins:

Comment 21 Randy Berry 2012-08-28 06:13:08 UTC
Created attachment 607410 [details]
License Check

Comment 22 Randy Berry 2012-08-28 06:16:16 UTC
Created attachment 607411 [details]
md5sum

Comment 24 Pavel Alexeev 2015-12-03 19:37:31 UTC
Ping?

Comment 25 Michal Ambroz 2016-06-21 01:24:09 UTC
SPEC URL: https://rebus.fedorapeople.org/SPECS/whatweb.spec
SRPM URL: https://rebus.fedorapeople.org/SRPMS/whatweb-0.4.8-0.git20160611.1.fc23.src.rpm

Bump to current git snapshot.

Comment 26 Michal Ambroz 2017-12-10 05:41:48 UTC
SPEC URL: https://rebus.fedorapeople.org/SPECS/whatweb.spec
SRPM URL: https://rebus.fedorapeople.org/SRPMS/whatweb-0.4.9-1.fc27.src.rpm

Bump to current upstream release.

>[!]: MUST Each %files section contains %defattr if rpm < 4.4
defattr removed

>[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
cleaning of the buildroot removed

>[!]: MUST Sources used to build the package match the upstream source, as
The release should MD5 match much better than git commit ad-hoc snapshot (please note that gzip contains timestamps so it might not be MD5 same).

>[!]: SHOULD Buildroot is not present
removed

>[!]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or
removed

>[!]: SHOULD SourceX / PatchY prefixed with %{name}.
link to release source is now resulting in whatweb-0.4.9.tar.gz

>[!]: SHOULD Spec use %global instead of %define.
define changed to global


Best regards
Michal Ambroz

Comment 27 Package Review 2020-07-10 00:45:36 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Comment 28 Package Review 2020-11-13 00:45:25 UTC
This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Comment 29 Otto Urpelainen 2021-04-21 06:36:11 UTC
I am sorry for the rough review experience of this package, even after the difficult issue of bundled dependencies got resolved. If you are still interested in getting WhatWeb packaged for Fedora, I can complete the review. If so, please update the spec and srpm to included the latest version so I can proceed to review. If you are not interested anymore, please either close this issue, or do nothing, in which case automation should close this issue in one month.

Comment 30 Michal Ambroz 2021-04-21 19:28:22 UTC
Hello Otto,
no need to be sorry ... we are all just volunteers. And thank you if you would pick-up this review. Here is the current version :

SPEC URL: http://rebus.fedorapeople.org/SPECS/whatweb.spec
SRPM URL: http://rebus.fedorapeople.org/SRPMS/whatweb-0.5.5-1.fc33.src.rpm

Here the scratch build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=66434481


$ rpmlint whatweb-0.5.5-1.fc33.src.rpm whatweb-0.5.5-1.fc33.noarch.rpm whatweb.spec 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 31 Otto Urpelainen 2021-04-22 06:36:37 UTC
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.

Comment 32 Michal Ambroz 2021-05-15 20:54:00 UTC
(In reply to Otto Urpelainen from comment #31)
SPEC URL: http://rebus.fedorapeople.org/SPECS/whatweb.spec
SRPM URL: http://rebus.fedorapeople.org/SRPMS/whatweb-0.5.5-2.fc34.src.rpm


> 1. What is the intent of fragment part #/%{name}-%{version}.tar.gz?
> > Source0:        https://github.com/%{gituser}/%{gitname}/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
when you download with the spectool -g whatweb.spec, this is what renames boring v5.5.0.tar.gz to sexy whatweb-5.5.0.tar.gz


 
> 2. Better %{_bindir}/ruby, since that is how rubypick package provides this
> > Requires: /usr/bin/ruby
Ok ... thanks


> 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
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Ruby/#_shebang_lines

I consider env to be potential security problem and preffer to be explicit about the interpreter used in the packaged stuff.
The guide is saying as well that it SHOULD use #!/usr/bin/ruby .
The "env ruby" is not always only /usr/bin/ruby. Depending on environment settings it could be also /usr/local/bin/ruby or ~user/bin/ruby or even /tmp/you_have_been_hacked/ruby .


 
> 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
Yes ... I guess that on Ubuntu they use different groff for formatting the man pages so it is ok for them.
On Fedora it complains so I have to remove that tags.


> 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
Yes still needed. I do not consider this ustream bug, but it relies to  Fedora packaging.



> 6. Are both this and the earlier sed call that commnents off 'bundle install' needed?
Nah ... Just the sed was working. The alias was not working I just forgot it there - thanks, removing the alias.


> 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?
At the time I was packaging I was probably trying to comply with the rule that all directories must be owned.
So I was trying to explicitly list them.
These days yes %{_datadir}/%{name} would do.


> 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
During build there is no networking.
The site used for tests - https://whatweb.net/ - is down.
Adding conditional and comment to build with tests.


 
> 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.
- my-plugins is meant for locally created plugins to separate them from the dozens of others. Its installed by upstream and searched for local libs - I do not want to change this.
- moved plugin-development to documentation
 
> 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.
- yes executables, but more like examples. Not core functionality.
- moved to documentation



> 11. Maybe a comment here explaining what is going on.
> Is it just that RHEL does not support Recommends?
Yes. RHEL7 doesn't know Recommends.
Commented.

 
> 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.
yes gplv2+ ... changed in spec.
doing that I have found in plugins ip2country.csv database with non-free (donationware) license from unreachable origin, changed spec to delete that one.

Comment 33 Mamoru TASAKA 2021-05-16 12:24:28 UTC
(In reply to Otto Urpelainen from comment #31)
 
> 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
> 

This does not say using env on shabang is permitted. The above guideline says when the package really needs jruby,
you can write "#!/usr/bin/jruby" on shebang (but I don't think jruby on Fedora is well-maintained).
All other should use "#!/usr/bin/ruby" on shebang. Anyway env should not be used on shebang.

Comment 34 Otto Urpelainen 2021-05-16 21:03:15 UTC
Commenting only on items where I have something to all, for all others: Thank you for the explanation.

(In reply to Michal Ambroz from comment #32)
> (In reply to Otto Urpelainen from comment #31)
> 
> > 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
> > https://docs.fedoraproject.org/en-US/packaging-guidelines/Ruby/#_shebang_lines
> 
> I consider env to be potential security problem and preffer to be explicit
> about the interpreter used in the packaged stuff.
> The guide is saying as well that it SHOULD use #!/usr/bin/ruby .
> The "env ruby" is not always only /usr/bin/ruby. Depending on environment
> settings it could be also /usr/local/bin/ruby or ~user/bin/ruby or even
> /tmp/you_have_been_hacked/ruby .

Ah, I apologize, I worded by comment badly and linked to wrong, only marginally relevant section of the guidelines. What I was trying to say was this: Nowadays, shebang lines are automatically modified to replace env with the correct interpreter, so there should be no need to do that manually. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shebang_lines

Doing that manually is not forbidden either, so it can stay that way if you prefer. But doing that explicitly does not change the result from what the defaults already do.

> > 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
> Yes ... I guess that on Ubuntu they use different groff for formatting the
> man pages so it is ok for them.
> On Fedora it complains so I have to remove that tags.

I see, it may be for the '.ni' case. The './plugins-disabled' is for sure part of the example for enabling and disabling plugin directories. I took a closer look at the upstream whatweb.1, and my interpretation is that the file simply is in a bad shape and would benefit from a PR like this. I encourage submitting fixes. But if you do not want to do that, that's ok too, I won't reject this review for that.

Regarding the substitutions:
1. Instead of 's|^.ni||', use 's|^\.ni||', to replace the unknown request '.ni' and not lines starting with 'Ani', 'Bni' etc.
2. Instead of 's|^\./plugins-disabled|+\./plugins-disabled|', use 's|^\./plugins-disabled|\\\./plugins-disabled|'. '+' is not a Groff way to escape '.', but happens to be Whatweb syntax and thus changes the example.

Comment 35 Package Review 2021-06-16 00:45:22 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.

Comment 36 Otto Urpelainen 2021-06-16 19:56:56 UTC
Oh, this is a shame, this was so close. My two remaining items were both non-blocking, so just a reply "I do not want to make any more changes" would have sufficed to get this accepted. Please reopen if you still wish to complete this.


Note You need to log in before you can comment on or make changes to this bug.