Bug 485954 - Review Request: marlin - A Sound Sample Editor for GNOME
Review Request: marlin - A Sound Sample Editor for GNOME
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Joseph Smidt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-17 12:06 EST by Dodji Seketeli
Modified: 2009-02-25 11:23 EST (History)
5 users (show)

See Also:
Fixed In Version: 0.13-4.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-24 18:27:45 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
josephsmidt: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dodji Seketeli 2009-02-17 12:06:40 EST
Description
=============
Marlin aims to be a fully featured and powerful sample editor for the<
GNOME2 platform.

spec file: http://people.redhat.com/dseketel/rpms/marlin/marlin.spec-1
srpm: http://people.redhat.com/dseketel/rpms/marlin/marlin-0.13-1.fc10.src.rpm

You can check the Koji build logs for F-10 and F-11 at:

Koji task for F-10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1133396
Koji task for F-11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1133563
Comment 1 Joseph Smidt 2009-02-17 23:21:17 EST
I will begin reviewing this package.
Comment 2 Joseph Smidt 2009-02-18 01:01:29 EST
Initailly a lot looks good.

A couple issues:

1. Please be consistant with macros.  In some places you use $RPM_BUILD_ROOT,
in others %{buildroot}.

2. There is a Requires(pre) but no %pre section.

3. https://fedoraproject.org/wiki/Packaging/ScriptletSnippets says scrollkeeper should look like this: 

%post
scrollkeeper-update -q -o %{_datadir}/omf/%{name} || :

%postun
scrollkeeper-update -q || :

And like this for update-desktop-database:

%post
update-desktop-database &> /dev/null || :

%postun
update-desktop-database &> /dev/null || :

Also, you need to add:
Requires(post): desktop-file-utils
Requires(postun): desktop-file-utils

4. Remember to post your output for rpmlint.

Great work so far.
Comment 3 Dodji Seketeli 2009-02-18 05:21:10 EST
Hello Joseph,

Thanks for the quick review. It's really appreciated.

Please find updated spec file, srpm and rpmlint output at:

http://people.redhat.com/dseketel/rpms/marlin/marlin.spec-2
http://people.redhat.com/dseketel/rpms/marlin/marlin-0.13-2.fc10.src.rpm
http://people.redhat.com/dseketel/rpms/marlin/marlin-0.13-2.rpmlint.txt

Koji tasks for the new srpm are:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1135111
http://koji.fedoraproject.org/koji/taskinfo?taskID=1135128

> --- Comment #2 from Joseph Smidt <jsmidt@fedoraproject.org>  2009-02-18 
[...]

> 1. Please be consistant with macros.  In some places you use $RPM_BUILD_ROOT,
> in others %{buildroot}.

Ok, only %{buildroot} is used now.

> 2. There is a Requires(pre) but no %pre section.

Ooopsy. Added.

> 3. https://fedoraproject.org/wiki/Packaging/ScriptletSnippets says scrollkeeper
> should look like this:
> %post
> scrollkeeper-update -q -o %{_datadir}/omf/%{name} || :

Done.

> %postun
> scrollkeeper-update -q || :
>

Done.

> And like this for update-desktop-database:
>
> %post
> update-desktop-database &> /dev/null || :
>

Done.

> %postun
> update-desktop-database &> /dev/null || :
>

Done.

> Also, you need to add:
> Requires(post): desktop-file-utils
> Requires(postun): desktop-file-utils

Done.

> 4. Remember to post your output for rpmlint.
>

Done.
For the executable warnings, I have filed upstream bug http://bugzilla.gnome.org/show_bug.cgi?id=572255.

Thank you for your time.
Comment 4 Fabian Affolter 2009-02-19 03:45:30 EST
Some other quick comments on your spec file

- '--vendor fedora' is obsolete
  https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files
- Isn't 'BuildRequires: gettext' (for translation) and 'Requires: hicolor-icon-theme' (for icons) missing?
- Unversioned shared libraries should go into a -devel subpackage
  https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages
- Take a look at https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database about 'Requires(post)/(postun): desktop-file-utils'
Comment 5 Dodji Seketeli 2009-02-19 10:13:40 EST
I have updated the spec/srpm following your comments, at:

http://people.redhat.com/dseketel/rpms/marlin/marlin-3.spec
http://people.redhat.com/dseketel/rpms/marlin/marlin-0.13-3.fc10.src.rpm

The build results for F-10 and F-11 are at:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1139769
http://koji.fedoraproject.org/koji/taskinfo?taskID=1139841

> --- Comment #4 from Fabian Affolter <fabian@bernewireless.net>  2009-02-19 03:45:30 EDT ---
[...]
> 
> - '--vendor fedora' is obsolete
Right. Removed.

>   https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files
> - Isn't 'BuildRequires: gettext' (for translation)

I believe this is in the Requires of intltool that is BuildRequire'ed by
Marlin already.

> and 'Requires: hicolor-icon-theme' (for icons) missing?

I believe this is in the Requires of gtk2 that is BuildRequired'd by
Marlin already.

> - Unversioned shared libraries should go into a -devel subpackage

Ah, in theory yes. But I did talk with upstream about this and he
doesn't want to have a devel package yet, even though the architecture
of marlin is done so that external apps can benefit from it's internal
libraries. The reason is that the internal libraries are still a moving
target so he can't guarantee any API/ABI compatibility yet. When he can
do that, we can start shipping a -devel package I think.

Does this make any sense ?

> - Take a look at
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database
> about 'Requires(post)/(postun): desktop-file-utils'
> 

Okay. Thanks. I removed the
'Requires(post)/(postun): desktop-file-utils'.
Comment 6 Joseph Smidt 2009-02-19 11:21:13 EST
Fabian, correct me if I'm wrong, but gtk2 being a BuildRequires won't actually pull hicolor-icon-theme into Requires.  If the end user doesn't build the package himself/herself they will never receive hicolor-icon-theme.  So I believe you still need that for Requires.
Comment 7 Dodji Seketeli 2009-02-19 13:01:25 EST
> --- Comment #6 from Joseph Smidt <josephsmidt@gmail.com>  2009-02-19 11:21:13 EDT ---
> Fabian, correct me if I'm wrong, but gtk2 being a BuildRequires won't actually
> pull hicolor-icon-theme into Requires. 

Right, sorry. I miss-read what Fabian said. I thought he was saying hicolor-icon-theme was missing in Requires. My bad. I will update the package then.

Were my other comments correct ?
Comment 8 Tom "spot" Callaway 2009-02-20 19:57:28 EST
(In reply to comment #5)

> > - Unversioned shared libraries should go into a -devel subpackage
> 
> Ah, in theory yes. But I did talk with upstream about this and he
> doesn't want to have a devel package yet, even though the architecture
> of marlin is done so that external apps can benefit from it's internal
> libraries. The reason is that the internal libraries are still a moving
> target so he can't guarantee any API/ABI compatibility yet. When he can
> do that, we can start shipping a -devel package I think.
> 
> Does this make any sense ?

Not really. This is a very bad idea. Upstream should be versioning any libraries that they want anyone to use. Even if the API/ABI changes aggressively, he should just bump the solib versioning aggressively.

With unversioned shared libraries in the main package, any other package that uses those libraries will not know when the ABI changes have broken it. RPM will be unable to track these breaks. We put the .so files in -devel specifically to make it obvious that no normal package should depend on anything -devel.

So, unfortunately, this is a blocker. They either go in -devel or they don't get packaged.
Comment 9 Dodji Seketeli 2009-02-21 04:21:53 EST
 > --- Comment #8 from Tom "spot" Callaway <tcallawa@redhat.com>  2009-02-20 19:57:28 EDT ---
[...]
 
> So, unfortunately, this is a blocker. They either go in -devel or they don't
> get packaged.

My point was to *not* package them. I was not pushing for having the *.so in the library. The reasons I was giving were to explain why I didn't want to ship a -devel package. Not to ship *.so in the package. And as far as can tell from the spec file I pointed to at 
 http://people.redhat.com/dseketel/rpms/marlin/marlin-3.spec, there are no unversioned
 library in the package. If I am missing something, please let me know.
Comment 10 Joseph Smidt 2009-02-21 09:13:56 EST
Okay, sorry for the misunderstanding about the unversioned libraries situation.  I will continue the review.
Comment 11 Joseph Smidt 2009-02-21 13:57:39 EST
At first glance the packaging looks good.  However, when I install and run marlin there is no icon in the menu, yet there is one called for in the .desktop file. (As there should be).
Comment 12 Dodji Seketeli 2009-02-22 11:27:00 EST
Ooops, good catch. I didn't notice that.

The problem comes from the tarball that installs the marlin icon in the wrong location. I filed a bug to upstream with a patch that fixes the problem: http://bugzilla.gnome.org/show_bug.cgi?id=572750.

I added the patch to the package for now.

Updated SPEC and SRPM are available at:

http://dodji.fedorapeople.org/rpms/marlin/marlin-4.spec
http://dodji.fedorapeople.org/rpms/marlin/marlin-0.13-4.fc10.src.rpm
Comment 13 Joseph Smidt 2009-02-22 19:58:19 EST
Okay, looks good.  APPROVED.
Comment 14 Dodji Seketeli 2009-02-23 04:26:21 EST
New Package CVS Request
=======================
Package Name: marlin
Short Description: A Sound Sample Editor for GNOME
Owners: dodji
Branches: F-10
InitialCC: dodji
Comment 15 Kevin Fenzi 2009-02-24 16:06:02 EST
cvs done.
Comment 16 Fedora Update System 2009-02-24 18:25:28 EST
marlin-0.13-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/marlin-0.13-4.fc10
Comment 17 Fedora Update System 2009-02-25 11:23:28 EST
marlin-0.13-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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