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
I will begin reviewing this package.
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.
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> 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.
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'
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> 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'.
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 #6 from Joseph Smidt <josephsmidt> 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 ?
(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 #8 from Tom "spot" Callaway <tcallawa> 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.
Okay, sorry for the misunderstanding about the unversioned libraries situation. I will continue the review.
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).
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
Okay, looks good. APPROVED.
New Package CVS Request ======================= Package Name: marlin Short Description: A Sound Sample Editor for GNOME Owners: dodji Branches: F-10 InitialCC: dodji
cvs done.
marlin-0.13-4.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/marlin-0.13-4.fc10
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.