SPEC: http://v3.sk/~lkundrak/SPECS/v8.spec SRPM: http://v3.sk/~lkundrak/SRPMS/v8-2.3.8-1.fc13.src.rpm Description: V8 is Google's open source JavaScript engine. V8 is written in C++ and is used in Google Chrome, the open source browser from Google. V8 implements ECMAScript as specified in ECMA-262, 3rd edition.
This is going to violently break my chromium builds.
(In reply to comment #1) > This is going to violently break my chromium builds. Well, you probably understand that I don't care much about a package that's not in Fedora and does not get enough upstream love either. Not a big problem for end-users either -- Google offers packages of browser that shares code base with chromium that bundles its own v8 library. On the other hand, I've checked that node.js (which is what I need this as dependency) works well with various v8 versions (I tried 2.4.4 and 2.3.8), so there's a chance that it will work with SVN snapshot current Chromium uses. Thus, if you provide a newer SVN snapshot in your chromium repository, you most likely won't break node.js. We can import the SVN current Chromium uses in Fedora as well (but as you can imagine, I won't be overly happy if we updated it frequently in Fedora). For this review, let us stick with the latest stable.
SPEC: http://v3.sk/~lkundrak/SPECS/v8.spec SRPM: http://v3.sk/~lkundrak/SRPMS/v8-2.4.4-1.fc13.src.rpm
I won't block this, but it does mean that my packages will inevitably break nodejs (or vice versa). There is no "stable" v8.
Btw they just tagged 2.4.7 http://code.google.com/p/v8/source/detail?r=5564
Ver. 2.4.9.8 http://code.google.com/p/v8/source/detail?r=5684
Peter, there's no point in keeping this updated before someone takes it for review.
I'm not committing to reviewing this but I noticed a couple things: 1) I see that we're installing jsmin.py... If this is a derivative of the jsmin stuff that Douglas Crockford did then we can't do that. jsmin is not under an fsf free license (the silly "must be used for good, not evil clause does that to us). 2) usually we don't make shared libraries if upstream is not making shared libraries due to being afraid that we'll choose a version that upstream will later stomp on. The same idea (upstream reusing the version number we use for a later, API incompatible, release of their own) would seem to apply here. However, I do see that Debian is providing versioned dynamic libs (Their patches are different, though... perhaps we should adopt their approach? http://packages.debian.org/experimental/libv8-2.4.7) The best outcome would be for upstream to start providing versioned dynamic libraries. Have you contacted them about this? The debian patches are to the build files so they could be an appropriate starting point for that discussion.
Could you please add a pkg-config file to v8-devel? It would make it easier to use as a library.
(In reply to comment #8) > I'm not committing to reviewing this but I noticed a couple things: > > 1) I see that we're installing jsmin.py... If this is a derivative of the jsmin > stuff that Douglas Crockford did then we can't do that. jsmin is not under an > fsf free license (the silly "must be used for good, not evil clause does that > to us). It's not. From ChangeLog: 2009-10-07: Version 1.3.14 Implemented a new JavaScript minifier for compressing the source of the built-in JavaScript. This removes non-Open Source code from Douglas Crockford from the project. > 2) usually we don't make shared libraries if upstream is not making shared > libraries due to being afraid that we'll choose a version that upstream will > later stomp on. The same idea (upstream reusing the version number we use for > a later, API incompatible, release of their own) would seem to apply here. > However, I do see that Debian is providing versioned dynamic libs (Their > patches are different, though... perhaps we should adopt their approach? > http://packages.debian.org/experimental/libv8-2.4.7) The best outcome would be > for upstream to start providing versioned dynamic libraries. Have you > contacted them about this? The debian patches are to the build files so they > could be an appropriate starting point for that discussion. In this version upstream provides a target to build shared libraries. Unfortunatelly, they picked a rather weird SONAME (v8-version.so; contrary to more customary v8.so.version, that Debian uses). I did not find a specification that would mandate the latter form, so I'm sticking with upstream for now. I don't care much about that though, if I'm proved that it's not a good idea I'll change it. (In reply to comment #9) > Could you please add a pkg-config file to v8-devel? It would make it easier to > use as a library. Well, not now. I guess this is something you'd need upstream to do; if upstream does not do that, packages that use v8 won't use pkg-config either. SPEC: http://v3.sk/~lkundrak/SPECS/v8.spec SRPM: http://v3.sk/~lkundrak/SRPMS/v8-2.5.9-1.fc15.src.rpm
Anything willing to review this? I believe the package is in fairly good shape now.
I'm happy to take this for review - I'm interested in all the node.js stuff, and getting this into Fedora I think is really important. I think Spot's issue with Chrome is a good point, though. As a developer I've been bitten by un-so-named libraries before, and I really think Debian's approach is a good one. But v8 is also pretty fast-moving; is it a sustainable approach or are there other ways we can avoid problems? I think there are going to be multiple consumers of v8 in the near future, Chromium being just one, and the problem needs to be addressed at some point.
(In reply to comment #12) > I'm happy to take this for review - I'm interested in all the node.js stuff, > and getting this into Fedora I think is really important. > I think Spot's issue with Chrome is a good point, though. As a developer I've > been bitten by un-so-named libraries before, and I really think Debian's > approach is a good one. But v8 is also pretty fast-moving; is it a sustainable > approach or are there other ways we can avoid problems? > > I think there are going to be multiple consumers of v8 in the near future, > Chromium being just one, and the problem needs to be addressed at some point. There's basically no issue with Chromium at this point, since Chromium is not in Fedora. Realistically, node.js is not specially picky about the version of v8 used, so it won't be difficult to find a version that fits both Chromium and node.js. (node.js worked well with any version I tried, including multiple versions from spot's chromium repository). In order to breakage in stable release, we should indeed stick with released versions and patch them for eventual (security) issues ourselves, given we can't rely on upstream doing that. Should not be hard. In two users of v8 have conflicting demands on API that can not be patched around, the last resort solution would be to use -compat packages.
I think that sounds a reasonable approach to begin with; ok. I'm taking this and will try to get an initial review in the next day or so.
Immediate problem; the package doesn't build from source. It builds for me with an additional Build-Requires of gcc-c++ - however, it really should be mock-clean. I haven't checked a mock build this time around, as I'm just taking an initial look at the package, but stuff like python-devel is going to be needed too I think. rpmlint output: rpmlint RPMS/x86_64/v8-2.5.9-1.fc14.x86_64.rpm: v8.x86_64: W: shared-lib-calls-exit /usr/lib64/libv8-2.5.9.so exit.5 v8.x86_64: W: no-manual-page-for-binary d8 1 packages and 0 specfiles checked; 0 errors, 2 warnings. I suggest we can ignore these two errors, although even a minimal man page for d8 would be nice to have. rpmlint RPMS/x86_64/v8-devel-2.5.9-1.fc14.x86_64.rpm: v8-devel.x86_64: W: no-documentation v8-devel.x86_64: E: non-executable-script /usr/lib/python2.7/site-packages/js2c.py 0644L /usr/bin/env v8-devel.x86_64: E: non-executable-script /usr/lib/python2.7/site-packages/jsmin.py 0644L /usr/bin/python2.4 1 packages and 0 specfiles checked; 2 errors, 1 warnings. I'm not mega worried about the lack of docs (although again, shipping the samples/* code would be nice I think), but the python stuff needs to be fixed imho. Arguably the jsmin.py error isn't a problem, although I would much rather it not have a shebang unless it's actually an executable (esp. since most Fedora won't have python2.4 around anyway). Also, I'm not sure js2c.py really is a library - it looks like a utility to me. I don't think that is the right location for it. rpmlint RPMS/x86_64/v8-debuginfo-2.5.9-1.fc14.x86_64.rpm: v8-debuginfo.x86_64: E: script-without-shebang /usr/src/debug/v8-2.5.9/src/compiler.cc v8-debuginfo.x86_64: E: script-without-shebang /usr/src/debug/v8-2.5.9/src/scanner.cc v8-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/v8-2.5.9/include/v8-debug.h 1 packages and 0 specfiles checked; 2 errors, 1 warnings. Various permissions errors here which should be fixed. The install -pm approach used in part of the specfile I think would be better - you could then lose the chmods and it would look a bit tidier. Other nits: - if you're going to use %{} macros, %{buildroot} looks nicer than $RPM_BUILD_ROOT rather than mixing the two styles - the .spec has some tabs and spaces mixed - Packaging guildelines state use of "ExcludeArch" with appropriate BZ references per-arch instead of "ExclusiveArch".
(In reply to comment #15) > Immediate problem; the package doesn't build from source. It builds for me with > an additional Build-Requires of gcc-c++ - however, it really should be > mock-clean. I haven't checked a mock build this time around, as I'm just taking > an initial look at the package, but stuff like python-devel is going to be > needed too I think. gcc-c++ doesn't need to be BR'd, it's guaranteed to be in the buildroot. http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
Thank you (In reply to comment #15) > v8.x86_64: W: shared-lib-calls-exit /usr/lib64/libv8-2.5.9.so exit.5 > v8.x86_64: W: no-manual-page-for-binary d8 > 1 packages and 0 specfiles checked; 0 errors, 2 warnings. > > I suggest we can ignore these two errors, although even a minimal man page for > d8 would be nice to have. I don't have resources to write the manual myself (I sort of hoped Debian would create one; they're quite good at that ;) -- I don't know much about d8 myself and nothing that would help me is included in the source. I guess this is best left as it is. > rpmlint RPMS/x86_64/v8-devel-2.5.9-1.fc14.x86_64.rpm: > > v8-devel.x86_64: W: no-documentation > v8-devel.x86_64: E: non-executable-script > /usr/lib/python2.7/site-packages/js2c.py 0644L /usr/bin/env > v8-devel.x86_64: E: non-executable-script > /usr/lib/python2.7/site-packages/jsmin.py 0644L /usr/bin/python2.4 > 1 packages and 0 specfiles checked; 2 errors, 1 warnings. > > I'm not mega worried about the lack of docs (although again, shipping the > samples/* code would be nice I think), These probably are not of much use to most -devel package users; they are built and used as part of the test use, but I don't think it's very wise or useful to ship them with the package. > but the python stuff needs to be fixed > imho. Arguably the jsmin.py error isn't a problem, although I would much rather > it not have a shebang unless it's actually an executable (esp. since most > Fedora won't have python2.4 around anyway). > > Also, I'm not sure js2c.py really is a library - it looks like a utility to me. > I don't think that is the right location for it. Stripped the shebang from both, made a /usr/bin/js2c wrapper for js2c.py which is supposed to be an user-called tool as well. > > rpmlint RPMS/x86_64/v8-debuginfo-2.5.9-1.fc14.x86_64.rpm: > > v8-debuginfo.x86_64: E: script-without-shebang > /usr/src/debug/v8-2.5.9/src/compiler.cc > v8-debuginfo.x86_64: E: script-without-shebang > /usr/src/debug/v8-2.5.9/src/scanner.cc > v8-debuginfo.x86_64: W: spurious-executable-perm > /usr/src/debug/v8-2.5.9/include/v8-debug.h > 1 packages and 0 specfiles checked; 2 errors, 1 warnings. > > Various permissions errors here which should be fixed. The install -pm approach > used in part of the specfile I think would be better - you could then lose the > chmods and it would look a bit tidier. Fixed. Still had to use chmod for -debuginfo. > - if you're going to use %{} macros, %{buildroot} looks nicer than > $RPM_BUILD_ROOT rather than mixing the two styles Well, the consistent use of macros guideline forbids mixing of %{buildroot} with $RPM_BUILD_ROOT or %{optflags} with $RPM_OPT_FLAGS, which I don't do. I believe that consistency with existing SPEC files is more important than anything else here. Most packages use $RPM_BUILD_ROOT for build root (and %{optflags} for compiler flags) and so do many tools that generate SPEC files (e.g. cpanspec). I'm really accustomed to that, it makes the SPEC files more legible to me and would like it to stay it that way (at least until a guideline deals with this diversity). > - the .spec has some tabs and spaces mixed /me looks down in shame uh, okay, fixed. > - Packaging guildelines state use of "ExcludeArch" with appropriate BZ > references per-arch instead of "ExclusiveArch". A guideline is probably wrong here. In this case the package it's really not a bug, given it is really specific to certain architectures those are only ones v8 JIT compilers were written for. SPEC: http://v3.sk/~lkundrak/SPECS/v8.spec SRPM: http://v3.sk/~lkundrak/SRPMS/v8-2.5.9-2.fc14.src.rpm
Nice job Lubomir. There's still a permissions problem in debuginfo: $ rpmlint RPMS/x86_64/v8-debuginfo-2.5.9-2.fc14.x86_64.rpm v8-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/v8-2.5.9/include/v8-debug.h 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Otherwise, the packages look very clean; I've done the full check against the Guidelines and I can't see anything new that we haven't already got on the bug. One minor comment/question: is there a reason you're pulling from trunk? # U=http://v8.googlecode.com/svn/trunk/ # R=$(svn log $U |awk '/^r[0-9]* / {r=$1} /2\.5\.9/ {print r; exit}') # svn export -$R $U v8-2.5.9 # tar czf v8-2.5.9.tar.gz v8-2.5.9 I would normally expect a tagged pull, e.g. # svn export http://v8.googlecode.com/svn/tags/2.5.9/ v8-2.5.9 # tar czf v8-2.5.9.tar.gz v8-2.5.9 As well as losing $U and $R, I would think it would be more reliable for finding the right source than trawling the svn log. If the tag is different to what ends up on trunk (?!) that would be worth an extra comment I think. There is another problem remaining: > - Packaging guildelines state use of "ExcludeArch" with appropriate BZ > references per-arch instead of "ExclusiveArch". Having thought about it, I'm inclined to agree with you. My understanding of the guideline is that we should only list known-not-to-work arches so that if some new arch comes along, we give it the benefit of the doubt until it's shown also not to work. However, in the case of v8 (which is really a JIT/compiler), that's not a sensible approach: it's not going to magically support new arches without significant upstream work. But we're left with the problem that this guideline is a MUST:, and that this problem is therefore considered to be a blocker. I will raise this on fedora-devel-list as I think this is a problem for me as the reviewer, rather than you as packager. However, if you wanted to change this to ExcludeArch in the meantime for the purposes of passing the review, that would be ok too.
Ok, consensus (so far) in this case is that we can ignore the Guidelines on ExcludeArch:, so you can keep the ExclusiveArch as it is I think: http://lists.fedoraproject.org/pipermail/devel/2010-December/147023.html So the only outstanding problem left is the permissions of the .h file in -debuginfo.
(In reply to comment #18) > There's still a permissions problem in debuginfo: > > $ rpmlint RPMS/x86_64/v8-debuginfo-2.5.9-2.fc14.x86_64.rpm > v8-debuginfo.x86_64: W: spurious-executable-perm > /usr/src/debug/v8-2.5.9/include/v8-debug.h > 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Fixed. > One minor comment/question: is there a reason you're pulling from trunk? > I would normally expect a tagged pull, e.g. > > # svn export http://v8.googlecode.com/svn/tags/2.5.9/ v8-2.5.9 > # tar czf v8-2.5.9.tar.gz v8-2.5.9 If I recall correctly they did not use to tag releases. They seem to do so now though, so I changed the source to tags/ as you have proposed. SPEC: http://v3.sk/~lkundrak/SPECS/v8.spec SRPM: http://v3.sk/~lkundrak/SRPMS/v8-3.0.0.1-1.fc15.src.rpm
Sorry to be the bearer of bad news! There's still one existing problem, and I think I've found a new one :( There is still a perms issue in -debuginfo: $ rpmlint RPMS/x86_64/v8-debuginfo-3.0.0.1-1.fc14.x86_64.rpm v8-debuginfo.x86_64: E: script-without-shebang /usr/src/debug/v8-3.0.0.1/src/compiler.cc v8-debuginfo.x86_64: E: script-without-shebang /usr/src/debug/v8-3.0.0.1/src/scanner.cc 1 packages and 0 specfiles checked; 2 errors, 0 warnings. Looking at the .spec, I think you'd need to add another -print0 before the -o in order to get the desired effect, this is probably shorter though: find -regex '.*\.\(cc\|h\)' -exec chmod -x {} \; However, there is a new issue as well. I'm sorry I didn't pick this up before, but I rechecked the licensing with the new 3.0 source package and I think I've found a problem. These files: benchmarks/earley-boyer.js benchmarks/raytrace.js ... are not copyright the V8 project and don't have any apparent license with them. I think this renders them non-distributable in the source even though they're not included in the binaries.
(In reply to comment #21) > There is still a perms issue in -debuginfo: Oh god, I suck. I actually confirmed it with rpmlint myself this time. > However, there is a new issue as well. I'm sorry I didn't pick this up before, > but I rechecked the licensing with the new 3.0 source package and I think I've > found a problem. These files: > > benchmarks/earley-boyer.js > benchmarks/raytrace.js > > ... are not copyright the V8 project and don't have any apparent license with > them. I think this renders them non-distributable in the source even though > they're not included in the binaries. Good catch. Removed them from the tarball. SPEC: http://v3.sk/~lkundrak/SPECS/v8.spec SRPM: http://v3.sk/~lkundrak/SRPMS/v8-3.0.0.1-2.fc14.src.rpm
Sorry for taking so long to pick this up again; I've been a bit ill this last week :( rpmlint outputs: $ rpmlint SPECS/v8.spec SPECS/v8.spec: W: invalid-url Source0: v8-3.0.0.1.tar.gz 0 packages and 1 specfiles checked; 0 errors, 1 warnings. $ rpmlint SRPMS/v8-3.0.0.1-2.fc14.src.rpm v8.src: W: invalid-url Source0: v8-3.0.0.1.tar.gz 1 packages and 0 specfiles checked; 0 errors, 1 warnings. $ rpmlint RPMS/x86_64/v8-3.0.0.1-2.fc14.x86_64.rpm v8.x86_64: W: shared-lib-calls-exit /usr/lib64/libv8-3.0.0.1.so exit.5 v8.x86_64: W: no-manual-page-for-binary js2c v8.x86_64: W: no-manual-page-for-binary d8 1 packages and 0 specfiles checked; 0 errors, 3 warnings. $ rpmlint RPMS/x86_64/v8-devel-3.0.0.1-2.fc14.x86_64.rpm v8-devel.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. $ rpmlint RPMS/x86_64/v8-debuginfo-3.0.0.1-2.fc14.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. No errors, and the warnings have already been accounted for. No problems there. Going through the review guidelines again: * Does not comply with ExcludeArch MUST, see c19. * All other MUST complied with. I can't find any other issues with the packages, therefore it passes my review and I'm happy to approve v8.
Thanks a lot Alex! New Package SCM Request ======================= Package Name: v8 Short Description: JavaScript Engine Owners: lkundrak Branches: f14 el6
Git done (by process-git-requests).
Imported and built.