Bug 1178911 - Review Request: cairo-dock - Light eye-candy fully themable animated dock
Summary: Review Request: cairo-dock - Light eye-candy fully themable animated dock
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jan Pokorný [poki]
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1178912
TreeView+ depends on / blocked
 
Reported: 2015-01-05 15:59 UTC by Mamoru TASAKA
Modified: 2015-04-20 11:53 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-27 18:04:28 UTC
jpokorny: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1180723 None None None Never

Internal Links: 1180723

Description Mamoru TASAKA 2015-01-05 15:59:24 UTC
Spec URL: https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock.spec
SRPM URL: https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-3.4.0-9.fc.src.rpm
Description: 
An light eye-candy fully themable animated dock for any 
Linux desktop. It has a family-likeness with OSX dock,
but with more options.

Fedora Account System Username: mtasaka

Background:
cairo-dock is now on rpmfusion. This was on Fedora, however on Feb 2009 this was removed from Fedora due to may-be-patent-infringement issue. Now with the upstream's advice I removed that "may-be" part from the original tarball. I talked with spot and this srpm got "legally okay" for Fedora.

Copr:
http://copr-fe.cloud.fedoraproject.org/coprs/mtasaka/cairo-dock0test/

Comment 1 Mamoru TASAKA 2015-01-05 16:08:38 UTC
rpmlint:
cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-plug-ins-gecko
cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-themes
- These are some old age Obsoletes.

cairo-dock-core.x86_64: E: incorrect-fsf-address /usr/share/licenses/cairo-dock-core/copyright
- I will tell the upstream together with cairo-dock-plug-ins source later

cairo-dock-devel.x86_64: W: only-non-binary-in-usr-lib
- This is usual

All the rest should be ignorable.


Note that "cairo-dock" is a metapackage, which also depends on cairo-dock-plug-ins{,-python2} (ref: bug 1178912), however I can cope with
- build cairo-dock srpm
- build cairo-dock-plug-ins srpm after cairo-dock rebuilt
- submit cairo-dock, cairo-dock-plug-ins to updates request altogether
and no bootstrapping is needed.

Comment 2 Jan Pokorný [poki] 2015-01-16 13:19:17 UTC
Mamoru-san,

I'm about to review cairo-dock{,-plug-ins}. Can you review clufter
[bug 1180723] in exchange, please?

Comment 3 Mamoru TASAKA 2015-01-16 14:22:12 UTC
Thank you for taking. I took your review request in exchange.

Comment 4 Jan Pokorný [poki] 2015-01-16 17:58:37 UTC
Briefly:
- minor BSD-licensed file discovered
- some dates in changelog would deserve fixing

Below I commented also on some items that are OK:


> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> 
> Issues:
> =======
> - 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 is included in %doc.
>   Note: Cannot find LICENSE in rpm(s)
>   See:
>   http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
> - Package do not use a name that already exist
>   Note: A package already exist with this name, please check
>   https://admin.fedoraproject.org/pkgdb/acls/name/cairo-dock
>   See:
>   https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Conflicting_Package_Names

I am not sure here, I suppose the existing orhpaned package will get un-retired
(https://admin.fedoraproject.org/pkgdb/package/cairo-dock/)

> 
> ===== MUST items =====
> 
> C/C++:
> [x]: Package does not contain kernel modules.
> [x]: Package contains no static executables.
> [x]: Development (unversioned) .so files in -devel subpackage, if present.
>      Note: Unversioned so-files in private %_libdir subdirectory (see
>      attachment). Verify they are not in ld path.

cairo-dock-core: /usr/lib64/cairo-dock/libcd-Help.so

... but this is not a development .so, and is correctly in private subdir

Furthermore, CMakeList.txt makes it clear it is a special plugin for cairo-dock
that cannot be packaged in extra plug-ins package.

> [x]: Header files in -devel subpackage, if present.
> [x]: ldconfig called in %post and %postun if required.
> [x]: Package does not contain any libtool archives (.la)
> [x]: Rpath absent or only used for internal libs.
> 
> Generic:
> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.

You mentioned there were some concerns in the past but now it was ACK'd
by spot.  It would be best if it there was a public record of that, couldn't
find anything on fedora-legal/fedora-devel MLs.  Only that it is better to
have a transparency in this regard.

IIUIC, the patent stuff is being removed prior to creating the resulting
source tarball thanks cairo-dock-create-fedora-tarball.sh.

> [!]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses found:
>      "GPL (v2 or later)", "LGPL (v2 or later)", "GPL (unversioned/unknown
>      version) GPL (v3)", "GPL (v3 or later)", "Unknown or generated". 13 files
>      have unknown license. Detailed output of licensecheck in /home/mock
>      /cairo-dock/1178911-cairo-dock/cairo-dock/licensecheck.txt

Everything seems OK except for BSD license of
    cairo-dock-3.4.0/cmake_modules/GNUInstallDirs.cmake
which refers to BSD license of CMake (and, as per instructions in the
header, it should be included in full in that very file!), which turns
to be BSD 3-Clause "New" or "Revised" license.

Hence I think the license should be: GPLv3+ and BSD

Alternatively, a fact that native CMake version exists can be utilized:
$ rpm -qf /usr/share/cmake/Modules/GNUInstallDirs.cmake
cmake-3.0.2-2.fc21.x86_64

> [ ]: License file installed when any subpackage combination is installed.

This doesn't seem to be true, e.g., one may install cairo-dock-libs and
miss the licenses/* files.

> [x]: %build honors applicable compiler flags or justifies otherwise.

Perhaps %cmake macro is robust enough so thar -O3 -> -O2 in CMakeLists.txt
is not necessary?

> [x]: Package contains no bundled libraries without FPC exception.
> [ ]: Changelog in prescribed format.

OK except rpmlint reports:

cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Thu Dec 21 2011 Mamoru Tasaka <mtasaka@fedoraproject.org> - 2.4.0.2-2
cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Sat Nov  6 2009 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 2.1.1.2-1
cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Wed May 27 2008 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 1.5.6-1.date20080528

> [x]: Sources contain only permissible code or content.
> [x]: Each %files section contains %defattr if rpm < 4.4
>      Note: %defattr present but not needed
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> [x]: Development files must be in a -devel package
> [x]: Package uses nothing in %doc for runtime.
> [x]: The spec file handles locales properly.
> [x]: Package consistently uses macros (instead of hard-coded directory names).

(/sbin/ldconfig based on
https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries
contradicts a bit, and there is no macro for that command)

> [x]: Package is named according to the Package Naming Guidelines.
> [x]: Package does not generate any conflict.
> [x]: Package obeys FHS, except libexecdir and /usr/target.
> [-]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> [x]: Requires correct, justified where necessary.

note wrt. cairo-dock, one can still install just cairo-dock-{core,libs} and
then particular plugins, so this is OK

> [x]: Spec file is legible and written in American English.
> [-]: Package contains systemd file(s) if in need.
> [x]: Useful -debuginfo package or justification otherwise.
> [x]: Package is not known to require an ExcludeArch tag.
> [x]: Large documentation must go in a -doc subpackage. Large could be size
>      (~1MB) or number of files.
>      Note: Documentation size is 30720 bytes in 3 files.
> [x]: Package complies to the Packaging Guidelines
> [x]: Package successfully compiles and builds into binary rpms on at least one
>      supported primary architecture.
> [x]: Rpmlint is run on all rpms the build produces.
>      Note: There are rpmlint messages (see attachment).
> [x]: Package requires other packages for directories it uses.
> [x]: Package must own all directories that it creates.
> [x]: Package does not own files or directories owned by other packages.
> [x]: All build dependencies are listed in BuildRequires, except for any that
>      are listed in the exceptions section of Packaging Guidelines.
> [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
> [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
> [x]: Macros in Summary, %description expandable at SRPM build time.
> [x]: Package contains desktop file if it is a GUI application.
> [x]: Package installs a %{name}.desktop using desktop-file-install or desktop-
>      file-validate if there is such a file.
> [x]: Package does not contain duplicates in %files.
> [x]: Permissions on files are set properly.
> [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
>      work.
> [x]: Package is named using only allowed ASCII characters.
> [x]: Package is not relocatable.
> [x]: Sources used to build the package match the upstream source, as provided
>      in the spec URL.
> [x]: Spec file name must match the spec package %{name}, in the format
>      %{name}.spec.
> [x]: File names are valid UTF-8.
> [x]: Packages must not store files under /srv, /opt or /usr/local
> 
> ===== SHOULD items =====
> 
> Generic:
> [ ]: If the source package does not include license text(s) as a separate file
>      from upstream, the packager SHOULD query upstream to include it.

Related to the possible BSD issue (License field in the package spec...)

> [x]: Final provides and requires are sane (see attachments).
> [x]: Fully versioned dependency in subpackages if applicable.
>      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in cairo-dock-
>      libs , cairo-dock-core , cairo-dock-devel
> [x]: Package functions as described.
> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [x]: Scriptlets must be sane, if used.
> [x]: SourceX tarball generation or download is documented.
>      Note: Package contains tarball without URL, check comments
> [ ]: Description and summary sections in the package spec file contains
>      translations for supported Non-English languages, if available.

This sound merely like nice-to-have :)

> [ ]: Package should compile and build into binary rpms on all supported
>      architectures.

Haven't verified this.

> [ ]: %check is present and all tests pass.

Tests are present in tests subdir, but perhaps impractical to run.

> [x]: Packages should try to preserve timestamps of original installed files.
> [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: Reviewer should test that the package builds in mock.
> [x]: Buildroot is not present
> [x]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
> [x]: Dist tag is present (not strictly required in GL).
> [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
> [x]: Uses parallel make %{?_smp_mflags} macro.
> [x]: The placement of pkgconfig(.pc) files are correct.
> [x]: SourceX is a working URL.
> [x]: Spec use %global instead of %define unless justified.


> Rpmlint
> -------
> Checking: cairo-dock-3.4.0-9.fc21.x86_64.rpm
>           cairo-dock-libs-3.4.0-9.fc21.x86_64.rpm
>           cairo-dock-core-3.4.0-9.fc21.x86_64.rpm
>           cairo-dock-devel-3.4.0-9.fc21.x86_64.rpm
>           cairo-dock-3.4.0-9.fc21.src.rpm

> cairo-dock.x86_64: W: spelling-error Summary(en_US) themable -> them able, them-able, fathomable
> cairo-dock.x86_64: W: spelling-error %description -l en_US metapackage -> meta package, meta-package, prepackage
> cairo-dock-core.x86_64: W: spelling-error %description -l en_US themable -> them able, them-able, fathomable

> cairo-dock.x86_64: E: no-binary

This cannot be noarch as then no subpackage cannot be reverted to "arch"
(was experimenting with something similar for clufter).

> cairo-dock.x86_64: W: no-documentation
> cairo-dock-libs.x86_64: W: no-documentation
> cairo-dock-devel.x86_64: W: no-documentation

> cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-plug-ins-gecko
> cairo-dock-core.x86_64: W: obsolete-not-provided cairo-dock-themes

justified

> cairo-dock-core.x86_64: E: incorrect-fsf-address /usr/share/licenses/cairo-dock-core/copyright

OK for now, to be discussed with upstream as per [comment 1].

> cairo-dock-devel.x86_64: W: only-non-binary-in-usr-lib

non-versioned lib symlink to versioned one

> cairo-dock.src: W: spelling-error Summary(en_US) themable -> them able, them-able, fathomable
> cairo-dock.src: W: spelling-error %description -l en_US metapackage -> meta package, meta-package, prepackage

not sure if there is any strong preference in other packages; themable OK
(~parsable, movable).

> cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Thu Dec 21 2011 Mamoru Tasaka <mtasaka@fedoraproject.org> - 2.4.0.2-2
> cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Sat Nov  6 2009 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 2.1.1.2-1
> cairo-dock.src: E: specfile-error warning: bogus date in %changelog: Wed May 27 2008 Mamoru Tasaka <mtasaka@ioa.s.u-tokyo.ac.jp> - 1.5.6-1.date20080528

might be worth fixing otherwise it will be reported over and over in any
(local) rebuild, etc.

> 5 packages and 0 specfiles checked; 5 errors, 11 warnings.

Comment 5 Jan Pokorný [poki] 2015-01-16 18:02:41 UTC
> Note that "cairo-dock" is a metapackage, which also depends on
> cairo-dock-plug-ins{,-python2} (ref: bug 1178912), however I can cope
> with
> - build cairo-dock srpm
> - build cairo-dock-plug-ins srpm after cairo-dock rebuilt
> - submit cairo-dock, cairo-dock-plug-ins to updates request altogether
> and no bootstrapping is needed.

Maybe worth documenting directly in the specfile, or perhaps even better,
in some MAINTAINERS.fedora file in the upstream tree.

Personally, I don't have experience with such a setup (used mockchain
locally) so hopefully it is acceptable and optimal.

Comment 6 Mamoru TASAKA 2015-01-19 07:07:13 UTC
Thank you for initial comments!!
First I write some reply to your comments

> I am not sure here, I suppose the existing orhpaned package will get un-retired
(https://admin.fedoraproject.org/pkgdb/package/cairo-dock/)
Yes.

> You mentioned there were some concerns in the past but now it was ACK'd
by spot.  It would be best if it there was a public record of that, couldn't
find anything on fedora-legal/fedora-devel MLs.
Well, there is no public record. All spot and my exchange was in private. I am not sure if I can make these show in public (as this is very legal issue), so for now I put them in private (but I can write the summary of the mail, will add)

> IIUIC, the patent stuff is being removed prior to creating the resulting
source tarball thanks cairo-dock-create-fedora-tarball.sh.
Yes.

> Hence I think the license should be: GPLv3+ and BSD
GPLv3+ is "more strict" than BSD (and BSD is compatible with GPL) so it is okay to write "GPLv3+" only:
https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F

> This doesn't seem to be true, e.g., one may install cairo-dock-libs and
miss the licenses/* files.
Yes, this must need fixing. Will fix in the next push

> Perhaps %cmake macro is robust enough so thar -O3 -> -O2 in CMakeLists.txt
is not necessary?
Well, for gcc compiler flags related to optimization level, the latter always wins (i.e. gcc -O2 -O3 is gcc -O3). So it is safer to change -O3 in source code to -O2 explicitly.

> For %changelog dates rpmlint
- Well, I don't rememeber what date is correct any longer... So although it is true that date is wrong in some way, I don't want to fix them.

> Personally, I don't have experience with such a setup (used mockchain
locally) so hopefully it is acceptable and optimal.
Well, for this build bootstrap is not difficult. Just building in the order of cairo-dock -> cairo-dock-plug-ins (srpm) is okay.

Other comments seems okay. I will fix
- license files placement (should be moved to -libs)
- write some brief explanation about legal issue.

Comment 7 Jan Pokorný [poki] 2015-01-19 13:05:33 UTC
>> You mentioned there were some concerns in the past but now it was ACK'd
>> by spot.  It would be best if it there was a public record of that,
>> couldn't find anything on fedora-legal/fedora-devel MLs.
> Well, there is no public record. All spot and my exchange was in private.
> I am not sure if I can make these show in public (as this is very legal
> issue), so for now I put them in private (but I can write the summary
> of the mail, will add)

Oh, don't want to cause more issues as opposed to just have somehow
confirmed any previous legal issue was overcome successfully.

That being said, Tom/spot, can you chime in to verify the statement,
please?

(The only reference I could find is
https://fedoraproject.org/w/index.php?title=Deprecated_packages&oldid=327569
).

Comment 8 Tom "spot" Callaway 2015-01-19 15:10:38 UTC
Yes, Mamoru has done the necessary work in conjunction with upstream to resolve the legal issues which blocked this package from Fedora before.

Comment 9 Jan Pokorný [poki] 2015-01-19 15:18:48 UTC
Thanks, spot!


Mamoru, some more points below:

Regarding the License tag vs BSD: there was an apparent mistake on my
side as I was considering all files within a source RPM, not binary RPMs.
"Better safe than sorry", but needless exercise (well, not sure how
common/sane is to relay on customizations of standard CMake modules,
but this is an upstream question).


>> For %changelog dates rpmlint
> - Well, I don't rememeber what date is correct any longer...
> So although it is true that date is wrong in some way, I don't want to
> fix them.

Accepted, I only wanted to point out that it is relatively a new feature
of RPM to complain any time a specfile with iffy dates is being parsed
and that this dirt under the carpet will keep exposing itself over
and over.


Could I please know your standpoint on:
- %check phase
- documenting the build order wrt. cairo-dock-plug-ins
  - side-question: will this get right automatically during mass rebuilds?

In the light of this review, I'll jump briefly on cairo-dock-plug-ins
part.

Comment 10 Mamoru TASAKA 2015-01-20 05:54:38 UTC
Well
%check
- There are some test scripts under tests, however this depends on 
  -plug-ins, and moreover proper dbus session needs running, so
  I don't think it is suitable.

- Build order
  - Not difficult and I don't think it needs documentation.
    For mass rebuild no build order need consideration.

Comment 11 Mamoru TASAKA 2015-01-20 06:22:58 UTC
https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock.spec
https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-3.4.0-10.fc.src.rpm

* Tue Jan 20 2015 Mamoru TASAKA <mtasaka@fedoraproject.org> - 3.4.0-10
- Move license files to -libs
- Add an explanation text for legal issue

Comment 12 Jan Pokorný [poki] 2015-01-21 13:56:59 UTC
Looks good to me except for one new thing regarding core/libs split:

/usr/share/cairo-dock/scripts/cairo-dock-package-theme.sh should IMHO be
packaged in cairo-dock-libs binary RPM, which contains the user of
this script (libgldi.so*)

When the split is in place, it's reasonable to make it follow the actual
inter-dependencies.  There may be more such cases, but that can be figured
out later on, it should be quite rare.

Comment 13 Mamoru TASAKA 2015-01-21 14:19:54 UTC
(In reply to Jan Pokorný from comment #12)
> Looks good to me except for one new thing regarding core/libs split:
> 
> /usr/share/cairo-dock/scripts/cairo-dock-package-theme.sh should IMHO be
> packaged in cairo-dock-libs binary RPM, which contains the user of
> this script (libgldi.so*)
> 
> When the split is in place, it's reasonable to make it follow the actual
> inter-dependencies.  There may be more such cases, but that can be figured
> out later on, it should be quite rare.

-libs usually contains "libraries" only avoid multilib issue when installing -devel package and does not contain any runtime files.

Comment 14 Jan Pokorný [poki] 2015-01-21 14:29:55 UTC
OK, not disputing it then.
ACK, -plug-ins review is in the works.

Comment 15 Jan Pokorný [poki] 2015-01-22 16:54:38 UTC
(After-the-fact) suggestion:
- %defattr(-,root,root,-) should not be needed as it is a default
  (similar to %doc/[1180723 comment 12]).

Comment 16 Jan Pokorný [poki] 2015-01-22 16:55:29 UTC
([bug 1180723 comment 12])

Comment 17 Jan Pokorný [poki] 2015-01-22 19:19:14 UTC
(After-the-fact) suggestion #2:

> Packages containing graphical applications should include AppData files.

https://fedoraproject.org/wiki/Packaging:AppData

Comment 18 Mamoru TASAKA 2015-01-23 07:26:05 UTC
Thank you!
For appdata, I will ask the upstream first.

For SCM request, I will do once -plug-ins is also approved.

Comment 19 Mamoru TASAKA 2015-02-26 05:26:14 UTC
Since plug-ins review also passed (bug 1178912), now I do scm request

Un-orphaning request

Package Change Request
======================
Package Name: cairo-dock
New Branches: devel f22 f21 f20
Owners: mtasaka
InitialCC:

Comment 20 Gwyn Ciesla 2015-02-26 13:32:02 UTC
Git done (by process-git-requests).

Unretired.

Comment 21 Mamoru TASAKA 2015-02-27 18:04:28 UTC
Rebuilt on all branches, push request on F-22/21/20, now closing.

Thank you for review and git preocedure.


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