Bug 548092
| Summary: | Review Request: shared-color-targets - Color targets from vendors for color calibration | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Richard Hughes <richard> |
| Component: | Package Review | Assignee: | Peter Lemenkov <lemenkov> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | fedora-package-review, lemenkov, notting, rhughes |
| Target Milestone: | --- | Flags: | lemenkov:
fedora-review+
kevin: 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: | 2009-12-21 22:03:27 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Richard Hughes
2009-12-16 15:23:10 UTC
I'll review it. 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.
(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 (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. (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. Ok, APPROVED 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 cvs done. Built, thanks! |