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 225855 - Merge Review: gphoto2
Summary: Merge Review: gphoto2
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Debarshi Ray
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 18:57 UTC by Nobody's working on this, feel free to take it
Modified: 2008-10-15 04:46 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-10-15 04:46:59 UTC
debarshir: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 18:57:38 UTC
Fedora Merge Review: gphoto2

http://cvs.fedora.redhat.com/viewcvs/devel/gphoto2/
Initial Owner: jnovy@redhat.com

Comment 1 Debarshi Ray 2008-06-21 20:16:56 UTC
MUST Items: 

OK - rpmlint is clean
OK - follows Naming Guidelines
OK - spec file is named as %{name}.spec

xx - package does not meet Packaging Guidelines
    + Is it necessary to define multilib_arches?
    + Since the package no longer carries the library, it should not be
mentioned in the description.
    + Source0 does not conform to
https://fedoraproject.org/wiki/Packaging/SourceURL packaging/rpm/package.spec.in
and packaging/rpm/gphoto2.spec seem to have a non-functional URL. Upstream
should be informed about it.
    + The --enable-docs and --enable-lockdev options could not be found in
configure and configure.ac and look like remnants from libgphoto2.
    + Is --with-doc-dir really needed? Shouldn't it by
%{_docdir}/%{name}-%{version} instead of %{_docdir}/%{name}?
    + To preserve timestamps you could consider using:
      make install INSTALL="%{__install} -p" DESTDIR=$RPM_BUILD_ROOT
    + Why not include ChangeLog and TODO in %doc?
    + Why not include contrib/simple-mtpupload in %doc?

OK - license meets Licensing Guidelines

xx - License field meets actual license
    + Should be GPLv2+ instead of GPLv2. See
http://fedoraproject.org/wiki/Licensing#SoftwareLicenses
packaging/rpm/package.spec.in and packaging/rpm/gphoto2.spec wrongly mention
LGPL. Upstream should be informed about it.

OK - upstream license file included in %doc
OK - spec file uses American English
OK - spec file is legible
OK - sources match upstream sources
OK - package builds successfully

OK - ExcludeArch for s390 and s390x
    + s390 and s390x are not Fedora supported architectures, yet. Out of
curiosity, what is the reason for this?

xx - redundant and extra build dependencies listed
    + libusb-devel and libexif-devel are brought in by libgphoto2-devel,
lockdev-devel looks like an old requirement from libgphoto2, while pkgconfig is
brought in by all the -devel packages providing *.pc files.

xx - locales not handled properly
    + BuildRequires: gettext should be added. See
https://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files

OK - no shared libraries
OK - package is not relocatable
OK - file and directory ownership
OK - no duplicates in %file

OK - file permissions set properly
    + The preferred attribute definition is: %defattr(-,root,root,-)

OK - %clean present

xx - macros not used consistently
    + Both %{buildroot} and $RPM_BUILD_ROOT notations used.
    + No need to enclose them within double quotes. According to
https://fedoraproject.org/wiki/Packaging/Guidelines#Macros only one style should
be used.

OK - contains code and permissable content
OK - -doc is not needed
OK - contents of %doc does not affect the runtime
OK - no header files
OK - no static libraries
OK - no pkgconfig files
OK - no library files
OK - -devel is not needed
OK - no libtool archives
OK - %{name}.desktop file not needed
OK - does not own files or directories owned by other packages
OK - buildroot correctly prepped
OK - all file names valid UTF-8

SHOULD Items:

OK - upstream provides license text
xx - no translations for description and summary
OK - package builds in mock successfully

OK - package builds on all supported architectures
    + s390 and s390x are excluded, which are not Fedora architectures.

OK - package functions as expected
OK - scriptlets are sane
OK - subpackages are not needed
OK - no pkgconfig files
OK - no file dependencies

Comment 2 Debarshi Ray 2008-06-22 12:03:29 UTC
If the package is creating the %{buildroot}%{_docdir}/%{name} directory, it
should either be removed or owned by the package because a package must own all
directories that it creates.

And I am only curious to know why this fails to build on s390 and s390x, and not
why they are not Fedora-supported architectures. :-)

Comment 3 Debarshi Ray 2008-07-11 20:27:16 UTC
Ping?

Comment 4 Debarshi Ray 2008-07-23 04:54:07 UTC
This review seems to be stalled.

Jindrich, according to
https://fedoraproject.org/wiki/PackageMaintainers/Policy/StalledReviews#Submitter_not_responding
you need to respond within a week.

Comment 5 Jindrich Novy 2008-08-02 07:35:57 UTC
Sorry, I was in Africa for 3 weeks with no  internet access.

I applied all your points except:
+ Why not include contrib/simple-mtpupload in %doc?
reason:
It contains perl script that could pull in a perl dependency, which shouldn't be
otherwise needed. The questin is whether /usr/share/doc is a good place for
scripts anyway.

IIRC the s390(x) excluding is because it simply doesn't have USB support.

Thanks for review!

Comment 6 manuel wolfshant 2008-08-02 10:18:12 UTC
Just "chmod -x simple-mtpupload " and rpm will no longer pull perl as dependency

Comment 7 Debarshi Ray 2008-08-07 04:39:41 UTC
(In reply to comment #5)

+ What is the need for two Source0 lines? The first one should be simply removed.

+ Unnecessary 'BuildRequires: pkgconfig' still remains.

+ You could look at Manuel's comment on simple-mtpupload, but its finally upto you.

Comment 8 Debarshi Ray 2008-08-07 04:44:44 UTC
(In reply to comment #5)

Fedora tends to prefer %defattr(-,root,root,-) (http://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo), so it would be nice if you could use it instead of %defattr(-,root,root).

But this is not a blocker for the review.

Comment 9 Jindrich Novy 2008-08-07 04:55:14 UTC
(In reply to comment #8)
> (In reply to comment #5)
> 
> Fedora tends to prefer %defattr(-,root,root,-)

Updated.

(In reply to comment #7)
> (In reply to comment #5)
> 
> + What is the need for two Source0 lines? The first one should be simply
> removed.

It was a typo, the older one should just have to go away.

> 
> + Unnecessary 'BuildRequires: pkgconfig' still remains.
> 

Removed.

> + You could look at Manuel's comment on simple-mtpupload, but its finally upto
> you.

Well, putting executable stuff into /usr/share/doc is not a good idea, we should either package it to be actually executable in /usr/bin or not to package it at all. Just removing the executable attribute to fool RPM is not a solution IMO.

Thanks for the review :)

Comment 10 Debarshi Ray 2008-08-09 12:58:02 UTC
(In reply to comment #9)

Looks like a rpmlint warning has crept in during the review:
    + [rishi@ginger x86_64]$ rpmlint gphoto2-2.4.2-3.fc10.x86_64.rpm
      gphoto2.x86_64: W: file-not-utf8 /usr/share/doc/gphoto2-2.4.2/ChangeLog
      [rishi@ginger x86_64]$ 

Could be fixed as shown in https://fedoraproject.org/wiki/PackageMaintainers/Common_Rpmlint_Issues#file-not-utf8

Otherwise it looks fine.

+---------------------------------+
| This package is APPROVED by me. |
+---------------------------------+

Comment 11 Debarshi Ray 2008-08-09 13:08:50 UTC
Also replace %{buildroot} with %%{buildroot} in the %changelog stanza to avoid rpmlint warnings on the SRPM.

Comment 12 Debarshi Ray 2008-10-14 21:28:46 UTC
Ping?

Is there anything which is stopping us from closing this review?

Comment 13 Jindrich Novy 2008-10-15 04:46:59 UTC
Nope, everything seems to be fixed now. Thanks.


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