Bug 555018 - Review Request: gnac - An audio converter for GNOME (first package, seeking sponsor)
Review Request: gnac - An audio converter for GNOME (first package, seeking s...
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Christoph Wickert
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2010-01-13 07:48 EST by Taylon
Modified: 2011-12-09 17:04 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-12-09 17:04:40 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
cwickert: fedora‑review?


Attachments (Terms of Use)

  None (edit)
Description Taylon 2010-01-13 07:48:19 EST
Spec URL: http://taylon.fedorapeople.org/gnac-0.2.1-1.fc12.src.rpm
SRPM URL: http://taylon.fedorapeople.org/gnac.spec
Description:

Gnac is an easy to use audio conversion program for the Gnome desktop. It is designed to be powerful but simple! It provides easy audio files conversion between all GStreamer supported audio formats.
Comment 1 Michael Schwendt 2010-01-13 16:34:08 EST
Only a brief look: It looks fairly well-packaged. The existence of the several GConf2 related scriptlet sections is evidence that either you've found them in the Wiki or have copied them from a package.


> $ rpmlint gnac-0.2.1-1.fc12.src.rpm 
> gnac.src: W: no-cleaning-of-buildroot %install
> gnac.src: W: no-buildroot-tag
> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.

In addition to the removal of the buildroot related stuff as discovered by rpmlint, you could also drop the %clean section, since in F-11 and newer there is a default one.


> checking for intltool >= 0.35.0...  found

This was a simple rpmbuild. "BuildRequires: intltool" is missing.

Notice that you could do full scratch-builds in Fedora's build system "koji" even before your packager account is approved:
http://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29


> configure: WARNING: The 'faac' element was not found.
> This will cause encoding to AAC to fail.

What are the implications with regard to file types which need 3rd party GStreamer plugins?

The .desktop file also defines several MIME types for file types, which are not directly supported by Fedora. Theoretically, it would be better to drop unsupported MIME types and let 3rd party add-on packages supply them together with the needed GStreamer plugins. E.g. in a gnac-freeworld package.


> Summary: An audio converter for GNOME

It's widely accepted practise to omit leading articles, such as "An" and "A" to shorten summaries even further:

  Summary: Audio converter for GNOME

That also looks better when displayed during installation.


> Requires: libgnomeui

Why is this needed? There is "BuildRequires: libgnomeui-devel", but the resulting build does not depend on libgnomeui-2.so.0. So, please adhere to
 http://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
for the explicit dependencies.


> %{_datadir}/icons/hicolor

Better:  %{_datadir}/icons/hicolor/*/*/%{name}.*

| MUST: A package must own all directories that it creates.
| If it does not create a directory that it uses, then it
| should require a package which does create that directory.
 
http://fedoraproject.org/wiki/Packaging/Guidelines#FileAndDirectoryOwnership

That means the "hicolor-icon-theme" package ought to be the only owner of that directory. And there is an indirect dependency on it through gtk2. A few other app packages still own %{_datadir}/icons/hicolor, but that is packaging mistake.


> %{_datadir}/applications/%{name}.desktop

| MUST: Packages containing GUI applications must include
| a %{name}.desktop file, and that file must be properly
| installed with desktop-file-install in the %install section.

Either that, or "desktop-file-validate" must be used.

Also, there is a MIME types definition in the .desktop file. Hence:
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database


* As a hint for the %files section: where you include entire directories recursively, such as 

  %{_datadir}/%{name}

it's more readable to append a slash like

  %{_datadir}/%{name}/

to make explicit that it's a directory and not a single file.
Comment 2 Michael Schwendt 2010-01-14 05:58:15 EST
Correcting my truncated quote of the build.log (as the "found" in there is misleading):

| checking for intltool >= 0.35.0...  found
| ./configure: line 15256: intltool-update: command not found
| configure: error: Your intltool is too old.  You need intltool 0.35.0 or later.
Comment 3 Taylon 2010-01-15 10:49:04 EST
(In reply to comment #1)
> Only a brief look: It looks fairly well-packaged. The existence of the several
> GConf2 related scriptlet sections is evidence that either you've found them in
> the Wiki or have copied them from a package.
> 

Yes, the wiki describes well the necessity of the GConf related scriptlet. I just followed the steps described on wiki.

> 
> > $ rpmlint gnac-0.2.1-1.fc12.src.rpm 
> > gnac.src: W: no-cleaning-of-buildroot %install
> > gnac.src: W: no-buildroot-tag
> > 1 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> In addition to the removal of the buildroot related stuff as discovered by
> rpmlint, you could also drop the %clean section, since in F-11 and newer there
> is a default one.
> 
Ok. I don't saw nothing about to remove the %clean section too. Maybe it should be included on the wiki too.


> > checking for intltool >= 0.35.0...  found
> 
> This was a simple rpmbuild. "BuildRequires: intltool" is missing.
> 
> Notice that you could do full scratch-builds in Fedora's build system "koji"
> even before your packager account is approved:
> http://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29DK
> 
I thought I should use koji only after be approved. Now I followed the steps to use koji and I saw the problem with inttool.

> > configure: WARNING: The 'faac' element was not found.
> > This will cause encoding to AAC to fail.
> 
> What are the implications with regard to file types which need 3rd party
> GStreamer plugins?
> 
The gnac will be unable to convert this file type. What I should to do in this case?

> The .desktop file also defines several MIME types for file types, which are not
> directly supported by Fedora. Theoretically, it would be better to drop
> unsupported MIME types and let 3rd party add-on packages supply them together
> with the needed GStreamer plugins. E.g. in a gnac-freeworld package.
>
How can I know exactly which are the MIME types supported by Fedora?
 
*** The other things I understand.
Thank you for the comments
Comment 4 Michael Schwendt 2010-01-15 12:40:53 EST
[%clean]

Indeed. Rely on the guidelines and keep the %clean then.

A quick recheck with rpm-4.7.2-1.fc12 only gives me the default --clean but not a default %clean that would remove the buildroot.


[the MIME types / unsupported file types]

"gst-inspect" prints many details for the installed GStreamer plugins. e.g. "gst-inspect mad" prints the MIME type audio/mpeg as one of the Capabilities of the "mad" plugin.

So far, gnac requires "gstreamer-plugins-base", but advertises support for the following types,

$ grep Mime gnac.desktop 
MimeType=application/ogg;application/xspf+xml;audio/mpeg;audio/mp4;audio/x-flac;audio/x-mpegurl;audio/x-musepack;audio/x-scpls;audio/vnd.rn-realaudio;audio/x-speex;audio/x-wav;audio/x-wma;

whereas some of the formats clearly need 3rd party GStreamer plugins. The question is whether to keep those defaults in the .desktop file?

If I import a .wma or .mpc file into gnac, for example, because the desktop assumes that gnac can handle those file formats, gnac doesn't complain at first. However, if I ask it to convert that file into .wav, it fails with an ambiguous warning in the status bar,

   Conversion complete with errors

and prints an additional message on the console:

$ gnac
Gnac failed to convert file file:///home/qa/Desktop/audio/test/1.wma
 Error message: An error occured during conversion

No hint on what went wrong or whether additional plugins would be needed. Since gnac package cannot depend on 3rd party GStreamer packages, it is sort of "crippled" without the missing plugin packages but tells the desktop database that it understands the file formats. It's a package design question.

The downside of removing unsupported file types from the .desktop file would be that a 3rd party gnac add-on package would be needed [to provide a hidden .desktop file that adds the additional types and to add all extra dependencies on plugin/codec packages].
Comment 5 Taylon 2010-01-15 13:41:28 EST
So wouldn't better to keep the file types on .desktop file and let the user take care of the 3rd party Gstreamer packages instalation?
Comment 6 Michael Schwendt 2010-01-16 05:59:58 EST
There's no guideline about that [yet]. You might want to ask for feedback on the Fedora Packaging list: http://admin.fedoraproject.org/mailman/listinfo/packaging

[...]

Personally, I would find it "better" and cleaner if an app didn't register for MIME types it doesn't support. And only by installing fully optional add-on/plugin packages the app would register additional file types.

The opposite would be like offering an audio player, which pretends that it can play MP3 files while actually it can't with the software provided by stock Fedora. Or like a compression utility that advertises support for RAR archives but actually is built without that capability.

It may look like a matter of taste, but eventually Fedora will need to cover this with its packaging guidelines.
Comment 7 Taylon 2010-01-26 07:16:09 EST
Well, we hadn't no general consensus on the list discussion[1], so I think better keep the current audio player behavior while we haven't no guideline.

I fixed the quoted problems:
Spec URL: http://taylon.fedorapeople.org/gnac-0.2.1-1.fc12.src.rpm
SRPM URL: http://taylon.fedorapeople.org/gnac.spec

[1] - http://lists.fedoraproject.org/pipermail/packaging/2010-January/006808.html
Comment 8 Michael Schwendt 2010-01-29 11:41:43 EST
Okay, I'll ignore that part of the review then. It's somewhat disappointing, though, to advertise support for MP3/WMA/MP4/RA and other audio formats which Fedora cannot handle. And gnac's functionality cannot be extended by installing arbitrary GStreamer audio decoder packages. It is limited to a set of file formats that is hardcoded somewhere.
Comment 9 Michael Schwendt 2010-02-02 05:32:58 EST
* The question about "libgnomeui" still needs an answer. ;)

* "BuildRequires: intltool" is still missing.

* On the run-time testing side, it sort of works, but I've had it crash (with a useless backtrace created by ABRT) after trying to add+convert+remove+add a couple of test audio files (not limited to WMA, MPC, M4A, MP3). Upon trying to reproduce the crash, I managed to make it fail to convert an MP3, which ended up as WAV file with a size of 0.

* There is a competing package/application, albeit not based on GStreamer: audio-convert-mod
Comment 10 Taylon 2010-02-06 12:20:41 EST
* After you warned me about the libgnomeui on the first comment, I read the documentation again and realized that libgnomeui isn't needed.

* But a added it on .spec file. There is more something needed?

* It's happening because you don't have the gstreamer-plugins-ugly installed, I didn't realize this problem before because I already had the gstreamer-plugins-ugly installed on my system, now that you reported this problem I unnistalled it and realized that is impossible to convert the most of file types without this. gstreamer-plugin-ugly is provided by rpmfusion-free, I search for something about gstreamer on wiki but didn't find.

* There are too the SoundConverter that's based on gstreamer and without gstreamer-plugins-ugly and gstreamer-plugins-ffmpeg (both provided by rpmfusion-free) it cannot convert almost nothing. Taking a look in audio-convert-mod, it too don't converts anything if you don't install the needed enconders/decoders.
Comment 11 Michael Schwendt 2010-02-08 07:25:53 EST
> intltool

Yeah, you managed to confuse me by not increasing the "Release" value of the package. The gnac-0.2.1-1.fc12.src.rpm I got was not the latest. Rule of thumb: Everytime you modify the .spec file to build a new src.rpm, increase the Release tag and add a %changelog entry.

> gstreamer-plugins-ugly

That one is installed, and still something went wrong after keeping Gnac open and performing some tests to examine app behaviour. It's not a blocker for the review process, but it might be that a bug is lurking in the code.

[...]

As I don't know when I will find the time to do more reviews of this one or other packages, I reset the flags of this ticket.
Comment 12 Taylon 2010-02-13 09:43:02 EST
Oh sorry, I thought I should increase the release value just after the review process.

Here is the package with the release increased:
Spec URL: http://taylon.fedorapeople.org/gnac-0.2.1-2.fc12.src.rpm
SRPM URL: http://taylon.fedorapeople.org/gnac.spec

Thank you very much by the reviews, now another reviewer will continue the package revision?
Comment 13 Michael Schwendt 2010-02-15 17:34:42 EST
Yes, the package review request remains in the queue,
  http://fedoraproject.org/PackageReviewStatus/NEW.html
and will eventually be picked up by somebody.
Comment 14 Christoph Wickert 2010-07-03 03:47:23 EDT
Taylon, are you still interested in packaging this? If so, please update the package to the latest release 0.2.2. I will then do the final review and can also sponsor you. Did you have the time to look at other reviews?
Comment 15 Taylon 2010-07-05 08:39:31 EDT
Yes, I'm still interested.
I'll upgrade to the latest release to other reviews.

Thanks.
Comment 16 Taylon 2010-07-22 12:03:50 EDT
Here is the package updated to the latest release:
SRPM URL: http://taylon.fedorapeople.org/gnac-0.2.2-1.fc13.src.rpm
SPEC URL: http://taylon.fedorapeople.org/gnac.spec
Comment 17 Christoph Wickert 2010-07-28 20:59:02 EDT
Thanks, I will review it ASAP.
Comment 18 Christoph Wickert 2010-07-29 08:52:58 EDT
REVIEW FOR 9fc296dc43daf7434c85f796c3ce4186  gnac-0.2.2-1.fc13.src.rpm


FIX - MUST: 
$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/gnac-*
gnac.src: W: no-cleaning-of-buildroot %install
gnac.src: W: no-buildroot-tag
gnac.x86_64: W: non-conffile-in-etc /etc/gconf/schemas/gnac.schemas
gnac-debuginfo.x86_64: E: debuginfo-without-sources
3 packages and 0 specfiles checked; 1 errors, 3 warnings.

The first two warnings can be ignored, however I prefer to add both the BuildRoot tag as well as 'rm -rf $RPM_BUILD_ROOT' at the beginning of %install for compatibility with older rpm version, but this is up to you.
The 3rd warning can be ignored as well, gconf files are no config files but templates for the per-user files.
The error about the missing debuginfo needs to be fixed though (see below).

OK - MUST: named according to the Package Naming Guidelines
OK - MUST: spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines (GPLv2+)
OK - MUST: License field in spec file matches the actual license (Note that COPYING is GPLv3, but the headers are GPLv2+, so I assume COPYING is wrong.)
OK - MUST: license file included in %doc
OK - MUST: spec is in American English
OK - MUST: spec is legible
OK - MUST: sources match the upstream source by MD5 da02008960c79f31a558008b9fd74d70
OK - MUST: successfully compiles and builds into binary rpms on x86_64
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: all build dependencies are listed in BuildRequires.
OK - MUST: handles locales properly with %find_lang
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
OK - MUST: Package does not bundle copies of system libraries.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: owns all directories that it creates
OK - MUST: no duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, includes %defattr(...)
OK - MUST: package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT
OK - MUST: consistently uses macros
OK - MUST: package contains code, or permissable content
N/A - MUST: Large documentation files should go in a -doc subpackage
OK - MUST: Files included as %doc do not affect the runtime of the application
N/A - MUST: Header files must be in a -devel package
N/A - MUST: Static libraries must be in a -static package
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix, then library files that end in .so must go in a -devel package.
N/A - MUST: devel packages must require the base package using a fully versioned dependency
OK - MUST: The package does not contain any .la libtool archives.
OK - MUST: The package contains a GUI application and includes a %{name}.desktop file, and that file is properly validated with desktop-file-validate in the %install section.
OK - MUST: package does not own files or directories already owned by other packages.
OK - MUST: all filenames valid UTF-8


SHOULD Items:
OK - SHOULD: Source package includes license text(s) as a separate file.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: builds in mock.
OK - SHOULD: compiles and builds into binary rpms on all supported architectures.
OK - SHOULD: functions as described.
FIX - SHOULD: Scriptlets are used, those scriptlets must be sane (gconf scriptlets need to be updated, see below).
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg
OK - SHOULD: no file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin


Other items:
OK - latest stable version
OK - SourceURL valid
FIX - Compiler flags ok
FIX - Debuginfo complete


Issues:
- Fix rpmlint as described above. For more info on the issue of the missing sources in the debuginfo package see http://fedoraproject.org/wiki/Packaging/Debuginfo 
I haven't investigated it, but it is most likely due to the compiler flags not being honored.
- Please use the new gconf macros from http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf
- Please change %{_mandir}/man1/gnac.1.gz to %{_mandir}/man1/gnac.1.* so the extension .gz is not hardcoded. We might switch to bz2 or xz compressed manpages some day.
- Bug upstream about including a copy of GPLv2 instead of GPLv3 or change the headers accordingly.
- the timestamp of the source tarball in the srpm doesn't match the original one, see https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
- I agree with Michael that the nonfree mimetypes in the desktop file and profiles in /usr/share/gnac/profiles should be removed from the package. For the desktop file you can use desktop-file-install instead of desktop-file-validate and the xml files can be removed at the end of the %install section.
I will then make a gnac-freeworld package at rpmfusion that provides a hidden desktop file with the mimetypes, the profiles and requires the necessary plugins. Sounds fair?
Comment 19 Taylon 2010-08-07 08:44:03 EDT
> The first two warnings can be ignored, however I prefer to add both the
> BuildRoot tag as well as 'rm -rf $RPM_BUILD_ROOT' at the beginning of %install
> for compatibility with older rpm version, but this is up to you.
> The 3rd warning can be ignored as well, gconf files are no config files but
> templates for the per-user files.
> The error about the missing debuginfo needs to be fixed though (see below).

Ok, I added both BuildRoot and 'rm -rf $RPM_BUILD_ROOT'.

> Issues:
> - Fix rpmlint as described above. For more info on the issue of the missing
> sources in the debuginfo package see
> http://fedoraproject.org/wiki/Packaging/Debuginfo 
> I haven't investigated it, but it is most likely due to the compiler flags not
> being honored.

I couldn't solve this issue. I checked a lot of things and everything seems to be ok.

- I checked the build output to make sure that '-g' option was on CFLAGS and CXXFLAGS tags.
- All binaries files are executable.
- There's no setuid or setgid binaries.
- I didn't find explicit strip symbols.

I also took a look on the Makefile and configure.ac but I didn't find nothing wrong.

What should I do? Ask for help on the Fedora Packaging list?

> - Please use the new gconf macros from
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf

Done.

> - Please change %{_mandir}/man1/gnac.1.gz to %{_mandir}/man1/gnac.1.* so the
> extension .gz is not hardcoded. We might switch to bz2 or xz compressed
> manpages some day.

Done.

> - Bug upstream about including a copy of GPLv2 instead of GPLv3 or change the
> headers accordingly.

Done.
https://bugzilla.gnome.org/show_bug.cgi?id=625585

> - the timestamp of the source tarball in the srpm doesn't match the original
> one, see https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

Done.

> - I agree with Michael that the nonfree mimetypes in the desktop file and
> profiles in /usr/share/gnac/profiles should be removed from the package. For
> the desktop file you can use desktop-file-install instead of
> desktop-file-validate and the xml files can be removed at the end of the
> %install section.
> I will then make a gnac-freeworld package at rpmfusion that provides a hidden
> desktop file with the mimetypes, the profiles and requires the necessary
> plugins. Sounds fair?    

Ok, so from desktop file I should remove:

audio/vnd.rn-realaudio
audio/x-wav
audio/x-wma

and from profiles:

mp3 and wav related

That's right?


Thanks for the review.
Comment 20 Hans de Goede 2010-11-03 08:33:34 EDT
Hi, Christoph

I'm going through some FE-NEEDSPONSOR review requests and stumbled over this
one it seems to be waiting on input from you, can you please answer comment 19 ?

Also you can sponsor right? Do you plan to sponsor Taylon?

Regards,

Hans
Comment 21 Christoph Wickert 2010-11-25 19:32:20 EST
Sorry, I copmpletely missed this one again. :(

(In reply to comment #19)
> > - Fix rpmlint as described above. For more info on the issue of the missing
> > sources in the debuginfo package see
> > http://fedoraproject.org/wiki/Packaging/Debuginfo 
> > I haven't investigated it, but it is most likely due to the compiler flags not
> > being honored.
> 
> I couldn't solve this issue. I checked a lot of things and everything seems to
> be ok.

Ok, I will look into it then.

> Ok, so from desktop file I should remove:
> 
> audio/vnd.rn-realaudio
> audio/x-wav
> audio/x-wma
> 
> and from profiles:
> 
> mp3 and wav related
> 
> That's right?

Wav is ok for us. To remove something, you can use desktop-file-install.


(In reply to comment #20)
> Do you plan to sponsor Taylon?

Yes.
Comment 22 Christoph Wickert 2010-12-19 04:53:11 EST
Taylon, is anything still unclear and you need help or can we continue?
Comment 23 Taylon 2010-12-23 07:23:50 EST
Sorry, I don't know why, but I didn't received e-mails informing about updates here.

I want to continue but until the next year (it's very close =D) I'll not be able.

I'll send updates as soon as possible ok?
Comment 24 Christoph Wickert 2011-02-20 16:12:55 EST
Tylon, are you still with us?
Comment 25 Christoph Wickert 2011-06-17 22:25:46 EDT
Tylon, I'd really like to finish this review and sponsor you.

If you are busy of cannot maintain this package for other reasons, please let us know.
Comment 26 kxra 2011-08-26 14:11:55 EDT
0.2.3 is out now: http://gnac.sourceforge.net/archives/26

Thanks for working on this-- appreciate it a lot! Take your time.
Comment 27 Christoph Wickert 2011-12-09 17:04:40 EST
It seems that Tylor is no longer interested Danny, if you would like to take over this package, please go ahead. Update the package, file a new review request and mark then change the resolution of this one to "Duplicate" of the new one. Thanks.

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