*** Bug 718681 has been marked as a duplicate of this bug. ***

Do you mind me to take over this review?

No ;) Do this!

(In reply to Christopher Meng from comment #2)
> Do you mind me to take over this review?

(In reply to Igor Gnatenko from comment #3)
> No ;) Do this!

Just to make things really clear - Igor would like you, Christopher, to review this.

new SPEC: http://ignatenkobrain.fedorapeople.org/for-review/luajit.spec
new SRPM: http://ignatenkobrain.fedorapeople.org/for-review/luajit-2.0.2-3.fc20.src.rpm

1. INSTALL_X= install -m 0755
INSTALL_F= install -m 0644

-------

-p option should be inserted also.

2. Package is fine.

Only 1 question:

Why do this?

mv doc/ html/

(In reply to Christopher Meng from comment #6)
> 1. INSTALL_X= install -m 0755
> INSTALL_F= install -m 0644
> 
> -------
> 
> -p option should be inserted also.
Fixed. One minute for new spec/srpm
> 2. Package is fine.
> 
> Only 1 question:
> 
> Why do this?
> 
> mv doc/ html/
because this folder contains html docs. I think html folder instead of doc would be good

new SPEC: http://ignatenkobrain.fedorapeople.org/for-review/luajit.spec
new SRPM: http://ignatenkobrain.fedorapeople.org/for-review/luajit-2.0.2-4.fc20.src.rpm

Don't forget set fedora-review flag ;)

> mv doc/ html/

Please don't do that. It modifies the build dir contents and breaks short-circuit builds. Here repeated calls of "rpmbuild --short-circuit -bi luajit.spec". Even if you don't use any short-circuit builds, other packages do, so please don't break them.

Solution: Create a temporary copy of the dir.

  …

  rm -rf _tmp_html ; mkdir _tmp_html
  cp -a doc _tmp_html/html

  …

  %files devel
  %doc _tmp_html/html/

  …

s/other packages/other packagers/

(In reply to Michael Schwendt from comment #9)
> > mv doc/ html/
> 
> Please don't do that. It modifies the build dir contents and breaks
> short-circuit builds. Here repeated calls of "rpmbuild --short-circuit -bi
> luajit.spec". Even if you don't use any short-circuit builds, other packages
> do, so please don't break them.
> 
> Solution: Create a temporary copy of the dir.
> 
>   …
> 
>   rm -rf _tmp_html ; mkdir _tmp_html
>   cp -a doc _tmp_html/html
> 
>   …
> 
>   %files devel
>   %doc _tmp_html/html/
> 
>   …

Thank you! Fixed.

New SPEC: http://ignatenkobrain.fedorapeople.org/for-review/luajit.spec
New SRPM: http://ignatenkobrain.fedorapeople.org/for-review/luajit-2.0.2-5.fc20.src.rpm

Scratch build http://koji.fedoraproject.org/koji/taskinfo?taskID=6246247

Hmm..

.pc on x86_64 RPM attached:

-------------------------

# Package information for LuaJIT to be used by pkg-config.
majver=2
minver=0
relver=2
version=${majver}.${minver}.${relver}
abiver=5.1

prefix=/usr
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
libname=luajit-${abiver}
includedir=${prefix}/include/luajit-${majver}.${minver}

INSTALL_LMOD=${prefix}/share/lua/${abiver}
INSTALL_CMOD=/usr/lib64/lua/${abiver}

Name: LuaJIT
Description: Just-in-time compiler for Lua
URL: http://luajit.org
Version: ${version}
Requires:
Libs: -L${libdir} -l${libname}
Libs.private: -Wl,-E -lm -ldl
Cflags: -I${includedir}
-------------------------

So I can't approve it now. Problem found!

libdir=${exec_prefix}/lib should be libdir=${exec_prefix}/lib64 on x86_64.

But, you can look at VCS repo of luajit, it now has supported lib64 by $(MULTILIB) define.

http://repo.or.cz/w/luajit-2.0.git/blob/HEAD:/Makefile
http://repo.or.cz/w/luajit-2.0.git/blob/HEAD:/etc/luajit.pc

Please package 2.1(You should obey the snapshot package guideline), or add patch to fix that.

Have you watched .spec ?

# fix .pc (besser82)
sed -i -e 's!${prefix}/lib!%{_libdir}!g' etc/luajit.pc

Yes, I watched it. But the pasted content were got from the koji build 6246247 for x86_64 -devel package.

Your sed command changed this on lib64 system from:
INSTALL_CMOD=${prefix}/lib/lua/${abiver}
to
INSTALL_CMOD=/usr/lib64/lua/${abiver}

But I still see:
libdir=${exec_prefix}/lib

Don't you think it should be libdir=${exec_prefix}/lib64?

ah. sorry. I was frozen... Fixed.

New SPEC: http://ignatenkobrain.fedorapeople.org/for-review/luajit.spec
New SRPM: http://ignatenkobrain.fedorapeople.org/for-review/luajit-2.0.2-6.fc20.src.rpm

PACKAGE APPROVED


-------------------

sed -i -e 's!${prefix}/lib!%{_libdir}!g' etc/luajit.pc
sed -i -e 's!${exec_prefix}/lib!%{_libdir}!g' etc/luajit.pc

Can just be:

sed -i -e 's!${.*prefix}/lib!%{_libdir}!g' etc/luajit.pc

;)

Christopher, thank you for review ! ;)

New Package SCM Request
=======================
Package Name: luajit
Short Description: Just-In-Time Compiler for Lua
Owners: ignatenkobrain
Branches: f19 f20

Git done (by process-git-requests).

luajit-2.0.2-6.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/luajit-2.0.2-6.fc20

luajit-2.0.2-6.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/luajit-2.0.2-6.fc19

luajit-2.0.2-6.fc19 has been pushed to the Fedora 19 testing repository.

Please read the bodhi comments at

https://admin.fedoraproject.org/updates/FEDORA-2013-22732/luajit-2.0.2-6.fc20 ,

I just tested, this package doesn't work.

/usr/bin/luajit -> luajit which the latter one doesn't exist.

Created attachment 834456 [details]
Don't strip binaries

The sed line that tries to disable stripping doesn't actually work, here's a more robust alternative that does.

Thank you! Fxd.

http://pkgs.fedoraproject.org/cgit/luajit.git/commit/?id=f05c145cb149f6aa507c7c6c30edd203316dd022

luajit-2.0.2-8.fc19 has been pushed to the Fedora 19 stable repository.

luajit-2.0.2-8.fc20 has been pushed to the Fedora 20 stable repository.