Bug 226417

Summary: Merge Review: shared-mime-info
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Orcan Ogetbil <oget.fedora>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: bnocera, caillon, oget.fedora
Target Milestone: ---Flags: oget.fedora: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-01-31 05:31:32 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Nobody's working on this, feel free to take it 2007-01-31 20:59:04 UTC
Fedora Merge Review: shared-mime-info

http://cvs.fedora.redhat.com/viewcvs/devel/shared-mime-info/
Initial Owner: caillon@redhat.com

Comment 1 Orcan Ogetbil 2008-12-15 04:12:45 UTC
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).

Comment 2 Orcan Ogetbil 2008-12-15 04:15:07 UTC
Added Bastien and Julian to CC since they are the last known maintainers. Sorry if this was not desired.

Comment 3 Bastien Nocera 2008-12-15 13:03:07 UTC
(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.

Comment 4 Julian Sikorski 2008-12-15 13:18:49 UTC
I'm not a real maintainer, just did a drive-by fix.

Comment 5 Orcan Ogetbil 2008-12-15 16:03:02 UTC
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)?

Comment 6 Orcan Ogetbil 2009-01-31 05:30:34 UTC
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.