Bug 459751

Just some quick comments

- '--vendor fedora' is obsolete for new packages
- I think that 'Requires: hicolor-icon-theme' is needed
- To make your life easier in the future, change 'Source: http://download.sourceforge.net/osggtk/osggtk-0.1.2.tar.bz2' to 'Source: http://download.sourceforge.net/osggtk/osggtk-%{version}.tar.bz2'. Same in '%setup -q -n osggtk-0.1.2'

> -n osggtk-0.1.2

-n %{name}-%{version}   is the default.

Here's an updated spec and srpm:
http://miskatonic.cs.nmsu.edu/pub/osgGtk.spec
http://miskatonic.cs.nmsu.edu/pub/osgGtk-0.1.3-1.fc10.src.rpm

And the changelog:
- Improved package summaries and descriptions
- Added hicolor-icon-theme to viewer requires
- Removed fedora vendor tag for desktop files
- Added desktop file validation on install
- Fixed license tag
- Parameterized prep directory
- Fixed unowned directories issue in devel files

MUST Items: 

OK - rpmlint is clean

xx - does not follow Naming Guidelines
    + It would be better to name this package 'osggtk' instead of 'osgGtk'.
      The tarball is named 'osggtk' and Fedora's other OpenSceneGraph
      packages are named 'osgcal' and 'osgal'. Therefore having a completely
      lower-case name would be more consistent. But since you are also the
      upstream author, I would be willing to listen to your rationale for
      preferring otherwise. :-)

OK - spec file is named as %{name}.spec

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'.
    + 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.
    + 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.
    + 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.
    + You could consider using '%{__install} -p' consistently through the
      %install stanza.

OK - license meets Licensing Guidelines

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.

OK - upstream license file included in %doc
OK - spec file uses American English
OK - spec file is legible
OK - sources match upstream sources
OK - package builds successfully
OK - ExcludeArch not needed

xx - redundant and extra build dependencies listed
    + pkgconfig is brought in by all the -devel packages providing *.pc files

OK - no locales
OK - %post and %postun invoke ldconfig
OK - package is not relocatable

xx - file and directory ownership
    + The -devel and osgGtkmm-devel sub-packages should have
      'Requires: gtk-doc' as it needs /usr/share/gtk-doc.

OK - no duplicates in %file
OK - file permissions set properly
OK - %clean present

OK - macros used consistently
    + Both %{name} and osgGtk are used. You could consider using %{name}
      throughout.
    + 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

OK - contains code and permissable content
OK - -doc is not needed
OK - contents of %doc does not affect the runtime
OK - header files in -devel
OK - no static libraries

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

OK - library files without suffix in -devel
OK - -devel requires base package
OK - no libtool archives

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@freebook osggtk-0.1.3]$ desktop-file-validate osgviewerGtk.desktop
      osgviewerGtk.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated
      [rishi@freebook osggtk-0.1.3]$ desktop-file-validate osgviewerGtkmm.desktop 
      osgviewerGtkmm.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated
      [rishi@freebook osggtk-0.1.3]$ 
      The key "Encoding" is deprecated on all supported versions of Fedora.
      Please consider removing it.

OK - does not own files or directories owned by other packages
OK - buildroot correctly prepped
OK - all file names valid UTF-8

SHOULD Items:

OK - upstream provides license text
xx - no translations for description and summary
OK - package builds in mock successfully
OK - package builds on all supported architectures
OK - package functions as expected

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

xx - subpackages other than -devel do not use fully versioned dependency
    + The osgGtkmm subpackage does not use a fully versioned dependency on
      osgGtk.

OK - pkgconfig files in -devel
OK - no file dependencies

(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@freebook osggtk-0.1.3]$ desktop-file-validate osgviewerGtk.desktop
>       osgviewerGtk.desktop: warning: key "Encoding" in group "Desktop Entry" is
> deprecated
>       [rishi@freebook osggtk-0.1.3]$ desktop-file-validate
> osgviewerGtkmm.desktop 
>       osgviewerGtkmm.desktop: warning: key "Encoding" in group "Desktop Entry"
> is deprecated
>       [rishi@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.

(In reply to comment #5)

> 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.

Can we have a fresh Spec/SRPM pair?

(In reply to comment #6)
> (In reply to comment #5)
> 
> > 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.
> 
> Can we have a fresh Spec/SRPM pair?  

I pushed out a new upstream release that fixes things like the license in the Makefiles, the pkgconfig .pc files, etc.

This spec has all the above mentioned items fixed.

I'd really like to keep the name at osgGtk to not only match the osg library names, but to also make it easier when I figure out how to get autoconf to name the dist packages osgGtk-0.1.4.tar.bz2. I've tried to keep the case notation consistent since programmers #include<osgGtk.h> or #include<osgGtkmm.h>

http://miskatonic.cs.nmsu.edu/pub/osgGtk.spec

http://miskatonic.cs.nmsu.edu/pub/osgGtk-0.1.4-1.fc10.src.rpm

Just touching base to make sure you saw that I pushed out a new release with the fixes.

Ping?

pbrobinson's scratch build of gtk-sharp-beans?#bbba0aef7a4bd673006df5b9a85a5763d9c026bc for epel7-archbootstrap and git://pkgs.fedoraproject.org/gtk-sharp-beans?#bbba0aef7a4bd673006df5b9a85a5763d9c026bc failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12037959

pbrobinson's scratch build of gtk-murrine-engine?#b4bddbf235c281d671c99e8008f609b6d4e314d0 for epel7-archbootstrap and git://pkgs.fedoraproject.org/gtk-murrine-engine?#b4bddbf235c281d671c99e8008f609b6d4e314d0 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12037955

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Sorry, for having ignored this review for so many years. I no longer have time to work on this. Therefore, I have cleared the fedora-review flag and reset the assignee.

So, Rick are you still interested in packaging this?

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.