Bug 634911 - Review Request: nodejs - Evented I/O for v8 JavaScript
Summary: Review Request: nodejs - Evented I/O for v8 JavaScript
Keywords:
Status: CLOSED DUPLICATE of bug 815018
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 14
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Pavel Alexeev
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 668628 (view as bug list)
Depends On: 634906 634908 634909
Blocks: 635359
TreeView+ depends on / blocked
 
Reported: 2010-09-17 10:57 UTC by Lubomir Rintel
Modified: 2018-04-11 08:08 UTC (History)
19 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-08-23 02:38:59 UTC
Type: ---
Embargoed:
pahan: fedora-review?


Attachments (Terms of Use)

Description Lubomir Rintel 2010-09-17 10:57:55 UTC
SPEC: http://v3.sk/~lkundrak/SPECS/node.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/node-0.2.1-1.fc13.src.rpm

Description:

Node's goal is to provide an easy way to build scalable network programs.
In the above example, the two second delay does not prevent the server from
handling new requests.

Comment 1 Damian Wrobel 2010-10-19 17:17:34 UTC
Lubomir,

It looks that there is already the "node" package in the Fedora: 
https://admin.fedoraproject.org/pkgdb/acls/name/node

Comment 2 Lubomir Rintel 2010-10-20 14:23:52 UTC
Damian, good catch. Thank you!

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

Comment 3 Damian Wrobel 2010-10-20 20:31:36 UTC
Lubomir,
I'll take this review.

BTW, please also remember to update packages that requires the nodejs (e.g. bug #635359).

Comment 4 Damian Wrobel 2010-10-21 22:06:32 UTC
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

Comment 5 Lubomir Rintel 2010-10-27 18:36:55 UTC
(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

Comment 6 Lubomir Rintel 2010-11-04 14:58:51 UTC
(In reply to comment #5)
> > - 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.

So, upstream is not being very helpful here: http://groups.google.com/group/nodejs/browse_thread/thread/12a673a14838aa9a#

Is this a blocker? Would it make sense to persuade the other package's upstream to change their executable name? That could be easier, since that's not an interpreter so is likely to be embedded in smaller number of scripts. Also it probably did not gain very wide adoption due state of their development, here's an excerpt from their README:

  This is a simple node frontend for Linux kernel AX.25, NETROM,
  ROSE and TCP. It's based on pms.c by Alan Cox (GW4PTS) but has been
  heavily modified since. It's probably not very well tested, not
  pretty, not very flexible and it is certainly not ready! However
  I think it's already somewhat usable.

Comment 7 Damian Wrobel 2010-11-04 19:47:53 UTC
(In reply to comment #6)
> (In reply to comment #5)
> 
> So, upstream is not being very helpful here:
> http://groups.google.com/group/nodejs/browse_thread/thread/12a673a14838aa9a#
> 
> Is this a blocker? Would it make sense to persuade the other package's upstream
> to change their executable name?

It's not a blocker (at least for me) but it would be nice to choose the correct option to avoid renames in the future.

Optionally, if neither of them are willing to compromise maybe it would be a good idea to approach it as suggested in [1]? In other words assuming that vast majority of users will use binaries from distros than from sources it would become more important to have it consistent between different distros even if the name will be potentially different then used by the upstream.

References:
[1]. http://fedoraproject.org/wiki/Packaging:Conflicts#Approaching_Upstream

Comment 8 Damian Wrobel 2010-11-04 19:50:12 UTC
Please find some additional comments:

(In reply to comment #5)
> (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.

According to the [1] the %{_libdir} by default resolves to: /usr/lib or /usr/lib64 (on 64-bit architectures). You can check it by invoking:
rpm --eval "%{_libdir}" on your platform.

IMHO the usage of: %{_prefix}/lib is not correct mainly because:

a) it generates rpmlint error (already pointed in the comment #4):
 nodejs.spec:78: E: hardcoded-library-path in %{_prefix}/lib/node

b) you can't be sure that the %{_prefix}/lib directory really exists on the platform.


Is there any reason why do you manually install: node.h and node_object_wrap.h headers and do not place them in the "node" subdirectory in the %{_includedir}? It looks that you can safely use: "waf install" for that purpose.

Originally they are placed in the "node" subdirectory which is better than try to put everything directly into the %{_includedir}. It would also be appreciated to provide an appropriate pkgconfig file.


According to the [2], please add a comment or a bug link to the provided patches.


The way the %files section is organized is quite confusing, and it's very
difficult to guess what directories the package owns and whether it's done intentionally or not.


> > Please consider to update to the latest available version[3].
> > 
> > NOT OK
> 
> Done.
They've managed to release yet another version [3].


Please also use macros consistently[4].


As the package do not compile on all architectures it should have an appropriate description as per[5].


References:
[1]. http://fedoraproject.org/wiki/Packaging:RPMMacros
[2]. http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
[3]. http://nodejs.org/dist/node-v0.2.4.tar.gz
[4]. http://fedoraproject.org/wiki/Packaging/Guidelines#macros
[5]. http://fedoraproject.org/wiki/Packaging/Guidelines#Architecture_Build_Failures

Comment 9 Jason Tibbitts 2011-01-11 16:10:49 UTC
Is anything happening with this ticket?  Someone else has submitted a node.js package, and it would be a shame to close it as a duplicate when this one has stalled out.

Comment 10 Patrice FERLET 2011-01-15 00:17:36 UTC
Hi,
As I did a request with my package, you redirect me to here :) 

Please, take a look on my spec:
http://www.metal3d.org/rpms/node.js.spec

I only move libs to be architecture independant. My Package makes some warnings, but nothing that I can't correct... I only don't have lot of time last weeks.

As I said here: https://bugzilla.redhat.com/show_bug.cgi?id=668628 Koji seems to like my package.

Comment 11 Patrice FERLET 2011-01-17 00:29:49 UTC
*** Bug 668628 has been marked as a duplicate of this bug. ***

Comment 12 Patrice FERLET 2011-01-19 11:32:56 UTC
I closed my ticket... but can anybody tell me if I reopen mine because this ticket has no answer since a long time...

Comment 13 Patrice FERLET 2011-01-20 00:43:29 UTC
Excuse me to ping again...
I am in discussion with node.js admins and devs. This is a new package proposition I can make with recomandations. Note that there is a last correction to make: every python files have shebangs but this is not a good deal while they are mainly imported from 2 python scripts (rpmlint is clear, I have to remove shebangs)

I am speaking about this with the nodejs Mailing List.

Note: "node" name is already taken for another package. The better name to use is "nodejs" or "node-js"

Spec file and src.rpm:
http://www.metal3d.org/rpms/nodejs.spec
http://www.metal3d.org/rpms/nodejs-0.2.6-3.fc14.src.rpm

If someone is following this ticket, please answer :)

Comment 14 Damian Wrobel 2011-01-24 21:22:07 UTC
(In reply to comment #13)
> If someone is following this ticket, please answer :)
Patrice, I've posted comments to the Lubomir's spec file in the comment #8, if you're going to take over this request it would have to be reviewed by someone else who could sponsor you.

Comment 15 Patrice FERLET 2011-01-27 16:15:06 UTC
Hi,

Sorry for my late answer, I'm currently moving out into a new apartment and my network connexion wasn't fully working.

I looked the comment #8 and I can say that 99% of points you underlined are corrects with my spec file.

Only one is complex: lib dir.

In fact, every files are "javascript sources", so they are not platform depedents... I wonder if I can move it to "/usr/share" directory... I'm looking how it's setup.

I made 3 packages because devel files are not required to develop some applications with node.js

I use another name, according to an already existing package named "node". Can you check my spec file ?

Thanks

Comment 16 Patrice FERLET 2011-02-01 23:26:00 UTC
I made corrections :
http://www.metal3d.org/rpms/nodejs.spec
http://www.metal3d.org/rpms/nodejs-0.2.6-4.fc14.src.rpm

There still be some "review" to check to know what to do with "lib" javascript files that reside into "/usr/lib" and are not plateform dependant...

Should I do a noarch "lib" package ?

Comment 17 Bobby Powers 2011-02-02 04:43:59 UTC
(In reply to comment #16)
> I made corrections :
> http://www.metal3d.org/rpms/nodejs.spec
> http://www.metal3d.org/rpms/nodejs-0.2.6-4.fc14.src.rpm
> 
> There still be some "review" to check to know what to do with "lib" javascript
> files that reside into "/usr/lib" and are not plateform dependant...
> 
> Should I do a noarch "lib" package ?

FWIW, gnome-js-common installs into
/usr/share/gnome-js
and puts its pkgconfig file in
/usr/share/pkgconfig

http://pkgs.fedoraproject.org/gitweb/?p=gnome-js-common.git;a=blob;f=gnome-js-common.spec;h=aa584aa02b8f539f380b19e6d23eab7557cd45ba;hb=HEAD
http://koji.fedoraproject.org/koji/rpminfo?rpmID=2026030

Bobby

Comment 18 Bobby Powers 2011-02-02 04:44:47 UTC
I should have been more clear.  It installs its (platform-independent javascript files) into /usr/share/gnome-js

Comment 19 Lubomir Rintel 2011-02-20 12:36:10 UTC
* Addressed the module directory issue (in accordance with FHS)
* Correctly installing the include files
* Added comments to patches

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

Comment 20 Pavel Alexeev 2011-04-03 08:26:33 UTC
(In reply to comment #3)
> Lubomir,
> I'll take this review.
Damian, did you forgot fire fedora-review flag and set state to assigned for that bug?

Guys, I also interesting in node.js, please say if I can help in something.

Comment 21 Damian Wrobel 2011-04-03 08:51:10 UTC
(In reply to comment #20)
> Damian, did you forgot fire fedora-review flag and set state to assigned for
> that bug?
No, I didn't (just please check the history), and as I haven't received any feedback for a long time I had to drop this and now I'm a little bit busy to return to it. So if anyone is willing, feel free to take it.

Comment 22 Pavel Alexeev 2011-04-03 14:21:08 UTC
Ok, then I'll take it. Formal review follow.

Comment 23 Pavel Alexeev 2011-04-04 07:30:37 UTC
Meantime upstream last version 0.4.5 please update it.

Comment 24 Pavel Alexeev 2011-04-04 21:37:34 UTC
Legend:
+ - Ok.
- - Error.
+/- - It item acceptable, but I strongly recommend enhancement.
= - N/A, accepted.

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

nodejs.i686: W: wrong-file-end-of-line-encoding /usr/share/doc/nodejs-0.4.1/doc/sh_main.js
nodejs.i686: E: zero-length /usr/share/doc/nodejs-0.4.1/doc/api/readline.markdown
nodejs.i686: E: zero-length /usr/share/doc/nodejs-0.4.1/doc/api/string_decoder.markdown
nodejs.i686: W: wrong-file-end-of-line-encoding /usr/share/doc/nodejs-0.4.1/doc/api_assets/sh_main.js
nodejs.i686: E: zero-length /usr/share/doc/nodejs-0.4.1/doc/api/appendix_2.markdown
nodejs.i686: W: wrong-file-end-of-line-encoding /usr/share/doc/nodejs-0.4.1/doc/api/api/assets/sh_main.js
nodejs.i686: W: wrong-file-end-of-line-encoding /usr/share/doc/nodejs-0.4.1/doc/sh_vim-dark.css
nodejs-devel.i686: W: no-documentation
3 packages and 0 specfiles checked; 3 errors, 5 warnings.

At least easy to fix wrong-file-end-of-line-encoding

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
[] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
[-] MUST: The License field in the package spec file must match the actual license.
  - tools/doctool/markdown.js is Released under MIT license and                                                                                             
    Copyright 2009-2010 Dominic Baggott and Ash Berli
You do not delete this file. As it is not client-side JavaScript I think we can't easy bundle it.

  - src/platform_darwin_proctitle.cc, has code taken from the Chromium                                                                                      
    project copyright Google Inc. and released with the BSD license.

  - tools/closure_linter is copyrighted by The Closure Linter Authors and                                                                                   
    Google Inc and is released under the Apache license.
Also had not deleted in %prep.

  - doc/sh_main.js, doc/api_assets/sh_main.js, doc/api_assets/sh_javascript.min.js, doc/sh_javascript.min.js
SHJS - Syntax Highlighting in JavaScript^M                                                                                                                  
Copyright (C) 2007, 2008 gnombat.net^M                                                                                                    
License: http://shjs.sourceforge.net/doc/gplv3.html

  - test/internet/testcfg.py
  - test/message/testcfg.py
  - test/pummel/testcfg.py
  - test/simple/testcfg.py
  - tools/cpplint.py
  - tools/js2c.py
  - tools/test.py
  Copyright 2008 the V8 project authors.
  BSD License?

  - tools/doctool
  // Copyright (c) 2009-2010 Dominic Baggott                                                                                                                  
  // Copyright (c) 2009-2010 Ash Berlin 

[+] MUST: 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 must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] 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 package, please see the Source URL Guidelines for how to deal with this.
$ md5sum node-v0.4.1.tar.gz node-v0.4.1.tar.gz_srpm
9566bdbd05c18cc2bbe1fa0fba60dd0a  node-v0.4.1.tar.gz
9566bdbd05c18cc2bbe1fa0fba60dd0a  node-v0.4.1.tar.gz_srpm

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[+] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.
Build on Fedora 14 failed, but in rawhide fine - http://koji.fedoraproject.org/koji/taskinfo?taskID=2972932

[+] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
[=] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
[=] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
[+/-] MUST: Packages must NOT bundle copies of system libraries.
See notes before abount bundled libs

[=] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker.<br>
[+/-] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.

%{_prefix}/lib*/node included but empty. For what it intended?

[+] MUST: A Fedora package must not list a file more than once in the spec file's %files listings.
[+] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.
[-] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) for Ferdora < 13 and EPEL <=5
As package even not build on Fedora 14, such instruction must be removed as garbage. Damian already mention it before.

[-] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
Also must nbe removed. See before.

[+] MUST: Each package must consistently use macros.
[=] MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity).
[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present.
[+] MUST: Header files must be in a -devel package.
[=] MUST: Static libraries must be in a -static package.
[-] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).
[=] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
[+] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
[+] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
[=] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation.
[+] MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time.
[+/-] MUST: All filenames in rpm packages must be valid UTF-8.
See rpmlint comply at beginning.

== SHOULD Items: ==
[=] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[=] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
[+] SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
[=] SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
[=] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[+] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb.
[+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.


Some additional notes:
1) Is there decision about binary name? What you think about name it as package /usr/bin/nodejs?
2) Please include AUTHORS into %doc
3) In /usr/lib/pkgconfig incorrect version (0.3.2).
4) I think it is good idea include binary by exact name instead of globbing:
%{_bindir}/node
but not
%{_bindir}/*

So, as total - the big problem it licensing only. And we still have question about naming...

Comment 25 Matěj Cepl 2011-06-19 00:46:43 UTC
Lubomír, ping?

Comment 26 Ian Weller 2011-08-03 20:11:51 UTC
Ping again?

Comment 27 T.C. Hollingsworth 2011-08-20 21:04:46 UTC
Since there's been no progress on this for awhile, I'd like to take this on if I can, mainly because I want to get CoffeeScript into Fedora.  :-)

This version uses node.js' new CMake support, which permits it to easily use the shared V8, libev, and c-ares libraries.  Furthermore, I patched the cmake files to also use shared libhttp_parser and libeio so absolutely no bundled libraries are used.  (I'm submitting that patch upstream so hopefully it won't stick around too long.)

Spec: http://www.u.arizona.edu/~tchol/fedora/nodejs.spec
SRPM: http://www.u.arizona.edu/~tchol/fedora/nodejs-0.4.11-1.fc15.src.rpm

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3289240
(Node currently segfaults on Koji i686 because V8's Fedora package is outdated.  I requested it be updated in bug732213.  Note that the x86_64 build still works.)

rpmlint output:

$ rpmlint SPECS/nodejs.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint RPMS/x86_64/nodejs-0.4.11-1.fc15.x86_64.rpm 
nodejs.x86_64: W: spelling-error Summary(en_US) Evented -> Vented, E vented, Evened
nodejs.x86_64: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs.x86_64: W: spelling-error %description -l en_US scalable -> salable, callable, calculable
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

$ rpmlint RPMS/x86_64/nodejs-devel-0.4.11-1.fc15.x86_64.rpm 
nodejs-devel.x86_64: W: spelling-error Summary(en_US) Evented -> Vented, E vented, Evened
nodejs-devel.x86_64: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs-devel.x86_64: W: spelling-error %description -l en_US scalable -> salable, callable, calculable
nodejs-devel.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

$ rpmlint RPMS/noarch/nodejs-doc-0.4.11-1.fc15.noarch.rpm 
nodejs-doc.noarch: W: spelling-error Summary(en_US) Evented -> Vented, E vented, Evened
nodejs-doc.noarch: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs-doc.noarch: W: spelling-error %description -l en_US scalable -> salable, callable, calculable
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Comment 28 Pavel Alexeev 2011-08-21 11:11:14 UTC
Lubomir, last ping. We wait one week answer, then this request will be closed.

T.C. Hollingsworth, If Lubomir will not respond to that ticket in one week, please make your one new bye Fedora rules, and close that as duplicate one.

Comment 29 Matěj Cepl 2011-08-22 00:20:57 UTC
I would have one objection to this package ... common name of node.js is /usr/bin/node, not /usr/bin/nodejs. Various examples from node.js related sites don't work with /usr/bin/nodejs (for example, curl http://npmjs.org/install.sh | sh).

Comment 30 Matthew Miller 2011-08-22 00:40:16 UTC
(In reply to comment #29)
> I would have one objection to this package ... common name of node.js is
> /usr/bin/node, not /usr/bin/nodejs. Various examples from node.js related sites
> don't work with /usr/bin/nodejs (for example, curl http://npmjs.org/install.sh
> | sh).

FWIW, no current fedora package appears to use /usr/bin/node.

On the other hand: gah, please never run something like `curl http://npmjs.org/install.sh | sh`.

Comment 31 T.C. Hollingsworth 2011-08-22 01:15:42 UTC
(In reply to comment #30)
> (In reply to comment #29)
> > I would have one objection to this package ... common name of node.js is
> > /usr/bin/node, not /usr/bin/nodejs. Various examples from node.js related sites
> > don't work with /usr/bin/nodejs (for example, curl http://npmjs.org/install.sh
> > | sh).
> 
> FWIW, no current fedora package appears to use /usr/bin/node.

The "node" package uses /usr/sbin/node.  (It also occupies /usr/share/man/man1/node so even if it were acceptable to have two "node"s in $PATH one would be without a man page.)

I really wish this were not so, because identifying and sedding the appropriate files in every node package really isn't fun.
 
> On the other hand: gah, please never run something like `curl
> http://npmjs.org/install.sh | sh`.

The correct incantation of this will be "yum install npm".  I'm working on packaging its dependencies as we speak.  :-)

Comment 32 Rahul Sundaram 2011-08-22 17:15:44 UTC
T.C. Hollingsworth,  if you are taking over this package, then close this review request, open a new one and mark this one as duplicate.  Our review stats scripts and other infrastructure pieces will look at the submitter of the review request and it is useful to follow the process.

Comment 33 T.C. Hollingsworth 2011-08-22 17:30:09 UTC
(In reply to comment #32)
> T.C. Hollingsworth,  if you are taking over this package, then close this
> review request, open a new one and mark this one as duplicate.  Our review
> stats scripts and other infrastructure pieces will look at the submitter of the
> review request and it is useful to follow the process.

I was told by the reviewer of this request that I had to wait a week for the original requester to officially be deemed MIA.

Comment 34 Matěj Cepl 2011-08-22 20:12:30 UTC
(In reply to comment #33)
> (In reply to comment #32)
> > T.C. Hollingsworth,  if you are taking over this package, then close this
> > review request, open a new one and mark this one as duplicate.  Our review
> > stats scripts and other infrastructure pieces will look at the submitter of the
> > review request and it is useful to follow the process.
> 
> I was told by the reviewer of this request that I had to wait a week for the
> original requester to officially be deemed MIA.

He is not missing, just terribly overloaded and disgusted with whole nodejs/v8 business. I was chatting with him today on IRC (lkundrak on Freenode, usually somewhere around channel #fedora-cs in the European business hours). He immediately gave up on v8 maintainership (poorly, he forgot to orphan released Fedora versions) when asked to do so.

Comment 35 T.C. Hollingsworth 2011-08-22 22:01:04 UTC
(In reply to comment #34)
> He is not missing, just terribly overloaded and disgusted with whole nodejs/v8
> business.

I don't blame him, "disgusting" is an apt term.  Unfortunately, some stuff I want to use requires it, so I'd rather get it done somewhat properly rather than having to resort to yucky stuff like what Matthew Miller pointed out.

At any rate,

Comment 36 T.C. Hollingsworth 2011-08-22 22:02:22 UTC
Sorry about that.

At any rate, I've filed a new request at bug732552 and this bug can be marked
can now be marked a duplicate of that.  (I don't have the necessary
permissions.)

Comment 37 Rahul Sundaram 2011-08-23 02:38:59 UTC

*** This bug has been marked as a duplicate of bug 732552 ***

Comment 38 Peter Lemenkov 2012-04-22 14:22:18 UTC

*** This bug has been marked as a duplicate of bug 815018 ***


Note You need to log in before you can comment on or make changes to this bug.