Fedora Merge Review: shared-mime-info http://cvs.fedora.redhat.com/viewcvs/devel/shared-mime-info/ Initial Owner: caillon
I reviewed this package. It surely needs some work: * Can bug #459365 be closed now? * gawk is among the default package set and hence doesn't need to be BR'd. * The BR perl-XML-Parser >= 2.31-16 is not used at all and can be removed. Am I wrong? * rpmlint says: shared-mime-info.x86_64: W: devel-file-in-non-devel-package /usr/share/pkgconfig/shared-mime-info.pc Is there a valid reason why this file is not in a -devel package? Also, from the guidelines: "Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability)." shared-mime-info.x86_64: E: explicit-lib-dependency glib2 shared-mime-info.x86_64: E: explicit-lib-dependency libxml2 I believe that these explicit R's can be dropped since rpmbuild itself picks up these dependencies. * Group tag is "System Environment/Libraries" but I don't see a library in this package. * What is wrong with the locale files in the tarball? (A more detailed explanation as a comment within the SPEC file please.) Also, assuming you have a legitimate reason to remove these files, why are you BR'ing gettext? * The files ChangeLog, HACKING and most importantly COPYING need to be listed under %doc. * Macros should be used consistently. If you want to use %{__rm} notation, use macros for the other commands as well (%{__cat}, %{__make}, etc.). OR do it the other way around. But please stay consistent. * Buildroot should be one of these: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) %{_tmppath}/%{name}-%{version}-%{release}-root * According to the COPYING and test-tree-magic.c files, the license tag should be GPLv2+ * The main source file (Source0) should be given in full URL. * Parallel make must be supported whenever possible. If it is not supported, this should be noted in the SPEC file as a comment. * About the defaults.list: Can't we provide a separate list for KDE users? This may need some hacking on the source code. * See http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo for correct usage of the snippet. It's interesting to see that the very package that this guideline is based on doesn't obey the guideline itself (at least, partially).
Added Bastien and Julian to CC since they are the last known maintainers. Sorry if this was not desired.
(In reply to comment #1) > I reviewed this package. It surely needs some work: > > * Can bug #459365 be closed now? Will be when the latest update is stable. > * gawk is among the default package set and hence doesn't need to be BR'd. Fixed. > * The BR perl-XML-Parser >= 2.31-16 is not used at all and can be removed. Am I > wrong? Needed for intltool, I change it to "perl(XML::Parser)" though. > * rpmlint says: > shared-mime-info.x86_64: W: devel-file-in-non-devel-package > /usr/share/pkgconfig/shared-mime-info.pc > Is there a valid reason why this file is not in a -devel package? > Also, from the guidelines: "Packages containing pkgconfig(.pc) files > must 'Requires: pkgconfig' (for directory ownership and usability)." It's used for applications to detect where the database is installed. It's not a development file though. > shared-mime-info.x86_64: E: explicit-lib-dependency glib2 > shared-mime-info.x86_64: E: explicit-lib-dependency libxml2 > I believe that these explicit R's can be dropped since rpmbuild itself picks up > these dependencies. Done > * Group tag is "System Environment/Libraries" but I don't see a library in this > package. Changed to SE/Base > * What is wrong with the locale files in the tarball? (A more detailed > explanation as a comment within the SPEC file please.) Done. > Also, assuming you have a legitimate reason to remove these files, why are you > BR'ing gettext? Because it's needed to put the translations in the .xml file. > * The files ChangeLog, HACKING and most importantly COPYING need to be listed > under %doc. Added HACKING and COPYING, not ChangeLog, as it replicates data from NEWS whilst being much bigger. > * Macros should be used consistently. If you want to use %{__rm} notation, use > macros for the other commands as well (%{__cat}, %{__make}, etc.). > OR do it the other way around. But please stay consistent. Used the commands directly now. > * Buildroot should be one of these: > %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) > %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) > %{_tmppath}/%{name}-%{version}-%{release}-root Changed it to the second one. > * According to the COPYING and test-tree-magic.c files, the license tag should > be GPLv2+ Done. > * The main source file (Source0) should be given in full URL. Done. > * Parallel make must be supported whenever possible. If it is not supported, > this should be noted in the SPEC file as a comment. Done. > * About the defaults.list: Can't we provide a separate list for KDE users? This > may need some hacking on the source code. Because KDE doesn't use that file, and they never expressed interest. > * See > http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo > for correct usage of the snippet. It's interesting to see that the very package > that this guideline is based on doesn't obey the guideline itself (at least, > partially). Added a note as to why it shouldn't ignore errors.
I'm not a real maintainer, just did a drive-by fix.
Thanks for the update. Everything looks fine. I just have two more comments. > > * The BR perl-XML-Parser >= 2.31-16 is not used at all and can be removed. Am I > > wrong? > > Needed for intltool, I change it to "perl(XML::Parser)" though. > Ah, I noticed that now. Just a side-note: BR'ing "intltool" will then pull up "perl(XML::Parser)", so still, explicitly BR'ing "perl(XML::Parser)" is not required. But if you think this dependency might change in the future, I'm OK with leaving it as it is. > > * The files ChangeLog, HACKING and most importantly COPYING need to be listed > > under %doc. > > Added HACKING and COPYING, not ChangeLog, as it replicates data from NEWS > whilst being much bigger. > Does NEWS contain all the relevant information from ChangeLog (both from a user's and a developer's point of view)?
I understand that dropping a line of comment can be difficult sometimes. Anyways, the package is good. I don't think anyone will complain about why you didn't include the ChangeLog. If they do, you'll add it later. Merge review APPROVED.