Bug 493684

Upstream notified about the Version and License issues by mail.

Licensing and versioning issues resolved.

SPEC: http://v3.sk/~lkundrak/SPECS/clearlooks-compact-gnome-theme.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/clearlooks-compact-gnome-theme-1.4-1.el5.src.rpm

*** Bug 495072 has been marked as a duplicate of this bug. ***

You package has a couple of issues that it does not make much sense to review it at this point:

$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/clearlooks-compact-gnome-theme-1.4-2.fc11.*
clearlooks-compact-gnome-theme.noarch: W: incoherent-version-in-changelog 1.4-1 ['1.4-2.fc11', '1.4-2']
clearlooks-compact-gnome-theme.noarch: W: no-documentation
clearlooks-compact-gnome-theme.src:16: W: unversioned-explicit-provides clearlooks-compact

Fix the changelog and include COPYING to make the the no docs warning disappear. Change the provides to
Provides:       clearlooks-compact = %{version}

The license is not clear to me. COPYING from the source is LGPLv2.1, but you say it's GPLv2+.

There is a new version available, see
http://martin.ankerl.com/files/ClearlooksCompact-1.5.tar.bz2

(In reply to comment #4)
> You package has a couple of issues that it does not make much sense to review
> it at this point:

Excuse me? What exactly makes this unreviewable.

> $ rpmlint
> /var/lib/mock/fedora-rawhide-i386/result/clearlooks-compact-gnome-theme-1.4-2.fc11.*
> clearlooks-compact-gnome-theme.noarch: W: incoherent-version-in-changelog 1.4-1
> ['1.4-2.fc11', '1.4-2']

Will fix.

> clearlooks-compact-gnome-theme.noarch: W: no-documentation

No problem, no documentation included by upstream.

> clearlooks-compact-gnome-theme.src:16: W: unversioned-explicit-provides
> clearlooks-compact
> 
> Fix the changelog and include COPYING to make the the no docs warning
> disappear. Change the provides to
> Provides:       clearlooks-compact = %{version}

Done.

> The license is not clear to me. COPYING from the source is LGPLv2.1, but you
> say it's GPLv2+.

Pardon me? I read this there:
License:        LGPLv2+

COPYING file does not determine which licensing terms are in use. Comments in files do. COPYING merely contains the license text.

> There is a new version available, see
> http://martin.ankerl.com/files/ClearlooksCompact-1.5.tar.bz2  

Updated.

SPEC: http://v3.sk/~lkundrak/SPECS/clearlooks-compact-gnome-theme.spec
SRPM:
http://v3.sk/~lkundrak/SRPMS/clearlooks-compact-gnome-theme-1.5-1.fc11.src.rpm

Looks good, you probably shouldn't install COPYING twice.

(In reply to comment #6)
> Looks good, you probably shouldn't install COPYING twice.  

Good point. Will fix on next package revision, which will probably take place after a full review.

(In reply to comment #5)
> Excuse me? What exactly makes this unreviewable.

I did not say it was "unreviewable" but that a review at that point did "not make much sense". The package contained a blocker (COPYING not included), but the main reason was that it was not the latest version. We are reviewing SRPMs, not the specs, so strictly speaking after updating the package I would have to perform another review.
 
> No problem, no documentation included by upstream.

COPYING _is_ documentation.

> Pardon me? I read this there:
> License:        LGPLv2+

Sorry, I would have sworn the spec said GPLv2+. My bad.


REVIEW FOR af3e76647b483eb253542a7ce8220ad6  clearlooks-compact-gnome-theme-1.5-1.fc11.src.rpm

OK - MUST: rpmlint must be run on every package. The output should be posted in the review:
$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/clearlooks-compact-gnome-theme-1.5-1.fc11.*
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK - MUST: The package is named according to the Package Naming Guidelines.
OK - MUST: The spec file name matches the base package %{name}, in the format %{name}.spec.
OK - MUST: The package meets the Packaging Guidelines.
OK - MUST: The package is licensed with a Fedora approved license and meets the Licensing Guidelines: LGPLv2+
OK - MUST: The License field in the package spec file matches the actual license.
OK - MUST: The license file from the source package is included in %doc.
OK - MUST: The spec file is in American English.
OK - MUST: The spec file for the package is legible.
OK - MUST: The source used to build the package matches the upstream source by MD5 9d6f111db273f065cfd2e3ee5c390eac
OK - MUST: The package successfully compiles and builds into binary rpms on i386
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires (none).
N/A - MUST: The spec file handles locales properly with the %find_lang macro.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: The package owns all directories that it creates.
FAIL - MUST: The package contains duplicate files in the %files listing: COPYING
OK - MUST: Permissions on files are set properly. Every %files section includes a %defattr(...) line.
OK - MUST: The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines.
OK - MUST: The package contains code, or permissable content.
N/A - MUST: Large documentation files should go in a -doc subpackage.
N/A - MUST: Files included as %doc do not affect the runtime of the application.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
OK - MUST: The package does not contain any .la libtool archives.
N/A - MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section.
OK - MUST: The packages does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
OK - MUST: All filenames in rpm packages are valid UTF-8.


SHOULD Items:
N/A - SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: The the package builds in mock.
OK - SHOULD: The package should compile and build into binary rpms on all supported architectures (noarch).
OK - SHOULD: The package functions as described.
N/A - SHOULD: If scriptlets are used, those scriptlets must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.


Package is APPROVED, but please don't forget to remove the 2nd COPYING after import into CVS.

Thanks a lot for review, Christoph!

(In reply to comment #8)
> (In reply to comment #5)
> > Excuse me? What exactly makes this unreviewable.
> 
> I did not say it was "unreviewable" but that a review at that point did "not
> make much sense". The package contained a blocker (COPYING not included), but
> the main reason was that it was not the latest version. We are reviewing SRPMs,
> not the specs, so strictly speaking after updating the package I would have to
> perform another review.

We don't do new reviews for new versions once the package is CVS anyways. It would surely block approval, but I don't see why would it block doing a full review. Reviewer's choice anyway

> > No problem, no documentation included by upstream.
> 
> COPYING _is_ documentation.

It was not present in 1.4. In fact, 1.4 was the latest version at the time the review request was last updated (in fact I made upstream release that version to address our concerns).

> Package is APPROVED, but please don't forget to remove the 2nd COPYING after
> import into CVS.

Sure. :)

New Package CVS Request
=======================
Package Name: clearlooks-compact-gnome-theme
Short Description: GNOME Desktop theme optimized for small displays
Owners: sindrepb, lkundrak
Branches: F-10

(In reply to comment #9)
> Thanks a lot for review, Christoph!

NP, you are welcome.

> We don't do new reviews for new versions once the package is CVS anyways.

That's what commits-list is for. If a maintainer does something really stupid upon an update, (hopefully) someone reading the commits will recognize it.

cvs done.

This package as built in koji is not x86_64 friendly. It apparently doesn't look for the 64bit version of libclearlooks.so and wants to pull in a bunch of i586 packages. Log attached.

Created attachment 339322 [details]
log of attempted yum install

Looks like a yum issue to me. I think we should require gtk2-engines instead of libclearlooks.so for now.

Must be a yum issue. I nodep installed it and it seems to work fine.

(In reply to comment #14)
> Looks like a yum issue to me. I think we should require gtk2-engines instead of
> libclearlooks.so for now.  

No yum issue but packaging bug (in clearlooks-compact-gnome-theme-1.5-1.fc11).
64 bit gtk2-engines Provides libclearlooks.so()(64bit), not
libclearlooks.so.

Imported and built