Bug 726210 - Review Request: freewrl - X3D / VRML visualization program
Summary: Review Request: freewrl - X3D / VRML visualization program
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-27 20:52 UTC by Tom "spot" Callaway
Modified: 2011-09-05 23:32 UTC (History)
4 users (show)

Fixed In Version: freewrl-1.22.12-0.3.pre2.el5.2
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-08-22 15:17:13 UTC
Type: ---
steve.traylen: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Tom "spot" Callaway 2011-07-27 20:52:28 UTC
Spec URL: http://spot.fedorapeople.org/freewrl.spec
SRPM URL: http://spot.fedorapeople.org/freewrl-1.22.12-0.1.pre2.fc15.src.rpm
Description: 
FreeWRL is an X3D / VRML visualization program. It can be used as a standalone
application or integrated as a browser plugin.

Koji Scratch Build (dist-f16): http://koji.fedoraproject.org/koji/taskinfo?taskID=3234564

Packager Notes:
rpmlint will throw:
freewrl.src:118: E: hardcoded-library-path in /usr/lib/jvm/java-openjdk/jre/lib/ext/vrml.jar

That is the correct path. There is no macro for the path. This package is "special" in how the Java bits work. :)

Comment 1 Tim Lauridsen 2011-08-09 06:03:22 UTC
I will review this package

Comment 2 Tim Lauridsen 2011-08-09 06:15:29 UTC
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.

Comment 3 Tim Lauridsen 2011-08-09 06:19:31 UTC
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.

Comment 4 Steve Traylen 2011-08-09 07:11:56 UTC
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.

Comment 5 Tom "spot" Callaway 2011-08-09 14:42:35 UTC
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

Comment 6 Tim Lauridsen 2011-08-09 16:58:37 UTC
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 :)

Comment 7 Steve Traylen 2011-08-09 18:28:23 UTC
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.

Comment 8 Tom "spot" Callaway 2011-08-10 15:30:52 UTC
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.

Comment 9 Steve Traylen 2011-08-11 09:10:45 UTC
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.

Comment 10 Tom "spot" Callaway 2011-08-11 18:18:51 UTC
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!

Comment 11 Tom "spot" Callaway 2011-08-11 18:20:55 UTC
New Package SCM Request
=======================
Package Name: freewrl
Short Description: X3D / VRML visualization program
Owners: spot
Branches: el5 el6 f14 f15 f16
InitialCC:

Comment 12 Gwyn Ciesla 2011-08-11 18:39:29 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2011-08-11 19:36:45 UTC
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

Comment 14 Fedora Update System 2011-08-11 19:36:55 UTC
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

Comment 15 Fedora Update System 2011-08-11 19:37:10 UTC
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

Comment 16 Fedora Update System 2011-08-12 04:21:13 UTC
freewrl-1.22.12-0.3.pre2.fc16 has been pushed to the Fedora 16 testing repository.

Comment 17 Steve Traylen 2011-08-14 09:38:37 UTC
Tom, 

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

Steve.

Comment 18 Tom "spot" Callaway 2011-08-16 14:46:06 UTC
Just request commit acl in pkgdb, and I'll grant it.

Comment 19 Fedora Update System 2011-08-17 10:00:18 UTC
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

Comment 20 Fedora Update System 2011-08-18 11:10:59 UTC
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

Comment 21 Fedora Update System 2011-08-22 15:17:08 UTC
freewrl-1.22.12-0.3.pre2.fc16 has been pushed to the Fedora 16 stable repository.

Comment 22 Fedora Update System 2011-08-23 04:33:17 UTC
freewrl-1.22.12-0.3.pre2.fc15 has been pushed to the Fedora 15 stable repository.

Comment 23 Fedora Update System 2011-08-23 04:34:11 UTC
freewrl-1.22.12-0.3.pre2.fc14 has been pushed to the Fedora 14 stable repository.

Comment 24 Fedora Update System 2011-09-05 23:31:21 UTC
freewrl-1.22.12-0.3.pre2.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 25 Fedora Update System 2011-09-05 23:32:04 UTC
freewrl-1.22.12-0.3.pre2.el5.2 has been pushed to the Fedora EPEL 5 stable repository.


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