Bug 1667935

Summary: Review request nodejs-mqtt - MQTT client library for nodejs
Product: [Fedora] Fedora Reporter: Tomas Hrcka <thrcka>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: hiwkby, package-review, zebob.m
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2020-08-10 00:58:44 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Bug Depends On:    
Bug Blocks: 201449    

Description Tomas Hrcka 2019-01-21 13:33:47 UTC
Spec URL: https://fedorapeople.org/~humaton/rpms/new/nodejs-mqtt.spec
SRPM URL: https://fedorapeople.org/~humaton/rpms/new/nodejs-mqtt-2.18.8-1.fc29.src.rpm

Description: MQTT.js is a client library for the MQTT protocol, written in JavaScript for node.js and the browser.

Fedora Account System Username: humaton

Comment 1 Hirotaka Wakabayashi 2019-02-11 05:31:14 UTC
Hello, this is an unofficial review. Please read this for your reference.

Summary
=======

1. rpmlint results
2. The 'Group' tag should not be used
3. Other suggestions
4. Appendix 2: Koji scratch build succeeded

Details
=======

1. rpmlint results
------------------

A warning on the source rpm and 24 warnings on the binary rpm, which
I built on my fc29 environment::
  $ rpmlint /home/vagrant/rpmbuild/SRPMS/nodejs-mqtt-2.18.8-1.fc29.src.rpm
  nodejs-mqtt.src: W: no-%build-section
  1 packages and 0 specfiles checked; 0 errors, 1 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/noarch/nodejs-mqtt-2.18.8-1.fc29.noarch.rpm
  nodejs-mqtt.noarch: W: incoherent-version-in-changelog 2.18.8-2 ['2.18.8-1.fc29', '2.18.8-1']
  nodejs-mqtt.noarch: W: only-non-binary-in-usr-lib
  nodejs-mqtt.noarch: W: pem-certificate /usr/lib/node_modules/mqtt/examples/tls client/crt.ca.cg.pem
  nodejs-mqtt.noarch: W: pem-certificate /usr/lib/node_modules/mqtt/examples/tls client/tls-cert.pem
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/commist /usr/lib/node_modules/commist
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/concat-stream /usr/lib/node_modules/concat-stream
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/end-of-stream /usr/lib/node_modules/end-of-stream
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/es6-map /usr/lib/node_modules/es6-map
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/help-me /usr/lib/node_modules/help-me
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/inherits /usr/lib/node_modules/inherits@2
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/minimist /usr/lib/node_modules/minimist
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/mqtt-packet /usr/lib/node_modules/mqtt-packet
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/pump /usr/lib/node_modules/pump
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/readable-stream /usr/lib/node_modules/readable-stream
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/reinterval /usr/lib/node_modules/reinterval
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/split2 /usr/lib/node_modules/split2
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/websocket-stream /usr/lib/node_modules/websocket-stream
  nodejs-mqtt.noarch: W: dangling-symlink /usr/lib/node_modules/mqtt/node_modules/xtend /usr/lib/node_modules/xtend
  nodejs-mqtt.noarch: W: pem-certificate /usr/lib/node_modules/mqtt/test/helpers/public-cert.pem
  nodejs-mqtt.noarch: W: pem-certificate /usr/lib/node_modules/mqtt/test/helpers/tls-cert.pem
  nodejs-mqtt.noarch: W: pem-certificate /usr/lib/node_modules/mqtt/test/helpers/wrong-cert.pem
  nodejs-mqtt.noarch: W: no-manual-page-for-binary mqtt
  nodejs-mqtt.noarch: W: no-manual-page-for-binary mqtt_pub
  nodejs-mqtt.noarch: W: no-manual-page-for-binary mqtt_sub
  1 packages and 0 specfiles checked; 0 errors, 24 warnings.


My review on the result above is as followings.

1.1. nodejs-mqtt.src: W: no-%build-section
I think this warning is meaningless because this module is neither a native
module nor needs no build processes and has nothing to do in %build section.

1.2. nodejs-mqtt.noarch: W: incoherent-version-in-changelog 2.18.8-2 ['2.18.8-1.fc29', '2.18.8-1']
The entry of 2.18.8-1 should exist because it must be the initial version of
this package.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs

'Release: 1%{?dist}' should be 'Release: 2%{?dist}' because the latest version
in the %changelog section is 2.18.8-2.

1.3. nodejs-mqtt.noarch: W: only-non-binary-in-usr-lib
You should ignore this because the following guideline says node.js modules
written purely in JavaScript should be installed to the %nodejs_sitelib.
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_macros

1.4. nodejs-mqtt.noarch: W: pem-certificate /usr/lib/node_modules/mqtt/examples...
I think this warning can be ignored because this PEM certificate will be used
for example purpose.

1.5. nodejs-mqtt.noarch: W: dangling-symlink ...
I think dangling-symlink warnings should be ignored the %nodejs_symlink_deps
macro creates a node_modules tree with symlinks for each dependency listed in
package.json.

1.6. nodejs-mqtt.noarch: W: pem-certificate /usr/lib/node_modules/mqtt/test...
I think this can be ignored because this PEM certificate will be used for test
purpose.

1.7. nodejs-mqtt.noarch: W: no-manual-page-for-binary ...
This should be fixed. You might know that help2man is a useful tool.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

2. The 'Group' tag should not be used
-------------------------------------

The Group: tag should not be used. Here is the guideline.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections
   
3. Other suggestions
--------------------

You can use the automatic dependency generator in nodejs-packaging that adds
versioned dependencies based on the information provided in a module’s
package.json file.
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_automatic_requires_and_provides

You can patch package.json if some dependency module's versions in it are 
incorrect by using the %nodejs_fixdep macro in nodejs-packaging.
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_correcting_dependencies

The guideline says if a test suite should be executed if the package provides
it and it is practical to do so.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites


Appendix 1: Koji scratch build succeeded
----------------------------------------
https://koji.fedoraproject.org/koji/taskinfo?taskID=32688759

Thanks in advance,
Hirotaka Wakabayashi

Comment 2 Robert-André Mauchin 🐧 2019-02-15 23:44:02 UTC
 - You don't need to specify Requires for nodejs package https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_automatic_requires_and_provides

Comment 3 Package Review 2020-07-10 00:57:05 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

Comment 4 Package Review 2020-08-10 00:58:44 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.