Bug 718681

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

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.

Sorry, SRPM URL: http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.1.beta8.fc16.src.rpm

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.

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

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

Apologies. I can't do the review. Didn't spot the FE-NEEDSPONSOR but the above points still apply.

Created attachment 527095 [details]
Spec file diff

Created attachment 527096 [details]
Latest spec file

Latest SRPM http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.3.beta8.fc17.src.rpm

Fixed errors pointed out by john5342.

Created attachment 527109 [details]
Spec file diff with fixed typo

Created attachment 527110 [details]
Latest spec file

Created attachment 527111 [details]
Spec file diff from previous version

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

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.

Created attachment 559690 [details]
Latest spec file

Created attachment 559691 [details]
Spec diff

SRPM:  http://dl.dropbox.com/u/11270386/luajit-2.0.0-0.4.beta9.fc17.src.rpm

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.

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.

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

Created attachment 792723 [details]
luajit-2.0.2.spec

(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?

I want to get this package in Fedora.

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

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