Bug 590387 - Review Request: lcms2 - Color Management System
Review Request: lcms2 - Color Management System
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chen Lei
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-09 06:16 EDT by Richard Hughes
Modified: 2011-11-17 13:47 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-06-21 04:17:24 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
supercyper1: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Richard Hughes 2010-05-09 06:16:09 EDT
Spec URL: http://people.freedesktop.org/~hughsient/temp/lcms2.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/lcms2-2.0-1.fc13.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2175169

Description: LittleCMS intends to be a small-footprint, speed optimized color management engine in open source form. LCMS2 is the current version of LCMS, and can be parallel installed with the original (deprecated) lcms.

Note:

It's important to get this package into Fedora before F14 as applications are being encouraged to switch to lcms2 as lcms is now maintained and unloved. Future versions of gnome-color-manager are going to depend on lcms2 for the API additions and speed increases.
Comment 1 Chen Lei 2010-06-18 07:18:06 EDT
I suggest to move shlibs to the mainpackage, then create -utils subpackage.

From http://www.littlecms.com, you'll find the main role of lcms2 is shlibs instead of applications.
Comment 2 Chen Lei 2010-06-18 07:28:44 EDT
Some more suggestions:

1. Update to the latest 2.0a

2. BuildRequires:  pkgconfig and Requires:  pkgconfig is not needed now, rpmbuild will add pkgconfig as dependency if .pc files exist.

3.BuildRequires:  swig >= 1.3.12 seems not needed, upstream tarball not include any wrappers currently.
Comment 3 Richard Hughes 2010-06-18 07:51:41 EDT
Good review points, thanks.

New packages here:

Spec URL: http://people.freedesktop.org/~hughsient/temp/lcms2.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/lcms2-2.0-2.fc13.src.rpm
Comment 4 Chen Lei 2010-06-18 08:27:16 EDT
rpmlint *.rpm
lcms2.src: I: enchant-dictionary-not-found en_US
lcms2.src: W: non-standard-group Unspecified
lcms2.src: W: no-cleaning-of-buildroot %clean
lcms2.src: W: no-buildroot-tag
lcms2.src: W: no-%clean-section
lcms2.x86_64: W: non-standard-group Unspecified
lcms2.x86_64: W: spurious-executable-perm /usr/share/doc/lcms2-2.0/AUTHORS
lcms2.x86_64: W: spurious-executable-perm /usr/share/doc/lcms2-2.0/COPYING
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmswtpnt.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/transicc/transicc.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmserr.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsxform.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsplugin.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsgmt.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmscnvrt.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/include/lcms2_plugin.h
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/common/utils.h
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsio1.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmstypes.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/common/vprf.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsopt.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/jpgicc/iccjpeg.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmspcs.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/common/xgetopt.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmspack.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsps2.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsio0.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsgamma.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsvirt.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmssamp.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/jpgicc/jpgicc.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/psicc/psicc.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsnamed.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsintrp.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/linkicc/linkicc.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmssm.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmscgats.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsmd5.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmscam02.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/utils/tificc/tificc.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmslut.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/lcms2_internal.h
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/src/cmsmtrx.c
lcms2-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/lcms-2.0/include/lcms2.h
lcms2-utils.x86_64: W: non-standard-group Unspecified
lcms2-utils.x86_64: W: spurious-executable-perm /usr/share/doc/lcms2-utils-2.0/AUTHORS
lcms2-utils.x86_64: W: manual-page-warning /usr/share/man/man1/jpgicc2.1.gz 40: warning: `p' not defined
lcms2-utils.x86_64: W: spurious-executable-perm /usr/share/doc/lcms2-utils-2.0/COPYING
lcms2-utils.x86_64: W: manual-page-warning /usr/share/man/man1/tificc2.1.gz 52: warning: `p' not defined
lcms2-utils.x86_64: W: no-manual-page-for-binary psicc2
lcms2-utils.x86_64: W: no-manual-page-for-binary linkicc2
lcms2-utils.x86_64: W: no-manual-page-for-binary transicc2
 
 non-standard-group Unspecified, spurious-executable-perm and no-%clean-section(if you want to push lcms2 to stable branches or EL6) should be fixed.
formal review here:
+:ok, =:needs attention, -:needs fixing

MUST Items:
[+] MUST: rpmlint must be run on every package.
[+] 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. [FIXME?: covers this
list and more]
[+] MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual
license.
[+] MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that 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,
as provided in the spec URL.
<<md5sum checksum>>c4f115462a7a5b306c247d018d7a8982
[+] MUST: The package must successfully compile and build into binary rpms on
at least one supported architecture.
[+] MUST: All build dependencies must be listed in BuildRequires
[+] MUST: The spec file MUST handle locales properly. This is done by using the
%find_lang macro.
[+] MUST: Every binary RPM package which stores shared library files (not just
symlinks) in any of the dynamic linker's default paths, must call ldconfig in
%post and %postun.
[+] MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[-] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every %files section must include a
%defattr(...) line.
[-] MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros, as described in the macros
section of Packaging Guidelines.
[+] MUST: The package must contain code, or permissible content. This is
described in detail in the code vs. content section of Packaging Guidelines.
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[+] MUST: Packages must NOT contain any .la libtool archives, these should be
removed in the spec.
[+] 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.
[+] 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 Items:
[+] 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.
[=] SHOULD: The description and summary sections in the package spec file
should contain translations for supported Non-English languages, if available.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all
supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[+] 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.


Some trival issue:
1.Version:        2.0 -> Version:        2.0a, in this case non-numeric is permiteed.
2.fix some rpm warings, see above.
3. %doc AUTHORS COPYING in -utils is not needed.
4. I suggest to change Summary:        Color Management System to Summary:        Color Management Engine
Comment 5 Richard Hughes 2010-06-18 09:05:43 EDT
I think I've addressed most issues:

lcms2.i686: W: spelling-error %description -l en_US lcms -> lams, LCM, locums
lcms2-debuginfo.i686: W: spelling-error Summary(en_US) lcms -> lams, LCM, locums
lcms2-debuginfo.i686: W: spelling-error %description -l en_US lcms -> lams, LCM, locums
lcms2-utils.i686: W: spelling-error Summary(en_US) lcms -> lams, LCM, locums
lcms2-utils.i686: W: spelling-error %description -l en_US lcms -> lams, LCM, locums
lcms2-utils.i686: W: manual-page-warning /usr/share/man/man1/jpgicc2.1.gz 40: warning: macro `p' not defined
lcms2-utils.i686: W: manual-page-warning /usr/share/man/man1/tificc2.1.gz 52: warning: macro `p' not defined
lcms2-utils.i686: W: no-manual-page-for-binary psicc2
lcms2-utils.i686: W: no-manual-page-for-binary linkicc2
lcms2-utils.i686: W: no-manual-page-for-binary transicc2
lcms2.src: W: spelling-error %description -l en_US lcms -> lams, LCM, locums
5 packages and 0 specfiles checked; 0 errors, 11 warnings.

SPEC:  http://people.freedesktop.org/~hughsient/temp/lcms2.spec
SRPMS: http://people.freedesktop.org/~hughsient/temp/lcms2-2.0a-3.fc13.src.rpm
Comment 6 Chen Lei 2010-06-18 09:54:20 EDT
Some suggestions:
1.Change Group from Applications/Productivity to  System Environment/Librariesfor for main package.
Add Group: Applications/Productivity  for -utils subpackage
2.

# install docs as this is all we've got
install -D -m 644 doc/LittleCMS2.0\ tutorial.pdf $RPM_BUILD_ROOT/usr/share/doc/lcms2-devel-2.0/tutorial.pdf
install -D -m 644 doc/LittleCMS2.0\ API.pdf $RPM_BUILD_ROOT/usr/share/doc/lcms2-devel-2.0/api.pdf
install -D -m 644 doc/LittleCMS2.0\ Plugin\ API.pdf $RPM_BUILD_ROOT/usr/share/doc/lcms2-devel-2.0/plugin-api.pdf

%file devel
%{_datadir}/doc/lcms2-devel-2.0/*.pdf


Why not to simply use 
chmod -x doc/*.pdf 
%doc doc/*.pdf instead?
Comment 7 Richard Hughes 2010-06-18 10:21:19 EDT
(In reply to comment #6)
> Some suggestions:
> 1.Change Group from Applications/Productivity to  System
> Environment/Librariesfor for main package.
> Add Group: Applications/Productivity  for -utils subpackage

Done, new spec uploaded, same location.

> 2.
> Why not to simply use 
> chmod -x doc/*.pdf 
> %doc doc/*.pdf instead?    

The docs are not installed by the project, and I'm choosing to install them manually.

Richard.
Comment 8 Chen Lei 2010-06-18 10:27:02 EDT
(In reply to comment #7)
> > 2.
> > Why not to simply use 
> > chmod -x doc/*.pdf 
> > %doc doc/*.pdf instead?    
> The docs are not installed by the project, and I'm choosing to install them
> manually.
> Richard.    

%doc doc/*.pdf = cp -p doc/*.pdf %{buildroot}/%{name}-devel-%{version}/

Using %doc is perfered in packaging guideline, unless you want to install docs to some non-standard place, you can simply use %doc instead.

This package is approved.
Comment 9 Richard Hughes 2010-06-18 11:32:41 EDT
(In reply to comment #8)
> Using %doc is perfered in packaging guideline, unless you want to install docs
> to some non-standard place, you can simply use %doc instead.

Cool. I'm talking with the upstream maintainer about generating the content rather than shipping binary files.

> This package is approved.    

Thanks!
Comment 10 Richard Hughes 2010-06-18 11:35:28 EDT
New Package CVS Request
=======================
Package Name: lcms2
Short Description: Color Management Engine
Owners: rhughes
Branches: F-12 F-13 EL-5 EL-6
InitialCC: rhughes
Comment 11 Chen Lei 2010-06-18 11:46:00 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > Using %doc is perfered in packaging guideline, unless you want to install docs
> > to some non-standard place, you can simply use %doc instead.
> Cool. I'm talking with the upstream maintainer about generating the content
> rather than shipping binary files.
> > This package is approved.    
> Thanks!    

Distribute binary data files is not forbidden as I known :)

FYI, %file %{_datadir}/doc/* will also add doc flags to all files under this directoty, regardless you don't add %doc before those files.
Comment 12 Kevin Fenzi 2010-06-20 22:01:58 EDT
CVS done (by process-cvs-requests.py).
Comment 13 Richard Hughes 2010-06-21 04:17:24 EDT
Thanks guys!
Comment 14 Nicolas Chauvet (kwizart) 2010-06-21 05:26:14 EDT
@Richard,
Can you use install -p to prevent timestamp changes at %install step for lcms2 headers. There is also a need to verify  what is done in Makefile.

That may prevent multilib conflicts.

Thx
Comment 15 Chen Lei 2010-06-21 05:35:09 EDT
(In reply to comment #14)
> @Richard,
> Can you use install -p to prevent timestamp changes at %install step for lcms2
> headers. There is also a need to verify  what is done in Makefile.
> That may prevent multilib conflicts.
> Thx    

Agree, docs in -devel subpackage also need such tweak to avoid multilib conflict.

Also, it'll be better to use %{version} instead of hardcoded 2.0 in %install
 and %file.
Comment 16 Richard Hughes 2010-06-21 08:06:46 EDT
(In reply to comment #14)
> @Richard,
> Can you use install -p to prevent timestamp changes at %install step for lcms2
> headers. There is also a need to verify  what is done in Makefile.

Sure, if you're a proven packager, just go in and make that change to devel, and I'll sync to F-* on the next release. Otherwise I'll make the change locally and just tag when we get the next upstream release.

Richard.
Comment 17 Xavier Bachelot 2011-11-17 11:21:44 EST
Richard, you requested branches for EPEL 5 and 6, but the package has not been imported nor, indeed, built. Do you plan to build for EPEL sooner or later ? This is a dependency for java-1.7.0-openjdk.
Comment 18 Richard Hughes 2011-11-17 13:47:15 EST
Xavier, I intended to build these, but got lost along the way. If you want to maintain them in EPEL I would be happy for the help.

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