Bug 1161099

Review:

+ Package built successful in mock (f22 x86_64)

+ rpmlint on generated rpms gave output
nodejs-wrappy.noarch: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs-wrappy.noarch: W: only-non-binary-in-usr-lib
nodejs-wrappy.src: W: spelling-error %description -l en_US js -> dis, ks, j
2 packages and 0 specfiles checked; 0 errors, 3 warnings.

+ Source verified with upstream as (sha256sum)
srpm tarball: 550aad331e605ac0f9ba770096ca8aea7db2cbcb2d24fc16dd6f6848f227c93d
upstream tarball: 550aad331e605ac0f9ba770096ca8aea7db2cbcb2d24fc16dd6f6848f227c93d

+ License is "ISC" and included in its own LICENSE file.

+ follows nodejs packaging guidelines.

Suggestions:
1) line no. 35 contains duplicate %check, remove it.

APPROVED.

Thanks Parag for review

New Package SCM Request
=======================
Package Name: nodejs-wrappy - 
Short Description: Callback wrapping utility
Owners: anishpatil
Branches: f20 f21
InitialCC: anishpatil

Git done (by process-git-requests).

Fixed name in request.

nodejs-wrappy-1.0.1-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/nodejs-wrappy-1.0.1-1.fc20

nodejs-wrappy-1.0.1-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/nodejs-wrappy-1.0.1-1.fc21

nodejs-wrappy-1.0.1-1.fc21 has been pushed to the Fedora 21 testing repository.

nodejs-wrappy-1.0.1-1.fc21 has been pushed to the Fedora 21 stable repository.

nodejs-wrappy-1.0.1-1.fc20 has been pushed to the Fedora 20 stable repository.

Hi, I tried to build a few packages dependent on this package and found out it's broken (missing macros). Please fix this as soon as possible.

Here are my sources which pass fine in mockbuild even with tests:

Spec URL: https://fedorapeople.org/~zvetlik/nodejs/nodejs-wrappy/nodejs-wrappy.spec
SRPM URL: https://fedorapeople.org/~zvetlik/nodejs/nodejs-wrappy/nodejs-wrappy-1.0.1-3.fc23.src.rpm

May I know how do you see the package is broken(missing macros)??

Looking at your spec it looks like you wrote it exclusively for EPEL6.

In Fedora we don't need Group tag, even we don't need it on EPEL6.

You changed the source to npm registry. I really don't like those npm registry sources where I see their upstreams (mostly github sources) contain more information(like docs, test files). So better to use github sources always.

Also, there are no dependencies to get included so adding %nodejs_symlink_deps does not make any difference.

I don't know why you have added scl macros in this spec file.

The only important thing to add/change in spec is to have conditional BR:
%if 0%{?enable_tests}
BuildRequires:  npm(tap)
%endif

and enable the tests.
%check
%if 0%{?enable_tests}
%{nodejs_symlink_deps} --check
tap test/*.js
%endif

I tried packaging two modules which are dependent on nodejs-wrappy and both have failed at tests althought everything else seemed alright. When I built and installed my own rpm, everything went fine, so I took a look at your package.

I rebuilt it from sources for el7, hence the macros. Feel free to delete the scl macros and Group tag if you want.

You might want to (re)read this: https://fedoraproject.org/wiki/Packaging:Node.js?rd=Node.js/Packagers

where you can find things like:

https://fedoraproject.org/wiki/Packaging:Node.js?rd=Node.js/Packagers#Using_tarballs_from_the_npm_registry

or 

https://fedoraproject.org/wiki/Packaging:Node.js?rd=Node.js/Packagers#Symlinking_Dependencies
(note the blue section)

Yes we have a section on how to use npm registry tarballs but that I don't see guidelines mandate to use only npm registry tarball while packaging nodejs module. Its just a guideline that says npm registry tarball follows certain archiving structure where parent directory is always named as package and not by module's name-version format.

So if you still see we need to use npm registry tarball, we may need to change guidelines to use wording from
The Source0 for such modules should be of the form http://registry.npmjs.org/<modulename>/-/<modulename>-<version>.tgz. 
to
The Source0 for such modules must be of the form http://registry.npmjs.org/<modulename>/-/<modulename>-<version>.tgz. 

I still not got how just changing the tarball from github to npm helped your dependent modules test cases to work.

"The canonical method for shipping most node modules is tarballs from the npm registry."
"This method should be preferred to using checkouts from git or automatically generated tarballs from GitHub."

Also, the github source might include things the developer doesn't want to have in official tarball posted on npmjs.org

Given the tarballs are the same, the missing macros made the builds fail.

Let's have the macros fixed first. Please check the nodejs-wrappy-1.0.1-2.fc22 build. Let me know if this update don't work for you.

Unfortunately it doesn't. I tried to play around a bit with it, but I couldn't make it work properly. The only difference is that the npm tarball contains a file named package and the github tarball contains a file named wrappy-1.0.1 (this also affects the spec file) and it makes the tests fail. Although the package might be functional, I would like tests to pass.

Also, it doesn't follow the github sources guidelines either (however, the tests fail even following this guideline).

https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Github

Thank you for your feedback. Sorry as I don't know what you are working on and what tests are failing, I have again updated the package with slight change keeping github tarball. If this new package is also not working for you then I will change the source tarball.

Actually it was a packaging mistake for not using the correct module name. I remember I had checked the spec when I reviewed this package but not sure what packager imported actually.

updating package for this change
-mkdir -p %{buildroot}%{nodejs_sitelib}/nodejs-wrappy
+mkdir -p %{buildroot}%{nodejs_sitelib}/%{module_name}
 cp -pr wrappy.js package.json \
-    %{buildroot}%{nodejs_sitelib}/nodejs-wrappy
+    %{buildroot}%{nodejs_sitelib}/%{module_name}

nodejs-wrappy-1.0.1-3.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/nodejs-wrappy-1.0.1-3.fc22

nodejs-wrappy-1.0.1-3.fc22 has been pushed to the Fedora 22 stable repository.  If problems still persist, please make note of it in this bug report.

Can you please update the package in F21 too?