Bug 718681 - Review Request: luajit - Just-In-Time Compiler for Lua
Summary: Review Request: luajit - Just-In-Time Compiler for Lua
Keywords:
Status: CLOSED DUPLICATE of bug 1035661
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-04 09:13 UTC by Andrei Lapshin
Modified: 2014-01-18 22:22 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-11-28 09:30:07 UTC
Type: ---


Attachments (Terms of Use)
Spec file diff (1.77 KB, patch)
2011-10-09 14:31 UTC, Andrei Lapshin
no flags Details | Diff
Latest spec file (2.57 KB, text/x-rpm-spec)
2011-10-09 14:32 UTC, Andrei Lapshin
no flags Details
Spec file diff with fixed typo (511 bytes, patch)
2011-10-09 17:04 UTC, Andrei Lapshin
no flags Details | Diff
Latest spec file (2.57 KB, text/x-rpm-spec)
2011-10-09 17:06 UTC, Andrei Lapshin
no flags Details
Spec file diff from previous version (1.77 KB, patch)
2011-10-09 17:07 UTC, Andrei Lapshin
no flags Details | Diff
Latest spec file (2.80 KB, text/x-rpm-spec)
2012-02-06 16:53 UTC, Andrei Lapshin
no flags Details
Spec diff (3.72 KB, patch)
2012-02-06 16:54 UTC, Andrei Lapshin
no flags Details | Diff
luajit-2.0.2.spec (2.72 KB, text/plain)
2013-09-02 06:41 UTC, Muayyad Alsadi
no flags Details

Description Andrei Lapshin 2011-07-04 09:13:08 UTC
Spec URL: http://dl.dropbox.com/u/11270386/luajit.spec
SRPM URL: http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.1.beta8.fc16.src.rpm

Description:
LuaJIT implements the full set of language features defined by Lua 5.1.
The virtual machine (VM) is API- and ABI-compatible to the standard
Lua interpreter and can be deployed as a drop-in replacement.

Comment 1 Damian L Brasher 2011-07-04 11:19:21 UTC
Informal review, initial check,

rpmlint -i luajit.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint -i luajit-2.0.0-0.1.beta8.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Damian Brasher

Comment 2 Andrei Lapshin 2011-07-04 17:28:31 UTC
After build libluajit-5.1.so.2.0.0 was not executable and rpmbuild created wrong cyclic dependencies. Fixed.

Spec URL: http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.1.beta8.fc16.src.rpm
SRPM URL: file:///home/alapshin/Dropbox/Public/luajit.spec


[alapshin@localhost rpmbuild]$ rpmlint -i SPECS/luajit.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

[alapshin@localhost rpmbuild]$ rpmlint -i SRPMS/luajit-2.0.0-0.1.beta8.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 3 Andrei Lapshin 2011-07-04 18:12:48 UTC
Sorry, SRPM URL: http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.1.beta8.fc16.src.rpm

Comment 4 Andrei Lapshin 2011-07-05 07:17:51 UTC
Apply upstream hotfix #1

SPEC URL: http://dl.dropbox.com/u/11270386/luajit.spec
SRPM URL: http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.2.beta8.fc16.src.rpm

rpmlint -i SPECS/luajit.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint -i SRPMS/luajit-2.0.0-0.2.beta8.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 5 anish 2011-08-30 10:45:08 UTC
Here's an unofficial review for F15,

+ rpmlint output

rpmlint output is  silent 
[makerpm@dhcp201-181 SOURCES]$ rpmlint luajit-2.0.0-0.2.beta8.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
 
+ License mentioned in the spec file are "MIT  which looks okie

+ Sepc file is in American English 

+ Change log indicates the revised versions

Suggestions:-

1: According to packaging rules, please use naming conventions such as lua-luajit

Comment 6 john5342 2011-09-27 16:15:42 UTC
Just a few initial notes looking at the spec file:


The actual shared library (libluajit-5.1.so.2.*) is in the main package as it should be but libluajit-5.1.so should only be used for linking against and should therefore be in the devel package.


The buildroot tag is not one of the standard ones so far as i remember but either way it has not been needed for some time now and is ignored on our build system.


The hotfix patch (which is obviously upstream) should be provided as a full URL just like the source package.


%{optflags} must always be used. Adding CFLAGS="%{optflags}" to the build commandline should do the trick according to the in Makefile documentation.


All build output should be in verbose mode to aid in debugging failed builds as well as verifying that correct flags are used. I don't know of an easy way with these hand written Makefiles but it certainly should be investigated.


A quick build and rpmlint of the resulting output:
$ rpmlint luajit-*.rpm
luajit.x86_64: W: shared-lib-calls-exit /usr/lib64/libluajit-5.1.so.2.0.0 exit@GLIBC_2.2.5

Can probably be ignored since it is required to implement os.exit()

luajit.x86_64: W: no-manual-page-for-binary luajit-2.0.0-beta8

Not ideal but upstream don't provide one.

luajit-debuginfo.x86_64: E: empty-debuginfo-package

This is an error. Not sure if it is because it's failing to pick up the debug options in %{optflags} or something else but difficult to tell without knowing what commands are run.

luajit-devel.x86_64: W: no-documentation

devel requires the main package which does contain docs

luajit-static.x86_64: W: spelling-error %description -l en_US libluajit -> liberality

Ignore

luajit-static.x86_64: W: no-documentation

Same as above

5 packages and 0 specfiles checked; 1 errors, 5 warnings.



If these things are sorted i can probably do a proper review when i get some time.

Comment 7 john5342 2011-09-27 16:16:48 UTC
Apologies. I can't do the review. Didn't spot the FE-NEEDSPONSOR but the above points still apply.

Comment 8 Andrei Lapshin 2011-10-09 14:31:48 UTC
Created attachment 527095 [details]
Spec file diff

Comment 9 Andrei Lapshin 2011-10-09 14:32:41 UTC
Created attachment 527096 [details]
Latest spec file

Comment 10 Andrei Lapshin 2011-10-09 14:40:37 UTC
Latest SRPM http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.3.beta8.fc17.src.rpm

Comment 11 Andrei Lapshin 2011-10-09 14:48:28 UTC
Fixed errors pointed out by john5342.

Comment 12 Andrei Lapshin 2011-10-09 17:04:56 UTC
Created attachment 527109 [details]
Spec file diff with fixed typo

Comment 13 Andrei Lapshin 2011-10-09 17:06:07 UTC
Created attachment 527110 [details]
Latest spec file

Comment 14 Andrei Lapshin 2011-10-09 17:07:57 UTC
Created attachment 527111 [details]
Spec file diff from previous version

Comment 15 Andrei Lapshin 2011-10-09 17:10:55 UTC
Sorry, found typo in %build section. Fixed and rebuild. New SRPM available by same address http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.3.beta8.fc17.src.rpm

Comment 16 Scott Tsai 2011-12-14 00:18:38 UTC
Informal review. I also can't sponsor people in Fedora.

1. Specify URL of Patch0 like this:
- Patch0:         beta8_hotfix1.patch
+ Patch0:         http://luajit.org/download/beta8_hotfix1.patch
- Source1:        http://luajit.org/download/beta8_hotfix1.patch

2. Rename /usr/bin/luajit-2.0.0-beta8 to /usr/bin/luajit
The main command is only installed as:
 /usr/bin/luajit-2.0.0-beta8
having the 'luajit' binary name change from under the user as
we update to newer beta releases seems clearly undesirable.

Quoting from upstream http://luajit.org/install.html:
"to avoid overwriting a previous version,
the beta test releases only install the LuaJIT executable under the versioned name
(i.e. luajit-2.0.0-beta8). You probably want to create a symlink for convenience ..."

Since we're not packaging multiple versions of luajit in Fedora at this time,
I recommend you simply rename the binary as /usr/bin/luajit
This would also get rid of the "no-manual-page-for-binary" rpmlint warning. 

3. Make up your mind on whether to support older rpmb versions
If support for EPEL5 is desired, then the -devel package should require pkgconfig.
Otherwise we can rely on newer rpm features and remove BuildRoot and "rm -rf %{buildroot}" from the spec.

Other comments:
I ran the small collection of Lua 5.1 tests from http://testmore.luaforge.net/ and
they worked.

Regarding libluajit calling exit, I did a quick scan of the source and besides implementing
os.exit() luajit may also call exit() during exception handling errors (lj_err.c).
rpmlint complaining about shared libraries calling exit seems to be codifying a policy
that's not universally applicable in the wrong place anyway.

Comment 17 Andrei Lapshin 2012-02-06 16:53:03 UTC
Created attachment 559690 [details]
Latest spec file

Comment 18 Andrei Lapshin 2012-02-06 16:54:12 UTC
Created attachment 559691 [details]
Spec diff

Comment 20 Michael Schwendt 2012-03-24 16:47:51 UTC
A brief but hopefully helpful look at the spec file:


> %package devel
> Summary:        Development files for %{name}
> Group:          System Environment/Libraries

Library -devel packages typically are in group "Development/Libraries" whereas "System Environment/Libraries" is for the base library packages.

> Requires:       %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

> Requires:       pkgconfig

This can be removed because it is automatic. Take a look at the built rpms with "rpm -qpR ..."


> %package static
> Summary:        Static library for %{name}
> Group:          System Environment/Libraries
> Requires:       %{name} = %{version}-%{release}

Same here as above, plus: It makes no sense for the -static package to require the base package. If at all, it could require the -devel package.


> %post devel -p /sbin/ldconfig
>
> %postun devel -p /sbin/ldconfig

The are not needed for the -devel package. There is nothing in the -devel package that would be affected by running ldconfig. This is library base package stuff.


> %files
> %defattr(-,root,root,-)

This %defattr is the default and need not be specified anymore:
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


> %{_datadir}/%{name}-%{version}/jit/*
> %dir %{_datadir}/%{name}-%{version}
> %dir %{_datadir}/%{name}-%{version}/jit

Strange order of lines. Due to the '*' wildcard, you could reduce these three lines to just

  %{_datadir}/%{name}-%{version}/

to include that directory and everything in it properly.


> %dir %{_libdir}/lua
> %dir %{_libdir}/lua/5.1
> %dir %{_datadir}/lua
> %dir %{_datadir}/lua/5.1

Empty directories so far. Intentional? If so, a comment in the spec file would be appropriate.

Comment 21 M. Edward (Ed) Borasky 2012-09-07 05:17:00 UTC
I don't know if I'm quite ready to take over this package, but it's required by a package that I do want to use (LuaAV), so I'll certainly take a look at it.

Comment 22 Muayyad Alsadi 2013-09-02 06:40:50 UTC
I've updated .spec to the latest upstream and fixed the issues

.spec file

http://pastebin.com/raw.php?i=fqgu34Pv
https://docs.google.com/file/d/0B15QfS_FMZ1GcEs2X2xZSGE3RHc/edit?usp=sharing


the .src.rpm file

https://docs.google.com/file/d/0B15QfS_FMZ1GVWhlRFFwclRRakk/edit?usp=sharing

Comment 23 Muayyad Alsadi 2013-09-02 06:41:58 UTC
Created attachment 792723 [details]
luajit-2.0.2.spec

Comment 24 Zbigniew Jędrzejewski-Szmek 2013-10-06 14:05:34 UTC
(In reply to Muayyad Alsadi from comment #23)
> Created attachment 792723 [details]
> luajit-2.0.2.spec

Are you taking over this package? If so, is FE-NEEDSPONSOR still required?

Comment 25 Igor Gnatenko 2013-11-27 22:16:41 UTC
I want to get this package in Fedora.

Comment 26 Zbigniew Jędrzejewski-Szmek 2013-11-28 00:10:40 UTC
(In reply to Igor Gnatenko from comment #25)
> I want to get this package in Fedora.
That'd be nice. Looking at the lack of recent responses from other people on this bug, I think you should just take over the review request, and maybe add other people as co-maintainers if they ever get sponsored.

Comment 27 Igor Gnatenko 2013-11-28 09:30:07 UTC
I submitted new review. If anyone want to maintain - I can add as co-maintainer.

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


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