Bug 548092 - Review Request: shared-color-targets - Color targets from vendors for color calibration
Review Request: shared-color-targets - Color targets from vendors for color c...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-16 10:23 EST by Richard Hughes
Modified: 2009-12-21 17:03 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-21 17:03:27 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
lemenkov: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Richard Hughes 2009-12-16 10:23:10 EST
Spec URL: http://people.freedesktop.org/~hughsient/temp/shared-color-targets.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/shared-color-targets-0.0.1-0.6.20091216git.fc12.src.rpm
Description: shared-color-targets contains target files for popular scanner calibration targets.

These are used in gnome-color-manager to create accurate color calibration profiles when using pre-printed targets from various vendors.

See http://blogs.gnome.org/hughsie/2009/12/16/advances-of-freedom/ for why they are important. Thanks.
Comment 1 Peter Lemenkov 2009-12-18 04:30:06 EST
I'll review it.
Comment 2 Peter Lemenkov 2009-12-18 04:59:44 EST
REVIEW:

+ rpmlint is silent

[petro@Sulaco SPECS]$ rpmlint ../RPMS/noarch/shared-color-targets-0.0.1-0.6.20091216git.fc12.noarch.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[petro@Sulaco SPECS]$

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.

- I've got few complaints about file contents directory layout. 

-- First, nobody owns dir %{_datadir}/color/targets. This issue must be fixed.
-- Second, I don't see necessity of creating {_datadir}/shared-color-targets/wolf_faust just for storing LICENSE and README. Just mark them as %doc. If you wish to reflect the fact, that these two files are relevant to wolf_faust, then just rename them into something like README.wolf_faust and LICENSE.wolf_faust (and, after that, just mark them as %doc). Also it resolves the issue with inclusion of files with licensing info (see note below)
-- I'm not sure, that we need to package ChangeLog at all - it contains only technical data, regarding repository changes.


+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.

- The License field in the package spec file contains wrong data. You forgot to add CC-BY-SA (see LICENSE file in {_datadir}/shared-color-targets/wolf_faust directory). Also I don't find any traces of "Public Domain" content. Perhaps, test.it8 is licensed under this license?

- The file, containing the text of the license(s) for the package, MUST  be included in %doc. See note above - you must mark LICENSE file for Wolf Faust's work as %doc.

+ The spec file is written in American English.
+ The spec file for the package is legible.

- The sources used to build the package, MUST match the upstream source, as provided in the spec URL. I've got 404 while trying to D/L Source0.


+ The package successfully compiles and builds into binary rpms on at least one primary architecture.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files.
+ The package does NOT bundle copies of system libraries.
+ The package is not designed to be relocatable.

- The package MUST own all directories that it creates. See message above.

+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files with a suffix (e.g. libfoo.so.1.1).
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8.

Ok, here is a TODO list:

* Provide downloadable Source0
* Fix License field in spec-file
* Package must own %{_datadir}/color/targets dir
* Relocate docs related to Wolf Faust's work.
Comment 3 Richard Hughes 2009-12-18 06:02:33 EST
(In reply to comment #2)
> * Provide downloadable Source0

Done.

> * Fix License field in spec-file

Done

> * Package must own %{_datadir}/color/targets dir

Done

> * Relocate docs related to Wolf Faust's work.  

Not done. Programs such as gnome-color-manager are expecting them to be in /usr/share/share-color-targets/$(VENDOR)/{README|LICENSE} -- this is so the user can click a link to the target readme or licence in the GUI and it will be displayed automatically. They shouldn't be listed as doc files as if they are missing then clients can't display the licence of the targets, which is bad.

I hope this is okay, as functionality in the GUI client depends on the fact they are present, and named in this way.

Richard.

New spec file: http://people.freedesktop.org/~hughsient/temp/shared-color-targets.spec
New SRPM: http://people.freedesktop.org/~hughsient/temp/shared-color-targets-0.0.1-0.7.20091218git.fc12.src.rpm
Comment 4 Peter Lemenkov 2009-12-18 06:35:42 EST
(In reply to comment #3)

+ The sources used to build the package, match the upstream source, as provided in the spec URL.

[petro@Sulaco SOURCES]$ sha256sum shared-color-targets-0.0.1.tar.gz*
47d52e4f4c21f88f71cb0332d7b07702c3efbb5fc24afe8b6022000e50db2e49  shared-color-targets-0.0.1.tar.gz
47d52e4f4c21f88f71cb0332d7b07702c3efbb5fc24afe8b6022000e50db2e49  shared-color-targets-0.0.1.tar.gz.1
[petro@Sulaco SOURCES]$ 

> > * Relocate docs related to Wolf Faust's work.  
> 
> Not done. Programs such as gnome-color-manager are expecting them to be in
> /usr/share/share-color-targets/$(VENDOR)/{README|LICENSE} -- this is so the
> user can click a link to the target readme or licence in the GUI and it will be
> displayed automatically. They shouldn't be listed as doc files as if they are
> missing then clients can't display the licence of the targets, which is bad.
> 
> I hope this is okay, as functionality in the GUI client depends on the fact
> they are present, and named in this way.
> 
> Richard.
> 
> New spec file:
> http://people.freedesktop.org/~hughsient/temp/shared-color-targets.spec
> New SRPM:
> http://people.freedesktop.org/~hughsient/temp/shared-color-targets-0.0.1-0.7.20091218git.fc12.src.rpm  

Ok, understood. You need to own %{_datadir}/shared-color-targets directory - that's the only issue remaining.
Comment 5 Richard Hughes 2009-12-18 07:27:20 EST
(In reply to comment #4)
> Ok, understood. You need to own %{_datadir}/shared-color-targets directory -
> that's the only issue remaining.  

Eek, forgot that -- apologies. Fixed now.

New spec file: http://people.freedesktop.org/~hughsient/temp/shared-color-targets.spec
New SRPM: http://people.freedesktop.org/~hughsient/temp/shared-color-targets-0.0.1-0.8.20091218git.fc12.src.rpm  

Richard.
Comment 6 Peter Lemenkov 2009-12-18 07:36:34 EST
Ok,

APPROVED
Comment 7 Richard Hughes 2009-12-18 07:56:58 EST
New Package CVS Request
=======================
Package Name: shared-color-targets
Short Description: shared-color-targets contains target files for popular scanner
calibration targets.
Owners: rhughes
Branches: F-12
InitialCC: rhughes
Comment 8 Kevin Fenzi 2009-12-21 14:43:02 EST
cvs done.
Comment 9 Richard Hughes 2009-12-21 17:03:27 EST
Built, thanks!

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