Bug 732552

$ 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/noarch/nodejs-devel-0.4.11-1.fc15.noarch.rpm 
nodejs-devel.noarch: W: spelling-error Summary(en_US) Evented -> Vented, E vented, Evened
nodejs-devel.noarch: W: spelling-error %description -l en_US js -> dis, ks, j
nodejs-devel.noarch: W: spelling-error %description -l en_US scalable -> salable, callable, calculable
nodejs-devel.noarch: W: conffile-without-noreplace-flag /etc/rpm/macros.nodejs
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.

*** Bug 634911 has been marked as a duplicate of this bug. ***

+ MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.
$ rpmlint ./nodejs-0.4.11-1.fc15.src.rpm RPMS/*/nodejs*.rpm\
    |grep -v spelling-error|grep -v '^\s*$'
nodejs-devel.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 0 errors, 13 warnings.
+ MUST: The package must be named according to the Package Naming Guidelines.
There is a conflict with already existing package node, so rename is allowed.
+ 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.
* I don't know if it is my English as a second language or what, but I don't understand this

# some partially generated docs in the tarball gum up the works
rm -rf doc/api/api

What's going on?

* mv doc/nodejs.1.gz %{buildroot}%{_mandir}/man1/
please don't mv, do rather cp -p ... instead. There could be an issue with SELinux labels, ownerships etc. corrupted.

* failing tests
# Fails 5 tests ATM
# known upstream: https://github.com/joyent/node/pull/1021
##%%check
##cd build
##ctest

Not reason to break the review, but couldn't you just switch off just the specific tests (renaming the file with test should help or something)?

* %doc %{_mandir}/man1/nodejs.1.gz
No, it is not %doc. %doc should be README.md, LICENSE etc., not manpage.

+ 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.
+ 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: ac4c3eaa0667d5e3eacf56fd26a4eadc
+ 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.
- 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.
Isn't this nodejs still dependent on v8 from the repo outside of Fedora? This package review is blocked by bug 732213 where we need to resolve this mess.
+ MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
no locales
+ 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.
+ 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.
+ 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.
+ MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)
+ MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example.
+ MUST: Each package must consistently use macros.
+ MUST: The package must contain code, or permissable content.
+ 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: 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}%{?_isa} = %{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.
+ 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 builds in mock.
+ SHOULD: The package should compile and build into binary rpms on all supported architectures.
+ 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.
+ SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.

------------
Please fix the issue indicated in Package Guidelines section and we have to wait on v8 (right?).

Built on Fedora 16 with other v8.

But in koji this doesn't build http://koji.fedoraproject.org/koji/taskinfo?taskID=3295369

Thank you for taking this on, and all your other help!

(In reply to comment #3)
> * I don't know if it is my English as a second language or what, but I don't
> understand this
> 
> # some partially generated docs in the tarball gum up the works
> rm -rf doc/api/api
> 
> What's going on?

Your English is fine, it's mine that leaves a little to be desired.  ;-)

Upstream ships API docs in doc/api in Markdown format, which are turned into HTML by "make doc".  Unfortunately, they also shipped partially generated ones in doc/api/api, and "make doc" tries to convert those too, but fails because they're not markdown.  I rewrote that comment to be more clear.
 
> * mv doc/nodejs.1.gz %{buildroot}%{_mandir}/man1/
> please don't mv, do rather cp -p ... instead. There could be an issue with
> SELinux labels, ownerships etc. corrupted.

Fixed.

> * failing tests
> # Fails 5 tests ATM
> # known upstream: https://github.com/joyent/node/pull/1021
> ##%%check
> ##cd build
> ##ctest
> 
> Not reason to break the review, but couldn't you just switch off just the
> specific tests (renaming the file with test should help or something)?

I have identified and disabled the broken tests and re-enabled %check.

> * %doc %{_mandir}/man1/nodejs.1.gz
> No, it is not %doc. %doc should be README.md, LICENSE etc., not manpage.

Fixed.

>Isn't this nodejs still dependent on v8 from the repo outside of Fedora? This
package review is blocked by bug 732213 where we need to resolve this mess.

(In reply to comment #5)
>But in koji this doesn't build
>http://koji.fedoraproject.org/koji/taskinfo?taskID=3295369

Actually, it builds fine with the V8 currently in the main Fedora repo.  http-parser was breaking the build.  When I force the bundled http-parser, it builds fine on Koji:  http://koji.fedoraproject.org/koji/taskinfo?taskID=3295572

It also builds fine locally with the RPM you pushed to bodhi yesterday.

--

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

Created attachment 519508 [details]
stdout/stderr of the failed build

(In reply to comment #6)
> Upstream ships API docs in doc/api in Markdown format, which are turned into
> HTML by "make doc".  Unfortunately, they also shipped partially generated ones
> in doc/api/api, and "make doc" tries to convert those too, but fails because
> they're not markdown.  I rewrote that comment to be more clear.

Understood.

> Actually, it builds fine with the V8 currently in the main Fedora repo. 
> http-parser was breaking the build.  When I force the bundled http-parser, it
> builds fine on Koji: 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=3295572

We've got a problem. Apparently nodejs still depends on some bundled libraries. Add to the end of %prep section

# Remove all bundled libraries
rm -rf deps/

Build fails then. I think we should make it working with these lines included in spec before we can get this into Fedora.

Building nodejs depends on some Python scripts from the tools directory of the upstream V8 distribution.  If these were shipped in v8-devel I could patch the cmake files to use that instead.  (It looks like Lubomir's version included them but Spot's does not.)

so, let's CC spot!

(also CC me, cos I'm interested. this would allow etherpad-lite to be packaged. shout if I can help out.)

(In reply to comment #9)
> so, let's CC spot!
> 
> (also CC me, cos I'm interested. this would allow etherpad-lite to be packaged.
> shout if I can help out.)

V8 issues are on bug 732213 ... I don't think poor spot should be bothered with this, which he don't want to have any business with.

This now builds successfully with v8-devel and rm -rf deps.

Since both Lubomir's old v8-devel and the new v8-devel put jsmin in %python_sitelib, this will build successfully with either version.  It will not build with the v8-devel from spot's Chromium repository as that does not install jsmin.py, but it will run fine if just installed with it.

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

rpmlint reports nothing new.

(P.S. I gave http-parser good karma so it can be pushed to stable, whereupon this should build successfully in koji.)

Sorry for bothering you (not sure whether this shouldn't go to new bug, or whether this is a package review blocker; if you disagree with having it in this bug, protest violently). Apparently npm install breaks on files being elsewhere then expected (https://github.com/polotek/libxmljs/issues/69). Yes, I agree that preferred method of installation packages should be yum install ..., but all programming languages in Fedora support both packaged and unpackaged packages.

(In reply to comment #12)
> Sorry for bothering you (not sure whether this shouldn't go to new bug, or
> whether this is a package review blocker; if you disagree with having it in
> this bug, protest violently). Apparently npm install breaks on files being
> elsewhere then expected (https://github.com/polotek/libxmljs/issues/69). Yes, I
> agree that preferred method of installation packages should be yum install ...,
> but all programming languages in Fedora support both packaged and unpackaged
> packages.

Sorry, this is not nodejs bug, but npm.

Otherwise this review is APPROVED. Ask for git repo (you can put me on CC).

Damn, back ... we have a problem ... http://koji.fedoraproject.org/koji/taskinfo?taskID=3302534

Reopening

(In reply to comment #14)
> Damn, back ... we have a problem ...
> http://koji.fedoraproject.org/koji/taskinfo?taskID=3302534
> 
> Reopening

Of course, it crashed for me even with my best v8 we have.

And that npm actually could be this package's problem:

bradford:nodejs $ node --vars
NODE_PREFIX: /usr
NODE_CFLAGS: -g -O3 -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic  -rdynamic  -D__POSIX__=1 -DHAVE_FDATASYNC=1 -DPLATFORM="linux" -DX_STACKSIZE=65536 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DEV_MULTIPLICITY=1 -D_FORTIFY_SOURCE=2 -DHAVE_CONFIG_H=1 -DHAVE_OPENSSL=1 -I/usr/include/node

(and https://github.com/polotek/libxmljs/issues/69#issuecomment-1911676)

That -I/usr/include/node is probably not right, right?

The build is still failing because YUM is still installing the old http-parser 0.3-6.20100911git.fc15.  It won't succeed until 1.0 is in the buildroot.

I'll patch CMake to set the -I in CFLAGS properly but I still think that's npm's problem.

(In reply to comment #17)
> The build is still failing because YUM is still installing the old http-parser
> 0.3-6.20100911git.fc15.  It won't succeed until 1.0 is in the buildroot.
> 
> I'll patch CMake to set the -I in CFLAGS properly but I still think that's
> npm's problem.

Fails even with http-parser-1.0-1.fc16 in the buildroot (http://koji.fedoraproject.org/koji/getfile?taskID=3305668&name=build.log, http://koji.fedoraproject.org/koji/taskinfo?taskID=3305667 ... something about jsmin.py).

(In reply to comment #18)
> Fails even with http-parser-1.0-1.fc16 in the buildroot
> (http://koji.fedoraproject.org/koji/getfile?taskID=3305668&name=build.log,
> http://koji.fedoraproject.org/koji/taskinfo?taskID=3305667 ... something about
> jsmin.py).

That looks like it's your version of nodejs release 2 where you added "rm -rf deps".  Release 3 has the patch that fixes jsmin with "rm -rf deps" and should build.  (I'm away from my normal machine for the weekend so I can't run a koji build myself right now, sorry.)

Yes, I was using wrong v8. I suggest in bug 732213 just to merge all our changes to master (which I did) and call it a day on changes to v8.

http://koji.fedoraproject.org/koji/taskinfo?taskID=3306054 failed, but I believe it is because of some tests expecting Internet connection (which is not available in koji machines ... all build machines are isolated from network for security reasons). Am I right?

If yes, I would argue that it is a bug in tests, but that's certainly nothing related to this package review.

When you either make npm install libxmljs working or show me that the bug is not in nodejs, I'll would think this package is done. Except we need a sponsor to stamp his approval on this (I am not).

(In reply to comment #20)
> http://koji.fedoraproject.org/koji/taskinfo?taskID=3306054 failed, but I
> believe it is because of some tests expecting Internet connection (which is not
> available in koji machines ... all build machines are isolated from network for
> security reasons). Am I right?

Well, that and i686 is still segfaulting!  I really don't understand why it segfaults on i686 and not x86_64.  I can't even reproduce it running an i686 build on x86_64, so I'm going to have to put together a i686 VM and see what I can figure out.

> If yes, I would argue that it is a bug in tests, but that's certainly nothing
related to this package review.

Looking at the tests, it should only require a working loopback device.  Is that blocked as well?

> When you either make npm install libxmljs working or show me that the bug is
not in nodejs, I'll would think this package is done. Except we need a sponsor
to stamp his approval on this (I am not).

I got around to patching the -I CFLAG but didn't get a chance to test it before I had to leave.  I'll try and have a new build done early next week.

Poking this bug because I'm interested in having nodejs for fedora. Any update on things? If this is stalled I might be able to take on the package.

(In reply to comment #22)
> Poking this bug because I'm interested in having nodejs for fedora. Any update
> on things? If this is stalled I might be able to take on the package.

Well, see comment 21. The last src.rpm is http://mcepl.fedorapeople.org/rpms/nodejs-0.4.11-3.fc16.src.rpm If you are able to make it work, your help would be more than appreciated.

Thank you

Sorry, I've been extremely busy lately and haven't had much time to work on this.  The time I did have hasn't been very fruitful, either.

I think the best way to go is to stop trying to do http-parser as a shared library and link statically instead as upstream does.  See bug 741137 for details.

I'll have a new nodejs build with that, the new upstream release, and (hopefully) fixing npm this week.

I'm pretty sure that will violate the policy against bundling libraries, unless there is an http-parser package with static libs...

(In reply to comment #25)
> I'm pretty sure that will violate the policy against bundling libraries, unless
> there is an http-parser package with static libs...

I'm still going to nuke the bundled copy, I just want to ship a static library instead of a shared one in the http-parser package.  The complete rationale is in bug 741137.

The spec file in http-parser (from bug 741137) creates a .o file only rather than a library. Below is a hack (to illustrate only) to the node js's cmake input to link with this .o rather than the expected shared or static lib (apologies for my lack of cmake foo):

d2 ~/rpmbuild/BUILD$ cat ../SOURCES/nodejs-static-http.patch 
diff -Naur node-v0.4.12.ORIG/cmake/libhttp_parser.cmake node-v0.4.12/cmake/libhttp_parser.cmake
--- node-v0.4.12.ORIG/cmake/libhttp_parser.cmake	2011-09-26 15:07:53.787962674 -0500
+++ node-v0.4.12/cmake/libhttp_parser.cmake	2011-09-26 18:11:38.807130936 -0500
@@ -1,10 +1,10 @@
 if(SHARED_HTTP_PARSER)
-  find_library(HTTP_PARSER_LIBRARY NAMES http_parser)
+  find_file(HTTP_PARSER_OBJ http_parser.o PATHS /usr/lib64/http-parser)
   find_path(HTTP_PARSER_INCLUDE_DIR http_parser.h
-    PATH_SUFFIXES include
+    PATH_SUFFIXES include/http-parser
     ) # Find header
-  find_package_handle_standard_args(http_parser DEFAULT_MSG HTTP_PARSER_LIBRARY HTTP_PARSER_INCLUDE_DIR)
+  find_package_handle_standard_args(http_parser DEFAULT_MSG HTTP_PARSER_OBJ HTTP_PARSER_INCLUDE_DIR)
 else()
   add_subdirectory(deps/http_parser)
   set(HTTP_PARSER_INCLUDE_DIR deps/http_parser)
-endif()
\ No newline at end of file
+endif()
diff -Naur node-v0.4.12.ORIG/cmake/node_build.cmake node-v0.4.12/cmake/node_build.cmake
--- node-v0.4.12.ORIG/cmake/node_build.cmake	2011-09-26 15:07:53.787962674 -0500
+++ node-v0.4.12/cmake/node_build.cmake	2011-09-26 18:08:09.811284914 -0500
@@ -112,7 +112,7 @@
   ev
   eio
   cares
-  http_parser
+  ${HTTP_PARSER_OBJ}
   ${V8_LIBRARY_PATH}
   ${CMAKE_THREAD_LIBS_INIT}
   ${extra_libs})

Created attachment 525610 [details]
reconstructed patch

(In reply to comment #27)
> The spec file in http-parser (from bug 741137) creates a .o file only rather
> than a library. Below is a hack (to illustrate only) to the node js's cmake
> input to link with this .o rather than the expected shared or static lib
> (apologies for my lack of cmake foo):

When applying the attached patch (please attach your patch, don't paste it, so as to avoid word-wrapping), I get this error:

 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic  -rdynamic
  CPPFLAGS:            -D__POSIX__=1 -DHAVE_FDATASYNC=1 -DPLATFORM="linux" -DX_STACKSIZE=65536 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DEV_MULTIPLICITY=1 -D_FORTIFY_SOURCE=2 -DHAVE_CONFIG_H=1 -DHAVE_OPENSSL=1
CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
HTTP_PARSER_OBJ
    linked by target "node" in directory /home/matej/build/BUILD/node-v0.4.11

Also, hardcoded /usr/lib64 is probably not The Right Thing™ to do, right?

Apologies, this patch was meant only to illustrate the issue, not propose a solution. It looks like bug 741137 (which created the .o file rather than the library) was closed as 'wontfix' so I'm not sure where this is all going now.

Putting the lib64 path in there directly is mostly definitely not The Right Thing, if it turns out 741137 is revived and the http parser lib stays a .o I will gladly figure out how to get the path via the correct variable in cmake and make a real patch.

(In reply to comment #29)
> Apologies, this patch was meant only to illustrate the issue, not propose a
> solution. It looks like bug 741137 (which created the .o file rather than the
> library) was closed as 'wontfix' so I'm not sure where this is all going now.

If you create a patch for bug 741137, than I’ll make sure it will get applied and sent upstream. I think it was more desperation, that solution to that bug is not complete solution as well. I believe you are on the right track, and moreover I don't think randomly scattered .o files are a good idea.

Per our discussion via e-mail, I'm going to ship Node.js in a third-party repository [1] for the time being, until such time that the bundled library problems can be patched or upstream decides they're worth working out:

>The big issue right now is bundled library madness.  It gets a lot
worse with 0.5, where upstream moved the ev/eio stuff into its own
library (libuv) that abstracts that out so Windows support can be
added.  That library is presently a spaghetti of bundled libraries
with just enough minor changes to make it unreasonably difficult to
split them out.  I'm hesitant to bother with a working 0.4 if 0.6
(which is due to be released pretty soon now) is never going to be
acceptable for Fedora.

>I fear the only way it might make it into Fedora is for FPC to decide
they've modified it enough to grant an exception, but that seems
unlikely (since this is pretty much the same situation Chromium is
in).  It might be wiser to just maintain it in a third-party
repository for the moment.

In the meantime, I'm closing out all my Node.js-related review requests.

[1] http://nodejs.tchol.org/

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