Bug 701031

Summary: Review Request: zeitgeist-datahub - The zeitgeist engine data logger
Product: [Fedora] Fedora Reporter: Deji Akingunola <dakingun>
Component: Package ReviewAssignee: Mario Blättermann <mario.blaettermann>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: alex, fedora-package-review, mario.blaettermann, notting, theo148
Target Milestone: ---Flags: mario.blaettermann: fedora-review+
j: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-05-13 14:26:11 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Bug Depends On:    
Bug Blocks: 701078    

Description Deji Akingunola 2011-04-30 13:18:30 UTC
Spec URL: http://deji.fedorapeople.org/zeitgeist-datahub.spec
SRPM URL: http://deji.fedorapeople.org/zeitgeist-datahub-0.7.0-1.fc15.src.rpm
Description: The datahub provides passive plugins which insert events into Zeitgeist

Comment 1 Mario Blättermann 2011-04-30 15:18:35 UTC
Please use macros consistently and as many as possible, as follows:

URL:		https://launchpad.net/%{name}
Source0:	http://launchpad.net/%{name}/0.7/%{version}/+download/%{name}-%{version}.tar.gz

%{_bindir}/%{name}
%{_mandir}/man1/%{name}.*
%{_sysconfdir}/xdg/autostart/%{name}.desktop

It's not needed to provide a https link to the homepage, http also works and leads to the https page anyway.

The ChangeLog is empty, should be dropped from %docs.

Please add a period after the description.

You have to drop glib2-devel and gettext from BuildRequires. They are recursive dependencies of gtk2-devel and intltool, respectively.

Comment 2 Mario Blättermann 2011-04-30 15:28:25 UTC
Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3040269

$ rpmlint -v *
zeitgeist-datahub.i686: I: checking
zeitgeist-datahub.i686: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging
zeitgeist-datahub.i686: I: checking-url https://launchpad.net/zeitgeist-datahub (timeout 10 seconds)
zeitgeist-datahub.i686: E: zero-length /usr/share/doc/zeitgeist-datahub-0.7.0/ChangeLog
zeitgeist-datahub.i686: W: non-conffile-in-etc /etc/xdg/autostart/zeitgeist-datahub.desktop
zeitgeist-datahub.src: I: checking
zeitgeist-datahub.src: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging
zeitgeist-datahub.src: I: checking-url https://launchpad.net/zeitgeist-datahub (timeout 10 seconds)
zeitgeist-datahub.src: I: checking-url http://launchpad.net/zeitgeist-datahub/0.7/0.7.0/+download/zeitgeist-datahub-0.7.0.tar.gz (timeout 10 seconds)
zeitgeist-datahub-debuginfo.i686: I: checking
zeitgeist-datahub-debuginfo.i686: I: checking-url https://launchpad.net/zeitgeist-datahub (timeout 10 seconds)
zeitgeist-datahub.spec: I: checking-url http://launchpad.net/zeitgeist-datahub/0.7/0.7.0/+download/zeitgeist-datahub-0.7.0.tar.gz (timeout 10 seconds)
3 packages and 1 specfiles checked; 1 errors, 3 warnings.


Seems to be OK so far, besides the empty ChangeLog.

Comment 3 Deji Akingunola 2011-04-30 17:33:18 UTC
(In reply to comment #1)
> Please use macros consistently and as many as possible, as follows:
> 
> URL:  https://launchpad.net/%{name}
> Source0:
> http://launchpad.net/%{name}/0.7/%{version}/+download/%{name}-%{version}.tar.gz
> 
> %{_bindir}/%{name}
> %{_mandir}/man1/%{name}.*
> %{_sysconfdir}/xdg/autostart/%{name}.desktop
> 
> It's not needed to provide a https link to the homepage, http also works and
> leads to the https page anyway.
> 
> The ChangeLog is empty, should be dropped from %docs.
> 
> Please add a period after the description.
> 
> You have to drop glib2-devel and gettext from BuildRequires. They are recursive
> dependencies of gtk2-devel and intltool, respectively.

I have dropped the empty ChangeLog file, replace the https link with the http one and put the period at the end of the description. I believe the rest if OK. 

Spec URL: http://deji.fedorapeople.org/zeitgeist-datahub.spec
SRPM URL: http://deji.fedorapeople.org/zeitgeist-datahub-0.7.0-2.fc15.src.rpm

Comment 4 Mario Blättermann 2011-04-30 18:58:00 UTC
(In reply to comment #3)

> I have dropped the empty ChangeLog file, replace the https link with the http
> one and put the period at the end of the description. I believe the rest if OK. 

No, unfortunately not:
(In reply to comment #1)
> Please use macros consistently and as many as possible, as follows:
> 
> URL:  https://launchpad.net/%{name}
>...
> 
> %{_bindir}/%{name}
> %{_mandir}/man1/%{name}.*
> %{_sysconfdir}/xdg/autostart/%{name}.desktop
> 
> ...
> You have to drop glib2-devel and gettext from BuildRequires. They are recursive
> dependencies of gtk2-devel and intltool, respectively.

Comment 5 Deji Akingunola 2011-05-02 02:26:19 UTC
I really appreciate you doing this review, but;

> No, unfortunately not:
> (In reply to comment #1)
> > Please use macros consistently and as many as possible, as follows:
> >
 
There is no inconsistency in using macros in the spec file, writing out the file names explicitly in the filelist is OK.

> > URL:  https://launchpad.net/%{name}
> >...
> > 
> > %{_bindir}/%{name}
> > %{_mandir}/man1/%{name}.*
> > %{_sysconfdir}/xdg/autostart/%{name}.desktop
> > 
> > ...

There is no guideline/rules against explicitly listing buildrequires, in fact it I know of a number of packagers who prefer it that way. The buildrequires for this package is small enough, that I will like to continue to list them all explicitly.

> > You have to drop glib2-devel and gettext from BuildRequires. They are recursive
> > dependencies of gtk2-devel and intltool, respectively.

Comment 6 Alex Lancaster 2011-05-02 07:53:59 UTC
Mario, have you formally taken this review?  If so, then the "?" review flag should be set, and it should be ASSIGNED to you.

Comment 7 Mario Blättermann 2011-05-02 18:23:05 UTC
(In reply to comment #6)
> Mario, have you formally taken this review?  If so, then the "?" review flag
> should be set, and it should be ASSIGNED to you.

Done.

Comment 8 Mario Blättermann 2011-05-05 19:24:52 UTC
Actually, I like to write a formal review now to approve your package, but it seems to be impossible to fetch the files from fedorapeople.org. Unfortunately, the Koji scratch build has crowded the spec and srpm. Seems to be a temporary problem on fedorapeople.org itself, because I cannot fetch other files, too. I try it again tomorrow.

Comment 9 Mario Blättermann 2011-05-06 17:59:20 UTC
Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3054989

$ rpmlint -v *
zeitgeist-datahub.i686: I: checking
zeitgeist-datahub.i686: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging
zeitgeist-datahub.i686: I: checking-url http://launchpad.net/zeitgeist-datahub (timeout 10 seconds)
zeitgeist-datahub.i686: W: non-conffile-in-etc /etc/xdg/autostart/zeitgeist-datahub.desktop
zeitgeist-datahub.src: I: checking
zeitgeist-datahub.src: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging
zeitgeist-datahub.src: I: checking-url http://launchpad.net/zeitgeist-datahub (timeout 10 seconds)
zeitgeist-datahub.src: I: checking-url http://launchpad.net/zeitgeist-datahub/0.7/0.7.0/+download/zeitgeist-datahub-0.7.0.tar.gz (timeout 10 seconds)
zeitgeist-datahub-debuginfo.i686: I: checking
zeitgeist-datahub-debuginfo.i686: I: checking-url http://launchpad.net/zeitgeist-datahub (timeout 10 seconds)
zeitgeist-datahub.spec: I: checking-url http://launchpad.net/zeitgeist-datahub/0.7/0.7.0/+download/zeitgeist-datahub-0.7.0.tar.gz (timeout 10 seconds)
3 packages and 1 specfiles checked; 0 errors, 3 warnings.


---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    LGPLv3+
[+] MUST: The License field in the package spec file must match the actual
license.
[+] MUST: The file containing the text of the license(s) for the package must
be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum *
    ebf822fc4aafbfe93784db60e1f9917a  zeitgeist-datahub-0.7.0.tar.gz
    ebf822fc4aafbfe93784db60e1f9917a  zeitgeist-datahub-0.7.0.tar.gz.packaged

[+] MUST: The package MUST successfully compile and build into binary rpms on
at least one primary architecture.
    - Succesful Koji build available, see above.
[.] MUST: If the package does not successfully compile, build or work on an
architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: The spec file MUST handle locales properly.
[.] MUST: If a package installs files below %{_datadir}/icons, the icon cache
must be updated.
[.] MUST: Packages storing shared library files (not just symlinks) must call
ldconfig in %post and %postun.
[.] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Packages must not provide RPM dependency information when that
information is not global in nature, or are otherwise handled.
[.] MUST: When filtering automatically generated RPM dependency information,
the filtering system implemented by Fedora must be used.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), ...
[.] MUST: devel packages must require the base package using a fully versioned
dependency.
[.] MUST: Packages must NOT contain any .la libtool archives.
[.] MUST: Packages containing GUI applications must include a %{name}.desktop
file
[.] MUST: .desktop files must be properly installed with desktop-file-install
in the %install section.
    The provided *.desktop file is not used in the usual way, to provide application launchers. It resides in the /etc/xdg/autostart folder and is for other purposes
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.


[.] SHOULD: If the source package does not include license text(s) as a
    separate file from upstream, the packager SHOULD query upstream...

[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: The reviewer should test that the package builds in mock.
    See Koji build above (which uses mock anyway)
[+] SHOULD: The reviewer should test that the package functions as described.
    I assume the packager has tested it.
[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin ...
[.] SHOULD: Your package should contain man pages for binaries/scripts.

----------------

PACKAGE APPROVED

----------------

Comment 10 Deji Akingunola 2011-05-11 19:09:51 UTC
New Package SCM Request
=======================
Package Name: zeitgeist-datahub
Short Description: The zeitgeist engine data logger
Owners: deji
Branches: f14 f15
InitialCC:

Comment 11 Jason Tibbitts 2011-05-12 16:39:01 UTC
Git done (by process-git-requests).

Comment 12 Deji Akingunola 2011-05-13 14:26:11 UTC
Package imported and build for rawhide and F15 (can't be built for F14 because of dependency issue).

Thanks to Mario for the review.