[Bug 459751] Review Request: osgGtk - Gtk and Gtkmm widgets for OpenSceneGraph
bugzilla at redhat.com
bugzilla at redhat.com
Mon Mar 2 16:33:19 UTC 2009
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=459751
--- Comment #5 from Rick L Vinyard Jr <rvinyard at cs.nmsu.edu> 2009-03-02 11:33:15 EDT ---
(In reply to comment #4)
> MUST Items:
>
> xx - does not follow Naming Guidelines
> I would be willing to listen to your rationale for
> preferring otherwise. :-)
I had one at one point, but I forget what it was. Must not have been
compelling. If I had to guess it was just to remain consistent with the naming
of the package, which was named to be consistent with the other OSG libraries
like osgDB, osgFX, osgParticle, osgShadow, osgTerrain, et. al.
Although I have AC_INIT([osgGtk]) for some reason autoconf insists on creating
lowercase tarballs. If I could figure out a way to get it to create tarballs
named osgGtk I would.
> xx - package does not meet Packaging Guidelines
> + Although the current Source0 URL works, according to
> https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net
> the Source0 tag should have 'downloads' and not 'download'.
Got it changed.
> + Even Fedora 9 has OpenSceneGraph-devel >= 2.2.0 for sometime now.
> According to
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requires no need to
> add it if its not really required.
I would kind of like to keep it in there for EPEL support and in case anyone
tries to backport it.
> + Why is there a runtime dependency on 'OpenSceneGraph-devel >= 2.2.0' for
> osgGtkmm-devel? If it is because the osgGtkmm header files need the
> OpenSceneGraph headers, then the osgGtkmm-1.0.pc should mention it.
Ah, fixed it upstream. It was missing the openscenegraph dependency in the .pc
file.
> + The osgGtkmm sub-package does not explicitly require osgGtk. Now I can
> understand that RPM is going to autogenerate the dependency on the
> shared library, but
>
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
> does that "subpackages other than -devel should also require the base
> package using a fully versioned dependency". I find that you have done
> so for all the other sub-packages, but only not for osgGtkmm. I confess
> that I do not know the rationale behind this guideline. In the meantime,
> I will try to find out the reason.
Actually, osgGtkmm doesn't have a dependency on osgGtk, either for libraries or
headers.
> + You could consider using '%{__install} -p' consistently through the
> %install stanza.
Got it fixed.
> xx - License field does not meet actual license
> + Going by the license notices in the source code:
> (i) Makefile.am, examples/Makefile.am, osgGtk/Makefile.am,
> osgGtkmm/Makefile.am are under LGPLv3.
> (ii) the others are under GPLv3.
> Since you are the upstream author, for the Makefile.ams please consider
> marking them as GPLv3 or use the license notices in the autogenerated
> Makefile.ins.
Got it fixed upstream.
> xx - redundant and extra build dependencies listed
> + pkgconfig is brought in by all the -devel packages providing *.pc files
I'd like to leave this one in for now to support backporting and EPEL
> xx - file and directory ownership
> + The -devel and osgGtkmm-devel sub-packages should have
> 'Requires: gtk-doc' as it needs /usr/share/gtk-doc.
Added
> OK - macros used consistently
> + Both %{name} and osgGtk are used. You could consider using %{name}
> throughout.
I added it to the -devel and -viewer requires
> + Using both %{buildroot} and $RPM_BUILD_ROOT is looked down upon. See
>
> https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
Got it fixed
> OK - devel has *.pc file and requires pkgconfig
> + Even though
> https://fedoraproject.org/wiki/Packaging:Guidelines#Pkgconfig_Files
> lays down that the -devel sub-package must have 'Requires: pkgconfig' if
> it includes a *.pc file, Fedora 11 onwards rpm-4.6 autogenerates this
> runtime dependency and the ones on the other -devel subpackages mentioned
> in the *.pc file. So please consider removing them from Fedora 11 and
> onwards using a %if %endif pair.
> In osgGtk-devel:
> Requires: gtk2-devel
> Requires: gtkglext-devel
> Requires: OpenSceneGraph-devel >= 2.2.0
> Requires: pkgconfig
> In osgGtkmm-devel:
> Requires: gtkmm24-devel
> Requires: gtkglextmm-devel
> Requires: pkgconfig
>
Same as above to support EPEL and backporting.
> xx - %{name}.desktop file is invalid
> + According to https://fedoraproject.org/wiki/Packaging/Guidelines#desktop
> desktop-file-validate must be run on the .desktop file, and it says:
> [rishi at freebook osggtk-0.1.3]$ desktop-file-validate osgviewerGtk.desktop
> osgviewerGtk.desktop: warning: key "Encoding" in group "Desktop Entry" is
> deprecated
> [rishi at freebook osggtk-0.1.3]$ desktop-file-validate
> osgviewerGtkmm.desktop
> osgviewerGtkmm.desktop: warning: key "Encoding" in group "Desktop Entry"
> is deprecated
> [rishi at freebook osggtk-0.1.3]$
> The key "Encoding" is deprecated on all supported versions of Fedora.
> Please consider removing it.
Got it fixed upstream.
> SHOULD Items:
>
> xx - scriptlets are not sane
> + Would be good if you could use the Gtk+ icon cache scripts from
>
> https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#GTK.2B_icon_cache
Got it fixed.
> xx - subpackages other than -devel do not use fully versioned dependency
> + The osgGtkmm subpackage does not use a fully versioned dependency on
> osgGtk.
osgGtkmm doesn't have a dependency on osgGtk.
Let me know what you think on these items, and then I'll push out a new
upstream release and post a new spec/srpm.
1. package name
2. Keeping versioned OSG BuildRequires
3. Keeping pkgconfig requires in -devel
I think the rest is either fixed upstream or in the spec.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
More information about the Fedora-package-review
mailing list