Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226417 - Merge Review: shared-mime-info
Summary: Merge Review: shared-mime-info
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orcan Ogetbil
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 20:59 UTC by Nobody's working on this, feel free to take it
Modified: 2009-01-31 05:31 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-01-31 05:31:32 UTC
oget.fedora: fedora-review+


Attachments (Terms of Use)

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.


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