Bug 608326 - Review Request: gtkmm30 - C++ interface for the GTK+ library
Review Request: gtkmm30 - C++ interface for the GTK+ library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
: Reopened
Depends On: 608141 610195
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-26 16:47 EDT by Kalev Lember
Modified: 2010-07-09 14:53 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-09 14:53:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kalev Lember 2010-06-26 16:47:27 EDT
Spec URL: http://kalev.fedorapeople.org/gtkmm30.spec
SRPM URL: http://kalev.fedorapeople.org/gtkmm30-2.90.3.1-1.fc14.src.rpm
Description:
gtkmm provides a C++ interface to the GTK+ GUI library. gtkmm30 wraps
GTK+ 3. Highlights include typesafe callbacks, widgets extensible via
inheritance, and a comprehensive set of widget classes that can be
freely combined to quickly create complex user interfaces.
Comment 1 Kalev Lember 2010-07-05 07:16:55 EDT
* Mon Jul 05 2010 Kalev Lember <kalev@smartlink.ee> - 2.90.4.0-1
- Update to 2.90.4.0

Spec URL: http://kalev.fedorapeople.org/gtkmm30.spec
SRPM URL: http://kalev.fedorapeople.org/gtkmm30-2.90.4.0-1.fc14.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2295904
Comment 2 Kalev Lember 2010-07-05 15:32:33 EDT
Reopening the ticket; looks like you closed a wrong one.
Comment 3 Orcan Ogetbil 2010-07-05 17:57:36 EDT
Hi, this is my preliminary notes. I still have a few other things to check to complete the review:

! The file ChangeLog can go to devel package, as we have the user-friendly NEWS file in the main package.

? Is it possible to run the tests in the tests/ directory in a %check section? Or should we include them in the devel package?

* Package name is odd. Actually the whole gtk related packages have weird names. I see that they didn't pass the merge review yet, that's probably why. Since there is no package called gtkmm, can we call this package simply gtkmm, so that we can stay more consistent with the guidelines?

* There is a problem with license. We have a file called COPYING.tools (GPLv2), which suggests that the tools/ directory is GPLv2. Indeed when we look at tools/extra_defs_gen/generate_defs_gtk.cc we see that it is licensed GPLv2+. However this file does not get installed. On the other hand, the contents of the directory tools/m4 get installed. Unfortunately, these files do not indicate a license. Are these files GPL or LGPL? This needs to be clarified by upstream.

* Macro issue: We should use %{_datadir} instead of /usr/share

- Requires: pkgconfig is missing in the devel package. However this is not a problem if the package will be Fedora only.
Comment 4 Orcan Ogetbil 2010-07-05 18:52:28 EDT
Alright, here is the rest of the complete review:

* rpmlint says:
    gtkmm30.src: W: invalid-url Source0: http://ftp.gnome.org/pub/GNOME/sources/gtkmm/2.90.4.0./gtkmm-2.90.4.0.tar.bz2 HTTP Error 404: Not Found 
    gtkmm30.x86_64: W: spelling-error %description -l en_US gtkmm 
These two can be ignored. However

    gtkmm30.x86_64: W: spelling-error %description -l en_US typesafe -> type safe, type-safe, typeset
    gtkmm30.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libgtkmm-3.0.so.1.1.0 ['/usr/lib64']
These two need a fix

! Please make the description span 80 columns as much as possible. Currently it is set to 70 columns.

! BRs: cairomm-devel and glibmm24-devel seem redundant. They get pulled it by other dependencies. They don't cause any harm though.

! The package gtk-doc is listed as a dependency for directory ownership. This is fine for now. However keep in mind that there is a discussion to change this policy so that all packages that use usr/share/gtk-doc/ must own this directory. See:

  https://fedoraproject.org/wiki/PackagingDrafts/Revised_File_and_Directory_Ownership#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function
Comment 5 Kalev Lember 2010-07-06 12:25:55 EDT
Thanks for the review.

(In reply to comment #3)
> ! The file ChangeLog can go to devel package, as we have the user-friendly
> NEWS file in the main package.
I am not sure if it's such a good idea; it's difficult for people to look up things if docs are cluttered all over in different directories:
/usr/share/doc/gtkmm30-2.90.4.0/NEWS
/usr/share/doc/gtkmm30-devel-2.90.4.0/ChangeLog

Anyway, I revised the docs in -devel package and removed PORTING file, which was largely useless.

That leaves us with only:
%doc demos/gtk-demo/
in -devel package. I wonder if it'd be better to move gtk-demo to -doc package too, so that most docs (including NEWS and ChangeLog) are in main package, -devel package contains no docs, and API docs + demos are in -doc package?


> ? Is it possible to run the tests in the tests/ directory in a %check section?
> Or should we include them in the devel package?

Ah, very good catch. Added it to the %check section.


> * Package name is odd. Actually the whole gtk related packages have weird
> names. I see that they didn't pass the merge review yet, that's probably why.
> Since there is no package called gtkmm, can we call this package simply gtkmm,
> so that we can stay more consistent with the guidelines?

I think it's less confusing to name it gtkmm30. Very different from most packages, gtk stack is designed to be parallel installable from the ground up.
 - link-libraries have 3.0 in their names:
     /usr/lib/libgdkmm-3.0.so
     /usr/lib/libgtkmm-3.0.so
 - pkgconfig files have 3.0 in their names:
     /usr/lib/pkgconfig/gdkmm-3.0.pc
     /usr/lib/pkgconfig/gtkmm-3.0.pc
 - headers are in versioned directory:
     /usr/include/gtkmm-3.0/

When for most packages major version upgrades come naturally, then for gtkmm people need to explicitly port their apps to use the new API: link-library names, pkgconfig file names, and include directories have all changed.

Lets say there will be gtkmm 3.0, 3.2, 3.4, and 3.6 releases with 3.0 API. After that comes gtkmm 3.8 API: gtkmm38-3.8, gtkmm38-3.10, gtkmm38-3.12, and so on.

As I said before, upstreams need to explicitly port their apps to use next API. That means if we named this package (API version 3.0) to just "gtkmm", we'll still need to name the package of the next API version "gtkmm38". Look at the package list we have:
gtkmm20
gtkmm24
gtkmm   <--- this one looks off
gtkmm38

I really think it's easier for end users to use gtkmm30 package name here.

Besides, I tried to look up the review request ticket for gtkmm20 but couldn't find it, grr. Wonder what has happened to it.

Any idea what's this "fedora bug 727"? From gtkmm24 spec:
* Sat Oct 4 2003 Michael Koziarski <michael@koziarski.org> - 0:2.2.8-0.fdr.1
- Incorporated Michael Schwendt's Comments in fedora bug 727

Also, the same spec file indicates that there was gtkmm package once:
* Mon Oct 14 2002 Gary Peck <gbpeck@sbcglobal.net> - 1.3.24-gp1
- Initial release of gtkmm2, using gtkmm spec file as base 


> * There is a problem with license. We have a file called COPYING.tools (GPLv2),
> which suggests that the tools/ directory is GPLv2. Indeed when we look at
> tools/extra_defs_gen/generate_defs_gtk.cc we see that it is licensed GPLv2+.
> However this file does not get installed. On the other hand, the contents of
> the directory tools/m4 get installed. Unfortunately, these files do not
> indicate a license. Are these files GPL or LGPL? This needs to be clarified by
> upstream.
Talked with upstream and this has been clarified to be LGPL: 
https://bugzilla.gnome.org/show_bug.cgi?id=623681


> * Macro issue: We should use %{_datadir} instead of /usr/share
Fixed. I should really dig into the build scripts and figure out something sane instead of resorting to all this sed magic.

> - Requires: pkgconfig is missing in the devel package. However this is not a
> problem if the package will be Fedora only.
Yes, it will be rawhide-only.


(In reply to comment #4)
>     gtkmm30.src: W: invalid-url Source0:
> http://ftp.gnome.org/pub/GNOME/sources/gtkmm/2.90.4.0./gtkmm-2.90.4.0.tar.bz2
> HTTP Error 404: Not Found 
Fixed. Looks like changing %global to %define makes rpmlint shut up here.

>     gtkmm30.x86_64: W: spelling-error %description -l en_US typesafe -> type
> safe, type-safe, typeset
Fixed. I also updated the whole description, which wasn't very readable, so please reread that.

>     gtkmm30.x86_64: E: binary-or-shlib-defines-rpath
> /usr/lib64/libgtkmm-3.0.so.1.1.0 ['/usr/lib64']
Fixed.

> ! Please make the description span 80 columns as much as possible. Currently
> it is set to 70 columns.
I did that and then changed back again after rewording the description. I have nothing against 80 columns per se, it's just that the right edge gets too jagged with 80 columns and it's easier to read it like it is now.


> ! BRs: cairomm-devel and glibmm24-devel seem redundant. They get pulled it by
> other dependencies. They don't cause any harm though.
Didn't change that. gtkmm's configure script looks explicitly for those two, so I'd rather keep the buildrequires instead of hoping that some other packages pull those in.

> ! The package gtk-doc is listed as a dependency for directory ownership. /.../
Thanks, will keep an eye on the draft.
Comment 6 Kalev Lember 2010-07-06 12:41:13 EDT
* Tue Jul 06 2010 Kalev Lember <kalev@smartlink.ee> - 2.90.4.0-2
- Review fixes (#608326)
- Fixed lib64 rpaths
- Added %%check section
- Use %%define instead of %%global to set release_version macro, as the latter
  seems to confuse rpmlint
- Replaced hardcoded /usr/share with %%_datadir macro
- Updated description

Spec URL: http://kalev.fedorapeople.org/gtkmm30.spec
SRPM URL: http://kalev.fedorapeople.org/gtkmm30-2.90.4.0-2.fc14.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2298573
Comment 7 Orcan Ogetbil 2010-07-06 22:20:01 EDT
I should have added that the *'s in my review were more important, i.e. blockers. !'s were suggestions and warnings. But almost everything is straightened out now as it seems, except the package name. 

I understand the convention you're using. However according to the guidelines, the latest stable version should be a straight "gtkmm", and older (or unstable newer) can have versions appended to the name, like automake14, python3 etc. I also understand that there is not enough manpower to change package names and do reviews, on every API change. Since this is the way it has been with gtk packages, I guess nobody will yell. Fair enough.

There is one last issue though. I think you didn't run rpmlint against the newest build. The devel package itself gives 25 errors, 48 warnings, such as
   W: doc-file-dependency
   W: spurious-executable-perm
   E: arch-dependent-file-in-usr-share
etc. You might want to have a look. I am not pasting the output here, as it is really long.

> That leaves us with only:
> %doc demos/gtk-demo/
> in -devel package. I wonder if it'd be better to move gtk-demo to -doc package
> too, so that most docs (including NEWS and ChangeLog) are in main package,
> -devel package contains no docs, and API docs + demos are in -doc package?

I am fine either way. But make sure you don't build them if you are putting them in /usr/share/, c.f. the rpmlints above.
Comment 8 Kalev Lember 2010-07-07 07:45:56 EDT
It appears that the demos are built on "make check", that's why the binary files didn't turn up in /usr/share/ before last batch of changes.

* Wed Jul 07 2010 Kalev Lember <kalev@smartlink.ee> - 2.90.4.0-3
- Avoid putting built demos in /usr/share (#608326)
- Moved demos to -doc package

Spec URL: http://kalev.fedorapeople.org/gtkmm30.spec
SRPM URL: http://kalev.fedorapeople.org/gtkmm30-2.90.4.0-3.fc14.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2299972
Comment 9 Orcan Ogetbil 2010-07-07 16:35:22 EDT
Thanks. All good.

------------------------------------------
This package (gtkmm30) is APPROVED by oget
------------------------------------------
Comment 10 Kalev Lember 2010-07-07 23:04:53 EDT
Thanks for the review, Orcan!

New Package CVS Request
=======================
Package Name: gtkmm30
Short Description: C++ interface for the GTK+ library
Owners: kalev rishi hguemar
Branches:
InitialCC:
Comment 11 Kevin Fenzi 2010-07-09 14:20:10 EDT
CVS done (by process-cvs-requests.py).
Comment 12 Kalev Lember 2010-07-09 14:53:32 EDT
Imported and built in rawhide, closing the ticket.

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