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.
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.
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.
(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
[%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].
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?
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.
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
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.
* 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
* 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.
> 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.
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?
Yes, the package review request remains in the queue, http://fedoraproject.org/PackageReviewStatus/NEW.html and will eventually be picked up by somebody.
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?
Yes, I'm still interested. I'll upgrade to the latest release to other reviews. Thanks.
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
Thanks, I will review it ASAP.
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?
> 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.
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
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.
Taylon, is anything still unclear and you need help or can we continue?
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?
Tylon, are you still with us?
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.
0.2.3 is out now: http://gnac.sourceforge.net/archives/26 Thanks for working on this-- appreciate it a lot! Take your time.
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.