Bug 460779 - Review Request: nekovm - Neko embedded scripting language and virtual machine
Review Request: nekovm - Neko embedded scripting language and virtual machine
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 460780
  Show dependency treegraph
 
Reported: 2008-08-31 15:05 EDT by Richard W.M. Jones
Modified: 2009-01-14 21:54 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-14 21:54:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dan: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
update set-soname patch (1.17 KB, patch)
2008-12-17 05:51 EST, Dan Horák
no flags Details | Diff
create -libs subpackage to be multilib compliant (1.67 KB, patch)
2008-12-17 05:53 EST, Dan Horák
no flags Details | Diff

  None (edit)
Description Richard W.M. Jones 2008-08-31 15:05:18 EDT
Spec URL: http://www.annexia.org/tmp/ocaml/nekovm.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/nekovm-1.7.1-2.fc10.src.rpm
Description: Neko embedded scripting language and virtual machine

(This isn't really an OCaml package .. It's pure C, but used by another OCaml package
called haXe which is coming along in a minute).

rpmlint says:
  nekovm.i386: W: no-soname /usr/lib/libneko.so
Comment 1 Richard W.M. Jones 2008-09-01 18:51:30 EDT
Spec URL: http://www.annexia.org/tmp/ocaml/nekovm.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/nekovm-1.7.1-3.fc10.src.rpm

Better method of stopping rpm from stripping the binaries.
Comment 2 Dan Horák 2008-09-02 03:52:41 EDT
- build in Rawhide/x86_64 fails (http://koji.fedoraproject.org/koji/taskinfo?taskID=799312) - maybe BR: sqlite-devel is missing?
- Fedora's CFLAGS are not used, could be solved with using "make CFLAGS="$RPM_OPT_FLAGS -I vm -D_GNU_SOURCE"" for building
- what about 64-bit compatibility - I see "CFLAGS += -D_64BITS" in the Makefile?
Comment 3 Richard W.M. Jones 2008-09-02 08:38:36 EDT
Yes, it was rather broken on 64 bit.  This seems to resolve the
issues, and I've also built it in Koji so the dependency / 64-bit
problems should be fixed.

Spec URL: http://www.annexia.org/tmp/ocaml/nekovm.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/nekovm-1.7.1-6.fc10.src.rpm

* Tue Sep  2 2008 Richard W.M. Jones <rjones@redhat.com> - 1.7.1-6
- BR sqlite-devel
- Remove DOS line-endings and executable bits from the source.
- Add RPM CFLAGS.
- *.ndll files are always installed in /usr/lib/neko (even on 64 bit).
- Search /usr/lib64 directory for libraries when building.
- When building, link against libmysqlclient.so (not .a).
- Avoid a compiler stack overflow when building on 64 bit.
- Stop prelink from stripping the binaries.

http://koji.fedoraproject.org/koji/taskinfo?taskID=799546
Comment 4 Dan Horák 2008-09-02 12:14:09 EDT
formal review is here, see notes at the end

OK	source files match upstream:
	    cf2d1ebcb483fe88abb263fe276015f710a751bb  neko-1.7.1.tar.gz
OK	package meets naming and versioning guidelines.
OK	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	build root is correct.
OK	license field matches the actual license.
OK	license is open source-compatible. License text included in package.
OK	latest version is being packaged.
OK	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	%clean is present.
OK	package builds in mock (Rawhide/x86_64).
OK	debuginfo package looks complete.
??	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
BAD	no shared libraries are added to the regular linker search paths.
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
BAD	no scriptlets present.
OK	code, not content.
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	headers in -devel.
OK	no pkgconfig files.
OK	no libtool .la droppings.
OK	not a GUI app.

- use --keepdate when running dos2unix to preserve the timestamps for the files
- shared library is added, but ldconfig scriptlets are missing
- rpmlint complains
nekovm.src:102: E: hardcoded-library-path in %{_prefix}/lib/neko
nekovm.src:109: E: hardcoded-library-path in %{_prefix}/lib
nekovm.src:110: E: hardcoded-library-path in %{_prefix}/lib/*.so
nekovm.src:127: E: hardcoded-library-path in %{_prefix}/lib/neko/
 - I think these will create a multilib issue together with shared libneko.so in the main package
 - what is the purpose of the *.ndll? runtime libs for nekovm based apps?

nekovm.x86_64: W: no-soname /usr/lib64/libneko.so
 - result of not-so-right linking command

I think you need to split the package to
main - %{_bindir} + /usr/lib/neko
libs - %{_libdir}
devel - %{_includedir}

- I think that more appropriate Group for the main package is Development/Languages, the -libs should go into System Environment/Libraries and -devel should be in Development/Libraries
Comment 5 Dan Horák 2008-12-08 09:55:41 EST
ping?
Comment 6 Richard W.M. Jones 2008-12-08 12:50:40 EST
pong?  Sorry, I need to look at this.  Set to NEEDINFO of me.
Comment 7 Richard W.M. Jones 2008-12-16 10:28:43 EST
(In reply to comment #4)
> BAD no shared libraries are added to the regular linker search paths.
> BAD no scriptlets present.

This is fixed now so that the shared library has
an SONAME, and we run ldconfig in the %post/%postun
scriptlets.

> - use --keepdate when running dos2unix to preserve the timestamps for the files

Done.

> - shared library is added, but ldconfig scriptlets are missing

Done.

> - rpmlint complains

rpmlint now says:

nekovm.src:114: E: hardcoded-library-path in %{_prefix}/lib
nekovm.src:121: E: hardcoded-library-path in %{_prefix}/lib
nekovm.src:122: E: hardcoded-library-path in %{_prefix}/lib/*
nekovm.src:123: E: hardcoded-library-path in %{_prefix}/lib
3 packages and 0 specfiles checked; 4 errors, 0 warnings.

But in this case it's just complaining about my
install script which is moving the files FROM
%{_prefix}/lib to %{_libdir}.  I think this is
rpmlint getting it wrong.

>  - I think these will create a multilib issue together with shared libneko.so
> in the main package

Fixed so now it uses %{_libdir} always.

>  - what is the purpose of the *.ndll? runtime libs for nekovm based apps?

These are some type of runtime shared library
containing neko VM code.

> nekovm.x86_64: W: no-soname /usr/lib64/libneko.so
>  - result of not-so-right linking command

This is now fixed.

> I think you need to split the package to
> main - %{_bindir} + /usr/lib/neko
> libs - %{_libdir}
> devel - %{_includedir}

I don't think this split will work.  The reason is
that the main package will always require the -libs
package, because it always needs (at least) std.ndll.

This is the updated package:

Spec URL: http://www.annexia.org/tmp/ocaml/nekovm.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/nekovm-1.8.0-2.fc10.src.rpm

* Tue Dec 16 2008 Richard W.M. Jones <rjones@redhat.com> - 1.8.0-2
- New upstream release 1.8.0.
- Use dos2unix --keepdate.
- Use scriptlets to run ldconfig.
- Use _libdir instead of _prefix/lib, and modify so it searches
  /usr/lib64 as well as /usr/lib.
- Set the soname correctly and include libneko.so.1.
Comment 8 Dan Horák 2008-12-16 12:06:47 EST
So all issues should be fixed, but there will be multilib/multiarch problems due the existence of the devel subpackage. I tend to agree with you that there is no simple solution, so you should ask rel-eng to add nekovm to the list of multilib incompatible packages. And/or try to discuss the issue on the fedora-devel mailing list.

But anyway, this package is APPROVED.
Comment 9 Richard W.M. Jones 2008-12-16 12:10:19 EST
Thanks for approving this.

Are you sure there are still multilib issues remaining?
The new package (1.8.0-2) should only install packages
in %{_libdir}.  Can this still cause a conflict?
Comment 10 Richard W.M. Jones 2008-12-16 12:11:28 EST
New Package CVS Request
=======================
Package Name: nekovm
Short Description: Neko embedded scripting language and virtual machine
Owners: rjones
Branches: F-10
InitialCC:
Comment 11 Dan Horák 2008-12-16 12:20:52 EST
(In reply to comment #9)
> Thanks for approving this.
> 
> Are you sure there are still multilib issues remaining?
> The new package (1.8.0-2) should only install packages
> in %{_libdir}.  Can this still cause a conflict?

The binaries from %{_bindir} will differ and thus will block parallel installation of the nekovm packages.
Comment 12 Dan Horák 2008-12-17 04:47:02 EST
I still think it can be split to be multilib compliant
main - %{_bindir} + %{_libdir}/neko/
-libs - %{_libdir}/libneko.so.*
-devel - %{_includedir} + %{_libdir}/libneko.so

both main and -devel will Require -libs
Comment 13 Dan Horák 2008-12-17 05:51:31 EST
Created attachment 327224 [details]
update set-soname patch

- make libneko.so.1 real file and libneko.so symlink
- use "cp -d" when copying the libs otherwise (plain "cp") the symlink was dereferenced to the real file
Comment 14 Dan Horák 2008-12-17 05:53:18 EST
Created attachment 327225 [details]
create -libs subpackage to be multilib compliant
Comment 15 Kevin Fenzi 2008-12-17 16:52:57 EST
cvs done.
Comment 16 Fedora Update System 2008-12-18 05:38:50 EST
nekovm-1.8.0-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/nekovm-1.8.0-2.fc10
Comment 17 Richard W.M. Jones 2008-12-18 05:44:09 EST
I've created a new bug for multilib support: bug 476962
Comment 18 Fedora Update System 2008-12-21 03:21:53 EST
nekovm-1.8.0-2.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nekovm'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-11575
Comment 19 Fedora Update System 2009-01-14 21:54:25 EST
nekovm-1.8.0-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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