Bug 732552 - Review Request: nodejs - Evented I/O for V8 JavaScript
Review Request: nodejs - Evented I/O for V8 JavaScript
Status: CLOSED DUPLICATE of bug 815018
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Matěj Cepl
Fedora Extras Quality Assurance
:
Depends On: 732213 741137 741481
Blocks: 457343 732216 732579 732583 732585 732590 732597 732598 732636 732639 732643 732650 732655 732660 732704
  Show dependency treegraph
 
Reported: 2011-08-22 17:56 EDT by T.C. Hollingsworth
Modified: 2012-10-05 05:55 EDT (History)
16 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-10-24 18:58:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mcepl: fedora‑review?


Attachments (Terms of Use)
stdout/stderr of the failed build (5.36 KB, text/plain)
2011-08-23 14:43 EDT, Matěj Cepl
no flags Details
reconstructed patch (1.35 KB, patch)
2011-09-29 14:02 EDT, Matěj Cepl
no flags Details | Diff

  None (edit)
Description T.C. Hollingsworth 2011-08-22 17:56:26 EDT
Spec URL: http://www.u.arizona.edu/~tchol/fedora/nodejs.spec
SRPM URL: http://www.u.arizona.edu/~tchol/fedora/nodejs-0.4.11-1.fc15.src.rpm
Description:
Node.js is a server-side JavaScript environment that uses an asynchronous
event-driven model.  Node's goal is to provide an easy way to build scalable
network programs.
Comment 1 T.C. Hollingsworth 2011-08-22 20:20:57 EDT
$ 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.
Comment 2 Rahul Sundaram 2011-08-22 22:39:00 EDT
*** Bug 634911 has been marked as a duplicate of this bug. ***
Comment 3 Matěj Cepl 2011-08-23 10:37:09 EDT
+ 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?).
Comment 4 Matěj Cepl 2011-08-23 10:38:44 EDT
Built on Fedora 16 with other v8.
Comment 5 Matěj Cepl 2011-08-23 11:12:41 EDT
But in koji this doesn't build http://koji.fedoraproject.org/koji/taskinfo?taskID=3295369
Comment 6 T.C. Hollingsworth 2011-08-23 12:04:28 EDT
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
Comment 7 Matěj Cepl 2011-08-23 14:43:42 EDT
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.
Comment 8 T.C. Hollingsworth 2011-08-24 15:51:59 EDT
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.)
Comment 9 Adam Williamson 2011-08-25 01:41:58 EDT
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.)
Comment 10 Matěj Cepl 2011-08-25 02:49:47 EDT
(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.
Comment 11 T.C. Hollingsworth 2011-08-25 22:25:53 EDT
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.)
Comment 12 Matěj Cepl 2011-08-26 05:24:07 EDT
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.
Comment 13 Matěj Cepl 2011-08-26 05:28:02 EDT
(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).
Comment 14 Matěj Cepl 2011-08-26 05:56:27 EDT
Damn, back ... we have a problem ... http://koji.fedoraproject.org/koji/taskinfo?taskID=3302534

Reopening
Comment 15 Matěj Cepl 2011-08-26 06:03:33 EDT
(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.
Comment 16 Matěj Cepl 2011-08-26 07:13:25 EDT
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?
Comment 17 T.C. Hollingsworth 2011-08-26 16:32:26 EDT
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.
Comment 18 Matěj Cepl 2011-08-27 13:38:28 EDT
(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).
Comment 19 T.C. Hollingsworth 2011-08-27 18:36:30 EDT
(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.)
Comment 20 Matěj Cepl 2011-08-27 18:59:49 EDT
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).
Comment 21 T.C. Hollingsworth 2011-08-27 19:19:58 EDT
(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.
Comment 22 Torrie Fischer 2011-09-22 23:41:52 EDT
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.
Comment 23 Matěj Cepl 2011-09-23 06:07:08 EDT
(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
Comment 24 T.C. Hollingsworth 2011-09-25 21:28:06 EDT
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.
Comment 25 Tom "spot" Callaway 2011-09-25 21:34:03 EDT
I'm pretty sure that will violate the policy against bundling libraries, unless there is an http-parser package with static libs...
Comment 26 T.C. Hollingsworth 2011-09-25 22:19:28 EDT
(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.
Comment 27 Eric Varsanyi 2011-09-26 19:21:14 EDT
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})
Comment 28 Matěj Cepl 2011-09-29 14:02:32 EDT
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?
Comment 29 Eric Varsanyi 2011-09-29 15:22:24 EDT
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.
Comment 30 Matěj Cepl 2011-09-29 16:42:01 EDT
(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.
Comment 31 T.C. Hollingsworth 2011-10-24 18:58:22 EDT
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/
Comment 32 Peter Lemenkov 2012-04-22 10:22:49 EDT

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