[Bug 634911] Review Request: nodejs - Evented I/O for v8 JavaScript

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=634911

--- Comment #4 from Damian Wrobel <dwrobel at ertelnet.rybnik.pl> 2010-10-21 18:06:32 EDT ---
Lubomir, please find some initial comments:

- MUST: rpmlint must be run on every package. The output should be posted in
the review.

rpmlint nodejs-0.2.1-3.fc13.src.rpm.
nodejs.src: W: spelling-error Summary(en_US) Evented -> Vented, E vented,
Evened
nodejs.src:50: W: configure-without-libdir-spec
nodejs.src:70: E: hardcoded-library-path in %{_prefix}/lib/node
nodejs.src: W: invalid-url Source0:
http://s3.amazonaws.com/four.livejournal/20100203/node-v0.2.1.tar.gz HTTP Error
403: Forbidden
1 packages and 0 specfiles checked; 1 errors, 3 warnings.

- The Source0: should point to the
http://nodejs.org/dist/node-v%{version}.tar.gz

- The %{_prefix}/lib should be replaced with %{_libdir}

NOT OK


- MUST: The package must meet the Packaging Guidelines.

- According to the guildeline[1] you can safely remove the BuildRoot from the
spec - if I'm not mistaken you're targeting only: >=F-14, >=el6.

- The package duplicates the waf[2] package which is already available in the
Fedora.

NOT OK


- MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.

OK, MIT, Modern Style with sublicense


- MUST: The License field in the package spec file must match the actual
license.

NOT OK. IMHO the license of the package is MIT only (assuming that the
duplicated "waf" files will be removed).


- MUST: The spec file must be written in American English.

- An American English is not my native language, but maybe it's a good idea to
change the "Evented" to e.g. "Event based".

- The description doesn't provide a useful information about the program. The
second sentence seems to contain irrelevant copy-paste content.

NOT OK


- MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL. Reviewers should use md5sum for this task. If no
upstream URL can be specified for this pa...

Please consider to update to the latest available version[3].

NOT OK


Additional things worth considering:

- The provided /usr/bin/node will generate a clash with the /usr/sbin/node[4]
especially when the user will have in the PATH both /usr/bin/ and /usr/sbin/
and node[4] package installed.


- I'm not sure what is the purpose of the node-waf or node-repl?


- There is the following shebang in the /usr/bin/node-repl:

#!/usr/bin/env node

which can be resolved to the /usr/sbin/node from the node[4] package.

- The -devel package contains the node.h file which includes the following
additional headers:

#include <ev.h>
#include <eio.h>
#include <v8.h>

but there is no explicit "Requires" for: v8-devel, libev-devel, libeio-devel.
It might be difficult for user to guess from which packages the aforementioned
headers comes from.

References:

[1]. http://fedoraproject.org/wiki/Packaging/Guidelines
[2]. https://admin.fedoraproject.org/pkgdb/acls/name/waf
[3]. http://nodejs.org/dist/node-v0.2.3.tar.gz
[4]. https://admin.fedoraproject.org/pkgdb/acls/name/node

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.