Bug 212715

I know Braden's work on openvrml for a long time and had invited him to
contribute this package to FE some time ago. Therefore I am going to review this
package and am willing to sponsor him.

Basically, the spec is in good shape for an initial submission. Nevertheless, it
needs some work:

Let's start with what rpmlint has to say:

1)
W: openvrml-debuginfo no-version-in-last-changelog
W: openvrml-devel no-version-in-last-changelog
W: openvrml-gl no-version-in-last-changelog
W: openvrml-gl-devel no-version-in-last-changelog
W: openvrml-gtkplug no-version-in-last-changelog
W: openvrml-mozilla-plugin no-version-in-last-changelog

Please add them.


2)
W: openvrml-devel no-version-dependency-on openvrml 0.16.1
openvrml-devel lacks
Requires: openvrml = %{version}-%{release}


3)
W: openvrml-gl-devel summary-ended-with-dot OpenVRML OpenGL renderer headers and
static library.
W: openvrml-gtkplug summary-ended-with-dot VRML browser GtkPlug component for
embedding in GTK+ applications.
W: openvrml-mozilla-plugin summary-ended-with-dot VRML browser plug-in for
Mozilla-based browsers.
Please remove the '.' at the end of the corresponding Summary: lines.


4)
E: openvrml-gl library-without-ldconfig-postin /usr/lib/libopenvrml-gl.so.6.0.3
E: openvrml-gl library-without-ldconfig-postun /usr/lib/libopenvrml-gl.so.6.0.3
Please add ldconfig calls, analogous to what you apply to the main package to
openvrml-gl


5)
E: openvrml-gtkplug info-files-without-install-info-postin
/usr/share/info/openvrml-gtkplug.info.gz
E: openvrml-gtkplug info-files-without-install-info-postun
/usr/share/info/openvrml-gtkplug.info.gz
You need to add appropriate calls to /usr/sbin/install-info to openvrml-gtkplug


5)
E: openvrml-gtkplug explicit-lib-dependency libXmu
This requirement is redundant and should be removed (The dependency on libXmu is
already being picked up by rpm).

In the same boat is this package's "Requires: gtk2". 
It also probably is redundant.


5) Any particular reason you explicitly "Requires: firefox" at several places?
I thought mozilla-plugins being installed into .../mozilla/plugins were supposed
to be usable by all mozilla-compatible browsers (mozilla, firefox, seamonkey,
etc.), so an explicit "Requires: firefox" seems unnecessarily restrictive to me.


6) Please "Requires: libGLU-devel" instead of "mesa-libGLU" in *-devel packages.


7) Please use "%{version}-%{release}"-style dependencies instead of "%{version}"
 dependencies inside of your package.

(In reply to comment #2)

> 6) Please "Requires: libGLU-devel" instead of "mesa-libGLU" in *-devel packages.

Oops, of cause I meant to write:
... "Requires: libGLU-devel" instead of "mesa-libGLU-devel" ...

(In reply to comment #2)
> Basically, the spec is in good shape for an initial submission. Nevertheless, it
> needs some work:
> 
> Let's start with what rpmlint has to say:
> 
> 1)
> W: openvrml-debuginfo no-version-in-last-changelog
> W: openvrml-devel no-version-in-last-changelog
> W: openvrml-gl no-version-in-last-changelog
> W: openvrml-gl-devel no-version-in-last-changelog
> W: openvrml-gtkplug no-version-in-last-changelog
> W: openvrml-mozilla-plugin no-version-in-last-changelog
> 
> Please add them.

Done, I think.

> 2)
> W: openvrml-devel no-version-dependency-on openvrml 0.16.1
> openvrml-devel lacks
> Requires: openvrml = %{version}-%{release}

Check.

> 3)
> W: openvrml-gl-devel summary-ended-with-dot OpenVRML OpenGL renderer headers and
> static library.
> W: openvrml-gtkplug summary-ended-with-dot VRML browser GtkPlug component for
> embedding in GTK+ applications.
> W: openvrml-mozilla-plugin summary-ended-with-dot VRML browser plug-in for
> Mozilla-based browsers.
> Please remove the '.' at the end of the corresponding Summary: lines.

Check.

> 4)
> E: openvrml-gl library-without-ldconfig-postin /usr/lib/libopenvrml-gl.so.6.0.3
> E: openvrml-gl library-without-ldconfig-postun /usr/lib/libopenvrml-gl.so.6.0.3
> Please add ldconfig calls, analogous to what you apply to the main package to
> openvrml-gl

Check.

> 5)
> E: openvrml-gtkplug info-files-without-install-info-postin
> /usr/share/info/openvrml-gtkplug.info.gz
> E: openvrml-gtkplug info-files-without-install-info-postun
> /usr/share/info/openvrml-gtkplug.info.gz
> You need to add appropriate calls to /usr/sbin/install-info to openvrml-gtkplug

Fixed, I think.

> 5)
> E: openvrml-gtkplug explicit-lib-dependency libXmu
> This requirement is redundant and should be removed (The dependency on libXmu is
> already being picked up by rpm).
> 
> In the same boat is this package's "Requires: gtk2". 
> It also probably is redundant.

Okay.

> 5) Any particular reason you explicitly "Requires: firefox" at several places?
> I thought mozilla-plugins being installed into .../mozilla/plugins were supposed
> to be usable by all mozilla-compatible browsers (mozilla, firefox, seamonkey,
> etc.), so an explicit "Requires: firefox" seems unnecessarily restrictive to me.

For openvrml, it's because libopenvrml links with libmozjs. Hopefully rpm picks
this up and this one isn't necessary.

For openvrml-mozilla-plugin, it's a holdover since the plug-in used to use
XPCOM. It doesn't anymore, so hopefully it's unnecessary, too.

> 6) Please "Requires: libGLU-devel" instead of "mesa-libGLU" in *-devel packages.

Okay. How come?

> 7) Please use "%{version}-%{release}"-style dependencies instead of "%{version}"
>  dependencies inside of your package.

Check.

spec: http://endoframe.com/openvrml.spec
srpm: http://endoframe.com/openvrml-0.16.1-2.src.rpm

Sorry for not having responded earlier, but ... I couldn't manage to look into
this earlier.

There still are some issues needing to be addressed:

* Please '%%' instead of '%' in references to rpm-tags in %changelog entries.
This is to work around rpm bogusly expanding '%' in %changelogs

* Please add a blank line at the end of each %post*/%pre block. Some versions of
rpm suffer from a bug which bogusly concatenate %post*/%pre otherwise.

* openvrml-gl-devel and openvrml-devel contain *.pc's
=>  each of them must
Requires: pkgconfig

* Building in mock fails:
...
error: Installed (but unpackaged) file(s) found:
   /usr/share/info/dir
=> Add 
rm -f %{_infodir}/dir
near to the end of %install

I'm interested in having this in Extras, too. A few comments:
1. redundant BuildRequires: pkgconfig (required by freetype-devel and
libpng-devel), libpng-devel (required by cairo-devel, which is required by
gtk2-devel), fontconfig-devel (required by cairo-devel), freetype-devel
(required by fontconfig-devel)
2. use BuildRequires: gecko-devel instead of firefox-devel and libGLU-devel
instead of mesa-libGLU-devel
3. reduntant Requires: mesa-libGLU for -gl subpackage
4. use %setup -q instead of %setup
5. use %configure --disable-dependency-tracking to speed up build process
6. use %{__make} instead of make in %install for consistency
7. avoid unnecessary docs duplication in every subpackage
8. rename mozilla-plugin to plugin

BTW: if it's Assigned to you, Ralf, why is the status still NEW?

Judging by the contents of the .pc file, I suggest adding the following for -devel:
Requires:       freetype-devel                                                 
                                        
Requires:       libjpeg-devel                                                  
                                        
Requires:       libpng-devel

(In reply to comment #6)
> I'm interested in having this in Extras, too. A few comments:
> 1. redundant BuildRequires: pkgconfig (required by freetype-devel and
> libpng-devel), libpng-devel (required by cairo-devel, which is required by
> gtk2-devel), fontconfig-devel (required by cairo-devel), freetype-devel
> (required by fontconfig-devel)

Okay. Presumably making openvrml-devel Require freetype-devel would moot Ralf's 
suggestion that openvrml-devel should Require pkgconfig.

> 2. use BuildRequires: gecko-devel instead of firefox-devel and libGLU-devel
> instead of mesa-libGLU-devel

Re: gecko-devel, I don't think I can do that. I think the change that does need 
to be made is for openvrml to Require firefox = 1.5.0.8. Note that libopenvrml 
links with libmozjs, which lives in a versioned subdirectory. Consequently, 
openvrml-devel would Require firefox-devel = 1.5.0.8.

> 3. reduntant Requires: mesa-libGLU for -gl subpackage

Why is that redundant?

> 4. use %setup -q instead of %setup
> 5. use %configure --disable-dependency-tracking to speed up build process
> 6. use %{__make} instead of make in %install for consistency

Okay.

> 7. avoid unnecessary docs duplication in every subpackage

Users shouldn't need to be aware of the package dependency chain in order to 
find the README or license for the package. Would it be acceptable to install 
symlinks?

> 8. rename mozilla-plugin to plugin

No. There are lots of kinds of plug-ins. This package makes a Mozilla/Gecko 
one. I might entertain changing it to "gecko-plugin"; but since "Mozilla plug-
in" or "Netscape plug-in" is what everyone calls these things, that's going to 
be a tough sell.

(In reply to comment #9)
> (In reply to comment #6)
> > I'm interested in having this in Extras, too. A few comments:
> > 1. redundant BuildRequires: pkgconfig (required by freetype-devel and
> > libpng-devel), libpng-devel (required by cairo-devel, which is required by
> > gtk2-devel), fontconfig-devel (required by cairo-devel), freetype-devel
> > (required by fontconfig-devel)
> 
> Okay. Presumably making openvrml-devel Require freetype-devel would moot
> Ralf's suggestion that openvrml-devel should Require pkgconfig.

As a result, yes.

> > 2. use BuildRequires: gecko-devel instead of firefox-devel and libGLU-devel
> > instead of mesa-libGLU-devel
> 
> Re: gecko-devel, I don't think I can do that. I think the change that does
> need to be made is for openvrml to Require firefox = 1.5.0.8. Note that
> libopenvrml links with libmozjs, which lives in a versioned subdirectory.
> Consequently, openvrml-devel would Require firefox-devel = 1.5.0.8.

OK, I guess this is moot, because the only package providing gecko-devel is
firefox-devel.

> > 3. reduntant Requires: mesa-libGLU for -gl subpackage
> 
> Why is that redundant?

$ rpm -qpR openvrml-gl-0.16.1-1.x86_64.rpm
...
libGLU.so.1()(64bit)
...
$ rpm -q --whatprovides "libGLU.so.1()(64bit)"
mesa-libGLU-6.5.1-7.fc6.x86_64

That's why.

> > 7. avoid unnecessary docs duplication in every subpackage
> 
> Users shouldn't need to be aware of the package dependency chain in order to 
> find the README or license for the package. Would it be acceptable to install 
> symlinks?

Well, I'll leave that for Ralf to decide. IMHO people are clueful enough to
search in openvrml-%version if they can't find it in
openvrml-mozilla-plugin-%version.

> > 8. rename mozilla-plugin to plugin
> 
> No. There are lots of kinds of plug-ins. This package makes a Mozilla/Gecko 
> one. I might entertain changing it to "gecko-plugin"; but since "Mozilla plug-
> in" or "Netscape plug-in" is what everyone calls these things, that's going to 
> be a tough sell.

OK.

One more thing. Looks like there is a test suite that can be executed using make
check. It seems to run properly even if $DISPLAY is not set, so please include
it in the specfile, i.e.:

%check
%{__make} check

(In reply to comment #9)
> (In reply to comment #6)
> > I'm interested in having this in Extras, too. A few comments:
> > 1. redundant BuildRequires: pkgconfig (required by freetype-devel and
> > libpng-devel), libpng-devel (required by cairo-devel, which is required by
> > gtk2-devel), fontconfig-devel (required by cairo-devel), freetype-devel
> > (required by fontconfig-devel)
> 
> Okay. Presumably making openvrml-devel Require freetype-devel would moot Ralf's 
> suggestion that openvrml-devel should Require pkgconfig.

Any package that contains a *.pc MUST directly "Require: pkgconfig" to make sure
%{_libdir}/pkgconfig has an owner.

> > 2. use BuildRequires: gecko-devel instead of firefox-devel
I fail to understand this.

> > 4. use %setup -q instead of %setup
Optional.

> > 5. use %configure --disable-dependency-tracking to speed up build process

Optional, left to the packager's preference. This doesn't make much sense for
this package, because this speed up is marginal for this package.

> > 7. avoid unnecessary docs duplication in every subpackage
> 
> Users shouldn't need to be aware of the package dependency chain in order to 
> find the README or license for the package. Would it be acceptable to install 
> symlinks?
No.

> > 8. rename mozilla-plugin to plugin
> 
> No. There are lots of kinds of plug-ins. This package makes a Mozilla/Gecko 
> one. I might entertain changing it to "gecko-plugin"; but since "Mozilla plug-
> in" or "Netscape plug-in" is what everyone calls these things, that's going to 
> be a tough sell.
ACK. 

I tried actually using it and it seems that the firefox plugin requires -gtkplug
subpackage to run at all. Also, it's extremely slow without hardware accelerated
OpenGL, but that's to be expected.

(In reply to comment #13)
> I tried actually using it and it seems that the firefox plugin requires -gtkplug
> subpackage to run at all.
I don't understand what you are trying to say. Rpm-wise, the mozilla-plugin
already depends on the *-gtkplug package.

(In reply to comment #7)
> BTW: if it's Assigned to you, Ralf, why is the status still NEW?
Because bugzilla hadn't changed it, when I assigned it to me?

(In reply to comment #11)
> One more thing. Looks like there is a test suite that can be executed using make
> check. It seems to run properly even if $DISPLAY is not set, so please include
> it in the specfile, i.e.:
> 
> %check
> %{__make} check

Except there's a crash-on-exit bug lurking somewhere that one of the test cases
triggers *some* of the time.


(In reply to comment #12)
> Any package that contains a *.pc MUST directly "Require: pkgconfig" to make sure
> %{_libdir}/pkgconfig has an owner.

Understood.

> > > 2. use BuildRequires: gecko-devel instead of firefox-devel
> I fail to understand this.

Not sure I do either; but for the reason I already mentioned, I think it's moot.

> > > 4. use %setup -q instead of %setup
> Optional.

What's it do?

> > > 5. use %configure --disable-dependency-tracking to speed up build process
> 
> Optional, left to the packager's preference. This doesn't make much sense for
> this package, because this speed up is marginal for this package.

True; I won't bother with it, at least for now.

> > > 7. avoid unnecessary docs duplication in every subpackage
> > 
> > Users shouldn't need to be aware of the package dependency chain in order to 
> > find the README or license for the package. Would it be acceptable to install 
> > symlinks?
> No.

Okay. I'll leave it as-is.


(In reply to comment #13)
> I tried actually using it and it seems that the firefox plugin requires -gtkplug
> subpackage to run at all.

Yes; there *is* a Requires to that effect.

> Also, it's extremely slow without hardware accelerated
> OpenGL, but that's to be expected.

There's also a bug that makes the plug-in soak up CPU cycles, which doesn't help
matters.

***

spec: http://endoframe.com/openvrml.spec
srpm: http://endoframe.com/openvrml-0.16.1-3.src.rpm

I hope I haven't missed anything.

(In reply to comment #15)
> (In reply to comment #11)

> > > > 4. use %setup -q instead of %setup
> > Optional.
> 
> What's it do?

-q silences the untarring of source tarball (tar xf instead of tar xvf). As
verbose untarring increases the size of log files, some people prefer 
"%setup -q". 

However given the size of the log files building openvrml produces, this
probably is negligible ;)
 
> > > > 5. use %configure --disable-dependency-tracking to speed up build process
> > 
> > Optional, left to the packager's preference. This doesn't make much sense for
> > this package, because this speed up is marginal for this package.
> 
> True; I won't bother with it, at least for now.
OK with me.
 
> > > > 7. avoid unnecessary docs duplication in every subpackage
> > > 
> > > Users shouldn't need to be aware of the package dependency chain in order to 
> > > find the README or license for the package. Would it be acceptable to install 
> > > symlinks?
> > No.
> 
> Okay. I'll leave it as-is.
Feel free to do as you like. Rule of thumb should be "docs should accompany
those components they document" and "ship them when you feel it's necessary".

You're the upstream author, so feel free to add "ChangeLogs", READMEs, License
files where you feel them to be appropriate.

> spec: http://endoframe.com/openvrml.spec
> srpm: http://endoframe.com/openvrml-0.16.1-3.src.rpm
> 
> I hope I haven't missed anything.

Will try to look into this later today.

(In reply to comment #16)
> > I hope I haven't missed anything.

Unfortunately, you did :(
 
> Will try to look into this later today.

Some MUSTFIXES remain:

* Some %post/%pre sections still lack a blank line at their end.

* The infos do not install/uninstall correctly:
- "info openvrml-gtkplug" doesn't jump to the corresponding info pages
- "rpm -e openvrml-gtkplug" fails:
install-info: warning: no entries found for
`/usr/share/info/openvrml-gtkplug.info.gz'; nothing deleted

The origin is a missing direntry inside of the *.info (cf. the openvrml-gtkplug
entry in /usr/share/info/dir after installing the rpm).
Normally this indicates broken/incomplete texinfo sources. 

Standard work-around would be to either patch/fix the sources, or to pass the
necessary info to install-info inside of the specs.

RECOMMENDATION:

* I recommend using %{?dist} in %release. In your case this would be
Release: 3%{?dist}

This eases a package's maintenance in longer terms.

There exist other minor issue, which IMO don't have to be addressed now and are
better be addressed after importing the packaging in FE.

(In reply to comment #17)
> (In reply to comment #16)
> > > I hope I haven't missed anything.
> 
> Unfortunately, you did :(

D'oh.

> > Will try to look into this later today.
> 
> Some MUSTFIXES remain:
> 
> * Some %post/%pre sections still lack a blank line at their end.

Damn.

> * The infos do not install/uninstall correctly:
> - "info openvrml-gtkplug" doesn't jump to the corresponding info pages
> - "rpm -e openvrml-gtkplug" fails:
> install-info: warning: no entries found for
> `/usr/share/info/openvrml-gtkplug.info.gz'; nothing deleted
> 
> The origin is a missing direntry inside of the *.info (cf. the openvrml-gtkplug
> entry in /usr/share/info/dir after installing the rpm).
> Normally this indicates broken/incomplete texinfo sources. 
> 
> Standard work-around would be to either patch/fix the sources, or to pass the
> necessary info to install-info inside of the specs.

I'm stumped here so far. There is a direntry in openvrml-gtkplug.texi; and if
there's something wrong with it, it's not apparent to me.

I do see the message you're getting upon uninstalling openvrml-gtkplug. If I
then try to reinstall it, I get this:

  install-info: menu item `openvrml-gtkplug' already exists, for file `(none)'
  error: %post(openvrml-gtkplug-0.16.1-3.x86_64) scriptlet failed, exit status 1

I'll play around with this a bit more; however, if this is not something that
can be repaired by passing arguments to install-info, I'm inclined to punt and
simply not package the info for 0.16.1. I'll fix it for 0.16.2.

Created attachment 141222 [details]
fix dirently in openvrml-gtkplug.texi

Braden, check this diff. It should solve the direntry issue.

When testing, first, manually remove any line referring to openvrml from
/usr/share/info/dir. Otherwise any attempts to run "rpm -e" will fail.

Thanks. I've buckled and converted this to a patch that is applied to the
pristine sources.

spec: http://endoframe.com/openvrml.spec
patch: http://endoframe.com/openvrml-gtkplug-info.patch
srpm: http://endoframe.com/openvrml-0.16.1-4.src.rpm

APPROVED - Packaging-wise, this package now looks sufficently well to me.

I am going to sponsor you, so you should now proceed with step 10. of
http://fedoraproject.org/wiki/Extras/Contributors


Functionally, I am facing an issue with openvrml-mozilla-plugin and firefox, 
I don't have an explaination for:

After installing "openvrml-mozilla-plugin" and restarting firefox, firefox
doesn't show the plugin in "about:plugins", nor does seem to know how to handle
*.wrl files.

I recall, earlier versions of openvrml and firefox/mozilla did, so I am
wondering about what might have happened.

Yea!

Thanks for your feedback, gentlemen.

Ralf, I'm not observing the problem you describe with openvrml-mozilla-plugin
using the 0.16.1-4 RPMs. Certainly the first thing to check is the presence of
/usr/lib/mozilla/plugins/openvrml.so.

(In reply to comment #22)
> Ralf, I'm not observing the problem you describe with openvrml-mozilla-plugin
> using the 0.16.1-4 RPMs.
I still observe it and haven't got the faintest clue about what might be going
on :(. 

ATM, I am inclined to blame (wild guesses!!) a firefox upgrade having happened
since having built the openvrml rpms (I have seen this happening several times
with other plugins and former upgrades of firefox), my very friend SELinux (It's
currently nagging me all over the place) and/or prelink .(

> Certainly the first thing to check is the presence of
> /usr/lib/mozilla/plugins/openvrml.so.
Of cause it's present. I imported the rpms into a local yum repository and used
a standard "yum install" to install the plugin.

Anyway, I officially sponsored you. The next step would be to import your srpm
into FE's CVS and to request a build. Unfortunately, AFAICT, right now FE's CVS
is broken and awaiting fixing. According to it's maintainer "on Monday" (US-time).

IME, it can be useful to copy the firefox launcher script to your home directory
and edit it such that useful console output doesn't get sent to /dev/null.

Confirming absence of OpenVRML plugin in 'about:plugins' and subsequent failure
to handle .wrl files, as reported in comment #21.

- firefox-1.5.0.8-1.fc6
- No improvement with new created profile
- strace reveals openvrml.so being loaded when consulting "about:plugins"
- ldd /usr/lib/mozilla/plugins/openvrml.so shows no obvious problems

Please report any outstanding issues as new bugs. Also note my suggestion in 
comment #24. (And see bug 157279.)

(In reply to comment #26)
> Please report any outstanding issues as new bugs.
I'd bugzilla them, but there is still no openvrml component in bugzilla.


> Also note my suggestion in comment #24. (And see bug 157279.)
Been there, tried it (and many other things), but I can't spot anything obvious.

"about:plugins" and accessing the plugin from inside firefox continue not to
work for me, on different machines, on different accounts, with locally built
rpms and with the official ones ;)

I realize STATUS=CLOSED, but for the sake of completeness :  the problem
originally reported in comment #21 is fixed in openvrml-mozilla-plugin-0.16.1-5.fc6.

NB: openvrml-mozilla-plugin-0.17.6-2.fc8 is still broken as per comment #21.

It's not "still" broken. You're observing a new bug. This may be the result of
the recent change that added -fvisibility=hidden when compiling. This works on
F9; but possibly the firefox/xulrunner headers on F8 don't specify the symbol
visibility for symbols that require external linkage.

Please file a new bug. And please include the output of

    $ nm -D /usr/lib/mozilla/plugins/openvrml.so | grep NP_

Substitute "lib64" if you're on x86_64.