Bug 1086245

One thing to be aware of here is that the NPM module is actually called "JSONStream" and there is a different NPM module called "jsonstream".

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 19 files have unknown license. Detailed output of
     licensecheck in /home/tom/1086245-nodejs-jsonstream/licensecheck.txt
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[!]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 3 files.
[!]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[-]: 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]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[!]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: nodejs-jsonstream-0.8.0-1.fc22.noarch.rpm
          nodejs-jsonstream-0.8.0-1.fc22.src.rpm
nodejs-jsonstream.noarch: W: spelling-error Summary(en_US) stringify -> stringy
nodejs-jsonstream.noarch: W: spelling-error Summary(en_US) js -> dis, ks, j
nodejs-jsonstream.noarch: W: summary-not-capitalized C streaming JSON.parse and stringify for Node.js
nodejs-jsonstream.noarch: W: spelling-error %description -l en_US stringify -> stringy
nodejs-jsonstream.noarch: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs-jsonstream.noarch: W: invalid-license MIT,APACHE2
nodejs-jsonstream.noarch: W: only-non-binary-in-usr-lib
nodejs-jsonstream.src: W: spelling-error Summary(en_US) stringify -> stringy
nodejs-jsonstream.src: W: spelling-error Summary(en_US) js -> dis, ks, j
nodejs-jsonstream.src: W: summary-not-capitalized C streaming JSON.parse and stringify for Node.js
nodejs-jsonstream.src: W: spelling-error %description -l en_US stringify -> stringy
nodejs-jsonstream.src: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs-jsonstream.src: W: invalid-license MIT,APACHE2
2 packages and 0 specfiles checked; 0 errors, 13 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint nodejs-jsonstream
nodejs-jsonstream.noarch: W: spelling-error Summary(en_US) stringify -> stringy
nodejs-jsonstream.noarch: W: spelling-error Summary(en_US) js -> dis, ks, j
nodejs-jsonstream.noarch: W: summary-not-capitalized C streaming JSON.parse and stringify for Node.js
nodejs-jsonstream.noarch: W: spelling-error %description -l en_US stringify -> stringy
nodejs-jsonstream.noarch: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs-jsonstream.noarch: W: invalid-license MIT,APACHE2
nodejs-jsonstream.noarch: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 7 warnings.
# echo 'rpmlint-done:'



Requires
--------
nodejs-jsonstream (rpmlib, GLIBC filtered):
    /usr/bin/env
    nodejs(engine)
    npm(jsonparse)
    npm(through)



Provides
--------
nodejs-jsonstream:
    nodejs-jsonstream
    npm(JSONStream)



Source checksums
----------------
http://registry.npmjs.org/JSONStream/-/JSONStream-0.8.0.tgz :
  CHECKSUM(SHA256) this package     : 7c731a42c477089fe1fa76824dc78b81a9e61f4ead32c8e4b9033cb69cac42d8
  CHECKSUM(SHA256) upstream package : 7c731a42c477089fe1fa76824dc78b81a9e61f4ead32c8e4b9033cb69cac42d8


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review -m compton-rawhide-x86_64 -b 1086245
Buildroot used: compton-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

A few issues for you to look at:

* Upstream has now released 0.9.0

* The license tag should be "MIT or ASL 2.0"

* Summary is missing an initial capital letter

* No need to install test directory, and example should be in %doc not installed

* Missing a %check to run the tests, though I see we are missing some of the modules so that probably isn't possible to enable yet - they do all appear to pass with it-is and event-stream installed locally

For the record:

On Nov 16, 2014 3:57 AM, "Tom Hughes" <tom> wrote:
>
> I'm working on a review (BZ#1086245) for nodejs-jsonstream.
>
> Now the interesting thing is that the module it is packaging is actually called JSONStream in the npm registry and there is in fact a separate jsonstream module.
>
> The general naming rules say that "You should generally use lowercase" but I guess this is a case where the node naming rules should take precedence, especially given that there is a conflict and we may need have both at some point?

Unfortunately the guidelines also forbid having two packages with names that differ only by case:
http://fedoraproject.org/wiki/Packaging:Conflicts#Conflicting_Package_Names

Also, these days npm doesn't allow uppercase letters in package names, so stuff like this can't happen anymore:
https://github.com/npm/npm/issues/3914

The uppercased version is older and more recently updated:

JSONStream:
"modified":"2014-08-04T19:15:52.140Z",
"created":"2011-09-23T11:01:36.806Z"

jsonstream: 
"modified":"2013-06-11T13:36:13.038Z",
"created":"2013-06-11T13:36:11.463Z"

Therefore, I believe JSONStream the capitalized deserves the name. I'm about to send a mail to both upstreams and the npm registry admins to this effect (with you and Anish on CC). Let's wait and see what they say.

If the capitalized version keeps the name, I would strongly recommend still calling the RPM package "nodejs-JSONStream" with the capitals so users don't mistake it for the lowercased version. I think this is a sufficient enough "compelling reason to follow a different upstream convention" as required by the guidelines.

-T.C.

The lowercased "jsonstream" package was voluntarily unpublished from the npm registry this morning by its upstream maintainer in response to my request. This should resolve our concerns about conflicts.

So this should be packaged as nodejs-jsonstream then? But with the code in /usr/lib/node_modules/JSONStream so that require works correctly?

As I said before, personally I would capitalize the RPM package name too so that users who are aware that there were once two jsonstreams know for sure which one it is. The guidelines say "You should generally use lowercase and turn underscores into dashes unless there's a compelling reason to follow a different upstream convention."  I think the fact that there once were two packages with the same name differently cased is a compelling enough reason to respect the upstream casing in this particular instance. But with the conflict resolved upstream I don't feel too strongly about it anymore so ultimately it's up to you two. :-)

But yes, regardless of what the rpm is named the capitalization needs to stay in the filesystem, since as you point out require() is case-sensitive when used on case-sensitive filesystems.

I think I agree with you, so anish if you add to the list of things that needs fixing to rename the spec and package name and the directory.

Is there any progress here?

(In reply to Parag AN(पराग) from comment #9)
> Is there any progress here?

I think we're waiting on anish to update his spec to use the lower case name for the package, but the mixed case name for the directory and also to look at the other things I mentioned after my initial review.

Thank you for your comments, please find new review attached,


Spec URL: http://anishpatil.fedorapeople.org/nodejs-jsonstream.spec
SRPM URL: http://anishpatil.fedorapeople.org/nodejs-JSONStream-0.10.0-1.fc20.src.rpm
Description: Streaming JSON.parse and stringify for Node.js
Fedora Account System Username:anishpatil

The "Name: nodejs-JSONStream" in the spec should be the lower case name, so that the resulting packages have a name that matches the spec.

Other than that it looks good.

Thank you for your comments, please find new review attached,

Spec URL: http://anishpatil.fedorapeople.org/nodejs-jsonstream.spec
SRPM URL: http://anishpatil.fedorapeople.org/nodejs-jsonstream-0.10.0-2.fc20.src.rpm
Description: Streaming JSON.parse and stringify for Node.js
Fedora Account System Username:anishpatil

Looks good now. Package approved.

New Package SCM Request
=======================
Package Name: nodejs-jsonstream
Short Description:  Streaming JSON.parse and stringify for Node.js
Owners: anishpatil
Branches: f20 f21 el6 epel7

Git done (by process-git-requests).

nodejs-jsonstream-0.10.0-2.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/nodejs-jsonstream-0.10.0-2.fc21

nodejs-jsonstream-0.10.0-2.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/nodejs-jsonstream-0.10.0-2.fc20

nodejs-jsonstream-0.10.0-2.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/nodejs-jsonstream-0.10.0-2.el7

nodejs-jsonstream-0.10.0-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/nodejs-jsonstream-0.10.0-2.el6

nodejs-jsonstream-0.10.0-2.el7 has been pushed to the Fedora EPEL 7 testing repository.

nodejs-jsonstream-0.10.0-2.fc20 has been pushed to the Fedora 20 stable repository.

nodejs-jsonstream-0.10.0-2.fc21 has been pushed to the Fedora 21 stable repository.

nodejs-jsonstream-0.10.0-2.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.

nodejs-jsonstream-0.10.0-2.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.