Bug 726210

I will review this package

Package Review
==============

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

[x] : MUST - Package successfully compiles and builds into binary rpms on at least one supported architecture.
[x] : MUST - Fully versioned dependency in subpackages, if present.
[x] : MUST - Header files in -devel subpackage, if present.
[x] : MUST - Spec file lacks Packager, Vendor, PreReq tags.
[x] : MUST - ldconfig called in %post and %postun if required.
[x] : MUST - Package does not contain any libtool archives (.la)
[x] : MUST - Package use %makeinstall only when make install DESTDIR=... doesn't work.
[x] : MUST - Package is named according to the Package Naming Guidelines.
[x] : MUST - Spec file name must match the spec package %{name}, in the format %{name}.spec.
[-] : MUST - %config files are marked noreplace or the reason is justified.
[-] : MUST - The spec file handles locales properly.
[-] : MUST - No %config files under /usr.
[-] : MUST - Static libraries in -static subpackage, if present.
[!] : MUST - Rpmlint output is silent.
        
        rpmlint freewrl-java-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        freewrl-java.i686: W: only-non-binary-in-usr-lib
        1 packages and 0 specfiles checked; 0 errors, 1 warnings.
        ================================================================================
        
        rpmlint libEAI-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        libEAI.i686: W: shared-lib-calls-exit /usr/lib/libFreeWRLEAI.so.1.22.12 exit
        1 packages and 0 specfiles checked; 0 errors, 1 warnings.
        ================================================================================
        
        rpmlint freewrl-1.22.12-0.1.pre2.fc17.src.rpm
        ================================================================================
        freewrl.src:120: E: hardcoded-library-path in /usr/lib/jvm/java-openjdk/jre/lib/ext/vrml.jar
        1 packages and 0 specfiles checked; 1 errors, 0 warnings.
        ================================================================================
        
        rpmlint freewrl-debuginfo-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        1 packages and 0 specfiles checked; 0 errors, 0 warnings.
        ================================================================================
        
        rpmlint freewrl-devel-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        1 packages and 0 specfiles checked; 0 errors, 0 warnings.
        ================================================================================
        
        rpmlint libEAI-devel-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        libEAI-devel.i686: W: no-documentation
        1 packages and 0 specfiles checked; 0 errors, 1 warnings.
        ================================================================================
        
        rpmlint freewrl-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        freewrl.i686: W: no-manual-page-for-binary freewrl_msg
        freewrl.i686: W: no-manual-page-for-binary freewrl_snd
        1 packages and 0 specfiles checked; 0 errors, 2 warnings.
        ================================================================================
        
[!] : MUST - Development .so files in -devel subpackage, if present.
        freewrl-1.22.12-0.1.pre2.fc17.i686.rpm : /usr/lib/mozilla/plugins/libFreeWRLplugin.so
        
[!] : MUST - Sources used to build the package matches the upstream source, as provided in the spec URL.
        /home/tim/tmp/reviewhelper/726210/freewrl-1.22.12_pre2.tar.bz2 :
          MD5SUM this package     : e9baa64e551483dbfcb21e879fdf0d8e
          MD5SUM upstream package : 7530cf536beade1cfbc4400c5725818d
        README.FreeWRL.java :
          MD5SUM this package     : e56065d5fc275961e6058540786d05fd
          MD5SUM upstream package : upstream source not found
        
[x] : MUST - Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
[x] : MUST - %build honors applicable compiler flags or justifies otherwise.
[x] : MUST - All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
[x] : MUST - Package contains no bundled libraries.
[x] : MUST - Changelog in prescribed format.
[x] : MUST - Sources contain only permissible code or content.
[x] : MUST - Macros in Summary, %description expandable at SRPM build time.
[x] : MUST - Package contains a properly installed %{name}.desktop using desktop-file-install file if it is a GUI application.
[x] : MUST - Package requires other packages for directories it uses.
[x] : MUST - Package uses nothing in %doc for runtime.
[x] : MUST - Package is not known to require ExcludeArch.
[x] : MUST - Permissions on files are set properly.
[x] : MUST - Package does not contain duplicates in %files.
[x] : MUST - Large documentation files are in a -doc subpackage, if required.
[x] : MUST - If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
[x] : MUST - License field in the package spec file matches the actual license.
[!] : MUST - License file installed when any subpackage combination is installed.
    libEAI subpackage don't require main package, so license file is not pulled in.
[x] : MUST - Package consistently uses macros. instead of hard-coded directory names.
[x] : MUST - Package meets the Packaging Guidelines.
[x] : MUST - Package does not generates any conflict.
[x] : MUST - Package does not contains kernel modules.
[x] : MUST - Package contains no static executables.
[x] : MUST - Package obeys FHS, except libexecdir and /usr/target.
[x] : MUST - Package must own all directories that it creates.
[x] : MUST - Package does not own files or directories owned by other packages.
[x] : MUST - Package installs properly.
[x] : MUST - Rpath absent or only used for internal libs.
[x] : MUST - Package is not relocatable.
[x] : MUST - Requires correct, justified where necessary.
[x] : MUST - Spec file is legible and written in American English.
[-] : MUST - Package contains a SysV-style init script if in need of one.
[x] : MUST - File names are valid UTF-8.
[x] : MUST - Useful -debuginfo package or justification otherwise.
[x] : SHOULD - Reviewer should test that the package builds in mock.
[x] : SHOULD - Dist tag is present.
[x] : SHOULD - The placement of pkgconfig(.pc) files are correct.
[x] : SHOULD - SourceX is a working URL.
[x] : SHOULD - Spec use %global instead of %define.
[-] : SHOULD - Uses parallel make.
[!] : SHOULD - SourceX / PatchY prefixed with %{name}.
        Source1:	README.FreeWRL.java (README.FreeWRL.java)
        
[-] : SHOULD - If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[x] : SHOULD - No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x] : SHOULD - Final provides and requires are sane (rpm -q --provides and rpm -q --requires).
[?] : SHOULD - Package functions as described.
[x] : SHOULD - Latest version is packaged.
[x] : SHOULD - Package does not include license text files separate from upstream.
[!] : SHOULD - Man pages included for all executables.
[-] : SHOULD - Patches link to upstream bugs/comments/lists or are otherwise justified.
[x] : SHOULD - Scriptlets must be sane, if used.
[!] : SHOULD - Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
[x] : SHOULD - Package should compile and build into binary rpms on all supported architectures.
[!] : SHOULD - %check is present and all tests pass.
[x] : SHOULD - Packages should try to preserve timestamps of original installed files.

Issues:
[!] : MUST - Rpmlint output is silent.
        
        rpmlint freewrl-java-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        freewrl-java.i686: W: only-non-binary-in-usr-lib
        1 packages and 0 specfiles checked; 0 errors, 1 warnings.
        ================================================================================
        
        rpmlint libEAI-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        libEAI.i686: W: shared-lib-calls-exit /usr/lib/libFreeWRLEAI.so.1.22.12 exit
        1 packages and 0 specfiles checked; 0 errors, 1 warnings.
        ================================================================================
        I am not sure about this one
        
        
        rpmlint freewrl-1.22.12-0.1.pre2.fc17.src.rpm
        ================================================================================
        freewrl.src:120: E: hardcoded-library-path in /usr/lib/jvm/java-openjdk/jre/lib/ext/vrml.jar
        1 packages and 0 specfiles checked; 1 errors, 0 warnings.
        ================================================================================
        This is ok, based on your comments
        
        rpmlint freewrl-debuginfo-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        1 packages and 0 specfiles checked; 0 errors, 0 warnings.
        ================================================================================
        
        rpmlint freewrl-devel-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        1 packages and 0 specfiles checked; 0 errors, 0 warnings.
        ================================================================================
        
        rpmlint libEAI-devel-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        libEAI-devel.i686: W: no-documentation
        1 packages and 0 specfiles checked; 0 errors, 1 warnings.
        ================================================================================
        OK
        
        rpmlint freewrl-1.22.12-0.1.pre2.fc17.i686.rpm
        ================================================================================
        freewrl.i686: W: no-manual-page-for-binary freewrl_msg
        freewrl.i686: W: no-manual-page-for-binary freewrl_snd
        1 packages and 0 specfiles checked; 0 errors, 2 warnings.
        ================================================================================
        OK
        
[!] : MUST - Development .so files in -devel subpackage, if present.
        freewrl-1.22.12-0.1.pre2.fc17.i686.rpm : /usr/lib/mozilla/plugins/libFreeWRLplugin.so
        This is ok, it is a mozilla plugin
        
[!] : MUST - Sources used to build the package matches the upstream source, as provided in the spec URL.
        /home/tim/tmp/reviewhelper/726210/freewrl-1.22.12_pre2.tar.bz2 :
          MD5SUM this package     : e9baa64e551483dbfcb21e879fdf0d8e
          MD5SUM upstream package : 7530cf536beade1cfbc4400c5725818d
        README.FreeWRL.java :
          MD5SUM this package     : e56065d5fc275961e6058540786d05fd
          MD5SUM upstream package : upstream source not found
          
      THis need to be fixed.
      
[!] : MUST - License file installed when any subpackage combination is installed.
    libEAI subpackage don't require main package, so license file is not pulled in.
      I think requiring the main package is needed here.

Hi Tim,
 I had already taken for review? Not to worry, here is my review I did last night I did match
 for the md5sum.

Here's my text anyway, I just planned to check it vaguely worked as well:


Package Review
==============
freewrl - https://bugzilla.redhat.com/show_bug.cgi?id=726210
8th August 2011
Builds okay in mock f16.

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

=== REQUIRED ITEMS ===
[x]  Package is named according to the Package Naming Guidelines.
Based on tar ball name, and a pre-release.
[x]  Spec file name must match the base package %{name}, in the format %{name}.spec.
[!]  Package meets the Packaging Guidelines.
I thought BuildRequires should now have _isa tag, see below.
[x]  Package successfully compiles and builds into binary rpms on at least one supported architecture.
Tested on:
[x]  Rpmlint output:

[x]  Package is not relocatable.
[x]  Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
No builidroot required these days.
[x]  Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
LGPLv3+ 
[!]  License field in the package spec file matches the actual license.
Font files , see below.
[x]  If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
[x]  Spec file is legible and written in American English.
[x]  Sources used to build the package matches the upstream source, as provided in the spec URL.
$ md5sum freewrl-1.22.12_pre2.tar.bz2 ../SOURCES/freewrl-1.22.12_pre2.tar.bz2 
e9baa64e551483dbfcb21e879fdf0d8e  freewrl-1.22.12_pre2.tar.bz2
e9baa64e551483dbfcb21e879fdf0d8e  ../SOURCES/freewrl-1.22.12_pre2.tar.bz2
[x]  Package is not known to require ExcludeArch, OR:
[x]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
But see comment below about %{_isa}
[-]  The spec file handles locales properly.
[x]  ldconfig called in %post and %postun if required.
[x]  Package must own all directories that it creates.
[x]  Package requires other packages for directories it uses.
[x]  Package does not contain duplicates in %files.
[x]  Permissions on files are set properly.
[x]  Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
Not needed anymore.
[x]  Package consistently uses macros.
[-]  Large documentation files are in a -doc subpackage, if required.
[x]  Package uses nothing in %doc for runtime.
[x]  Header files in -devel subpackage, if present.
[-]  Static libraries in -devel subpackage, if present.
[x]  Package requires pkgconfig, if .pc files are present.
But see comment below.
[x]  Development .so files in -devel subpackage, if present.
There's a mozilla plugin so fine.
[x]  Fully versioned dependency in subpackages, if present.
[x]  Package does not contain any libtool archives (.la).
[x]  Package contains a properly installed %{name}.desktop file if it is a GUI application.
[x]  Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
[x]  Latest version is packaged.
[x]  Package does not include license text files separate from upstream.
[x]  Reviewer should test that the package builds in mock.
Tested on:
[x]  Package should compile and build into binary rpms on all supported architectures.
Tested on:
[-]  Package functions as described.
[x]  Scriptlets must be sane, if used.
[x]  The placement of pkgconfig(.pc) files are correct.
[x]  File based requires are sane.


=== Issues ===
1. I thought e.g.
   BuildRequires:  zlib-devel
   should now be zlib-devel%{?_isa} but now looking at the guidelines I
   look to be wrong.

2. appleOSX/OSX_Specific/fonts cotains fonts and a license but
   you are the expert on this.
   If they are not actually used in the build and distributed it's 
   more obvious if you can rm -rf them in %prep.

3. Requires: pkgconfig, this is normally auto determined anyway for
   new systems so is probably not needed.

I'm not sure why Tim is getting sum mismatches on the source tarball, I've downloaded several times now and I get e9baa64e551483dbfcb21e879fdf0d8e each time, and it looks like Steve does too. As to the other file, there is no upstream source because I generated the README.FreeWRL.Java.

shared-lib-calls-exit is just sloppy code, but it isn't a blocker, IMHO.

Dropped the Requires: pkgconfig, it is no longer necessary.

Deleted the appleOSX/ dir in %prep.

libEAI-devel requires libEAI, and libEAI contains COPYING and COPYING.LESSER, so there is no license need to require the freewrl package.

New SPEC: http://spot.fedorapeople.org/freewrl.spec
New SRPM: http://spot.fedorapeople.org/freewrl-1.22.12-0.2.pre2.fc15.src.rpm

Sorry Steve, I missed you have taken the review. Please assign it to yourself :)

The md5sum issue must be something with my review helper script. So I have no outstanding issues :)

I found the package I started on back in November.
I should have looked at the sooner in this review to see what the
problem was.

http://cern.ch/straylen/rpms/freewrl/

If I remember correctly it was failing at that time due to 
it needing to be ported to xulrunner2 which has now been done.

For the rpath it looks like I used the sed on libtool trick which
is preferential to the chrpath which is last resort.

I also split the firefox plugin to a separate package to
avoid firefox as a dependency on the cmdline tool,
completely up to you if you want to do this, it makes sense to me.

Running your package on Fedora 15 (via an ssh -X session)

$  wget http://cic.nist.gov/vrml/nistlogo.wrl
$ freewrl ./nistlogo.wrl
opengl version=1.4 (2.1.2 NVIDIA 270.41.06)
FreeWRL got a SIGSEGV - can you please mail the file(s) to
 freewrl-09 with a valid subject line. Thanks.

which I now remember was a problem I had.

After 

# yum install bitstream-vera-sans-fonts bitstream-vera-sans-mono-fonts

which I required for some reason it still fails run so it's not just that.
Steve.

Splitting out the browser plugin into its own subpackage makes sense. Not sure why I didn't do that from the beginning.

New SRPM: http://spot.fedorapeople.org/freewrl-1.22.12-0.3.pre2.fc15.src.rpm
New SPEC: http://spot.fedorapeople.org/freewrl.spec

As to your segfault, I cannot reproduce it on my system using only FOSS drivers:

[spot@pterodactyl master]$ freewrl http://cic.nist.gov/vrml/nistlogo.wrl
opengl version=2.1 Mesa 7.11-devel
--2011-08-10 10:50:44--  http://cic.nist.gov/vrml/nistlogo.wrl
Resolving cic.nist.gov... 129.6.13.45
Connecting to cic.nist.gov|129.6.13.45|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 9767 (9.5K) [model/vrml]
Saving to: “/tmp/freewAyqW7D”

100%[=====================================================================================================================================>] 9,767       --.-K/s   in 0.1s    

2011-08-10 10:50:44 (65.0 KB/s) - “/tmp/freewAyqW7D” saved [9767/9767]

I suspect strongly that your crash is -ENVIDIA.

Indeed I was on NVIDEA, does not work from a Mac either but it's fine on an intel card.

You did not switch from chrpath to a sed of libtool and

http://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath

says to use chrpath as a last resort. Having said that the end result
is good so I leave to you.

APPROVED.

I'd like to see these in EPEL (5 and 6) as well and am happy to maintain them if you do not want
to do this? Let me know.

Steve.

I'm not really interested in EPEL for this one, but I'll ask for the branches and you can maintain them. Thanks for the review!

New Package SCM Request
=======================
Package Name: freewrl
Short Description: X3D / VRML visualization program
Owners: spot
Branches: el5 el6 f14 f15 f16
InitialCC:

Git done (by process-git-requests).

freewrl-1.22.12-0.3.pre2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/freewrl-1.22.12-0.3.pre2.fc15

freewrl-1.22.12-0.3.pre2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/freewrl-1.22.12-0.3.pre2.fc16

freewrl-1.22.12-0.3.pre2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/freewrl-1.22.12-0.3.pre2.fc14

freewrl-1.22.12-0.3.pre2.fc16 has been pushed to the Fedora 16 testing repository.

Tom, 

If you can release ownership of the EPEL branches I'll take them.

Steve.

Just request commit acl in pkgdb, and I'll grant it.

freewrl-1.22.12-0.3.pre2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/freewrl-1.22.12-0.3.pre2.el6

freewrl-1.22.12-0.3.pre2.el5.2 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/freewrl-1.22.12-0.3.pre2.el5.2

freewrl-1.22.12-0.3.pre2.fc16 has been pushed to the Fedora 16 stable repository.

freewrl-1.22.12-0.3.pre2.fc15 has been pushed to the Fedora 15 stable repository.

freewrl-1.22.12-0.3.pre2.fc14 has been pushed to the Fedora 14 stable repository.

freewrl-1.22.12-0.3.pre2.el6 has been pushed to the Fedora EPEL 6 stable repository.

freewrl-1.22.12-0.3.pre2.el5.2 has been pushed to the Fedora EPEL 5 stable repository.