[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 #5 from Lubomir Rintel <lkundrak at v3.sk> 2010-10-27 14:36:55 EDT ---
(In reply to comment #4)
> - The %{_prefix}/lib should be replaced with %{_libdir}
> 
> NOT OK

Not really. This is /usr/share/lib even on 64-bit architectures, since it
contains the architecture independent-code. This is consistent with Perl and
possibly other scripting languages.

Note that dependent packages that ship compiled *.node files should not go
there, this is not the case for libxmljs for which another review request is
filed.

Node currently looks into ~/.node_libraries and /usr/lib/node; an arch-specific
directory should be added there; I'll try to work that out with upstream.

> - 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.

Not sure really. I run this on el5 primarily, but use a custom-made build. I
would certainly prefer this from EPEL but don't know if it would be possible
yet. Please let me keep it for now; I'll eventually bulk-remove if from all my
packages once el6 is out and el5 will get reasonably obsolete.

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

> - 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).

Fixed

> - 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".

This is what upstream uses...
Changed to event-driven.

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

Uh, good catch. Fixed.

> - 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

Done.

> 
> 
> 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.

Upstream probably could not have chosen a more generic name. I'll leave it as
it is now and will attempt to settle this with upstream cooperation.

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

node-waf is useless when waf is packaged and is now gone.

> - 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.

Fixed. May change if upstream decides to rename the binary.

> - 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.

Fixed.

SPEC: http://v3.sk/~lkundrak/SPECS/nodejs.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/nodejs-0.2.3-1.fc13.src.rpm

-- 
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.