Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: nodejs - Evented I/O for V8 JavaScript
https://bugzilla.redhat.com/show_bug.cgi?id=732552
Summary: Review Request: nodejs - Evented I/O for V8 JavaScript Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: unspecified Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: tchollingsworth@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: ---
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.
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841(FE-NEEDSPONSOR)
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=732552
--- Comment #1 from T.C. Hollingsworth tchollingsworth@gmail.com 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.
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Alias| |nodejs
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732579
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732583
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732585
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=732552
Rahul Sundaram metherid@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |lkundrak@v3.sk
--- Comment #2 from Rahul Sundaram metherid@gmail.com 2011-08-22 22:39:00 EDT --- *** Bug 634911 has been marked as a duplicate of this bug. ***
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732590
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732216
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732597
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732598
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732636
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732639
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732643
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732650
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732655
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732660
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |732704
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=732552
Matěj Cepl mcepl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |732213
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=732552
Matěj Cepl mcepl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |mcepl@redhat.com Flag| |fedora-review?
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=732552
--- Comment #3 from Matěj Cepl mcepl@redhat.com 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?).
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=732552
--- Comment #4 from Matěj Cepl mcepl@redhat.com 2011-08-23 10:38:44 EDT --- Built on Fedora 16 with other v8.
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=732552
--- Comment #5 from Matěj Cepl mcepl@redhat.com 2011-08-23 11:12:41 EDT --- But in koji this doesn't build http://koji.fedoraproject.org/koji/taskinfo?taskID=3295369
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=732552
--- Comment #6 from T.C. Hollingsworth tchollingsworth@gmail.com 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
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=732552
--- Comment #7 from Matěj Cepl mcepl@redhat.com 2011-08-23 14:43:42 EDT --- Created attachment 519508 --> https://bugzilla.redhat.com/attachment.cgi?id=519508 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.
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=732552
--- Comment #8 from T.C. Hollingsworth tchollingsworth@gmail.com 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.)
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=732552
Adam Williamson awilliam@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |awilliam@redhat.com, | |tcallawa@redhat.com
--- Comment #9 from Adam Williamson awilliam@redhat.com 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.)
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=732552
--- Comment #10 from Matěj Cepl mcepl@redhat.com 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.
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=732552
François Kooman fkooman@tuxed.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |457343
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=732552
--- Comment #11 from T.C. Hollingsworth tchollingsworth@gmail.com 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.)
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=732552
--- Comment #12 from Matěj Cepl mcepl@redhat.com 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.
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=732552
Matěj Cepl mcepl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #13 from Matěj Cepl mcepl@redhat.com 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).
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=732552
Matěj Cepl mcepl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review+ |fedora-review?
--- Comment #14 from Matěj Cepl mcepl@redhat.com 2011-08-26 05:56:27 EDT --- Damn, back ... we have a problem ... http://koji.fedoraproject.org/koji/taskinfo?taskID=3302534
Reopening
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=732552
--- Comment #15 from Matěj Cepl mcepl@redhat.com 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.
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=732552
--- Comment #16 from Matěj Cepl mcepl@redhat.com 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?
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=732552
--- Comment #17 from T.C. Hollingsworth tchollingsworth@gmail.com 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.
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=732552
--- Comment #18 from Matěj Cepl mcepl@redhat.com 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).
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |needinfo?
--- Comment #19 from T.C. Hollingsworth tchollingsworth@gmail.com 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.)
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=732552
Matěj Cepl mcepl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|needinfo? |
--- Comment #20 from Matěj Cepl mcepl@redhat.com 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).
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=732552
--- Comment #21 from T.C. Hollingsworth tchollingsworth@gmail.com 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.
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=732552
Trever Fischer wm161@wm161.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |wm161@wm161.net
Bug 732552 depends on bug 732213, which changed state.
Bug 732213 Summary: Update v8 to 3.4.14 https://bugzilla.redhat.com/show_bug.cgi?id=732213
What |Old Value |New Value ---------------------------------------------------------------------------- Resolution| |ERRATA Status|ON_QA |CLOSED
--- Comment #22 from Trever Fischer wm161@wm161.net 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.
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=732552
Matěj Cepl mcepl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |needinfo?(wm161@wm161.net)
--- Comment #23 from Matěj Cepl mcepl@redhat.com 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
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=732552
--- Comment #24 from T.C. Hollingsworth tchollingsworth@gmail.com 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.
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |741137
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=732552
--- Comment #25 from Tom "spot" Callaway tcallawa@redhat.com 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...
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=732552
--- Comment #26 from T.C. Hollingsworth tchollingsworth@gmail.com 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.
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=732552
Eric Varsanyi fedora62@foo21.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fedora62@foo21.com
--- Comment #27 from Eric Varsanyi fedora62@foo21.com 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})
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=732552
Bug 732552 depends on bug 741137, which changed state.
Bug 741137 Summary: package http-parser as a static library https://bugzilla.redhat.com/show_bug.cgi?id=741137
What |Old Value |New Value ---------------------------------------------------------------------------- Resolution| |NOTABUG Status|NEW |CLOSED
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |741481
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=732552
Bug 732552 depends on bug 741481, which changed state.
Bug 741481 Summary: -mtune=atom on i686 breaks nodejs https://bugzilla.redhat.com/show_bug.cgi?id=741481
What |Old Value |New Value ---------------------------------------------------------------------------- Resolution| |DEFERRED Status|NEW |CLOSED
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=732552
--- Comment #28 from Matěj Cepl mcepl@redhat.com 2011-09-29 14:02:32 EDT --- Created attachment 525610 --> https://bugzilla.redhat.com/attachment.cgi?id=525610 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?
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=732552
--- Comment #29 from Eric Varsanyi fedora62@foo21.com 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.
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=732552
--- Comment #30 from Matěj Cepl mcepl@redhat.com 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.
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=732552
T.C. Hollingsworth tchollingsworth@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Blocks|177841(FE-NEEDSPONSOR) | Resolution| |WONTFIX Flag|needinfo?(wm161@wm161.net) | Last Closed| |2011-10-24 18:58:22
--- Comment #31 from T.C. Hollingsworth tchollingsworth@gmail.com 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.
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=732552
Peter Lemenkov lemenkov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |lemenkov@gmail.com Resolution|WONTFIX |DUPLICATE
--- Comment #32 from Peter Lemenkov lemenkov@gmail.com 2012-04-22 10:22:49 EDT ---
*** This bug has been marked as a duplicate of bug 815018 ***
https://bugzilla.redhat.com/show_bug.cgi?id=732552
Gregor Tätzner gregor@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |858027
https://bugzilla.redhat.com/show_bug.cgi?id=732552
Gregor Tätzner gregor@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|858027 |
https://bugzilla.redhat.com/show_bug.cgi?id=732552
Matěj Cepl mcepl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Alias|nodejs |
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla