Bug 226417 - Merge Review: shared-mime-info
Merge Review: shared-mime-info
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 15:59 EST by Nobody's working on this, feel free to take it
Modified: 2009-01-31 00:31 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-31 00:31:32 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:59:04 EST
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-14 23:12:45 EST
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-14 23:15:07 EST
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 08:03:07 EST
(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 08:18:49 EST
I'm not a real maintainer, just did a drive-by fix.
Comment 5 Orcan Ogetbil 2008-12-15 11:03:02 EST
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 00:30:34 EST
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.