Bug 1178912 - Review Request: cairo-dock-plug-ins - Plug-ins files for Cairo-Dock
Summary: Review Request: cairo-dock-plug-ins - Plug-ins files for Cairo-Dock
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Jan Pokorný [poki]
QA Contact: Fedora Extras Quality Assurance
Depends On: 1178911
TreeView+ depends on / blocked
Reported: 2015-01-05 16:00 UTC by Mamoru TASAKA
Modified: 2015-04-20 11:53 UTC (History)
4 users (show)

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

Attachments (Terms of Use)

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

Internal Links: 1180723

Description Mamoru TASAKA 2015-01-05 16:00:15 UTC
Spec URL: https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins.spec
SRPM URL: https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins-3.4.0-9.fc.src.rpm
This package contains plug-ins files for Cairo-Dock.

Fedora Account System Username: mtasaka

Comment 1 Mamoru TASAKA 2015-01-05 16:10:03 UTC

incorrect-fsf-address: I will tell the upstream later, together with cairo-dock side.

Comment 2 Jan Pokorný [poki] 2015-01-21 15:10:34 UTC
From a first sight, there are legally questionable images in
- amazon.png
- google.png
- yahoo.png
(and perhaps more, wikipedia is likely non-issue)

1. As these reflect proper logos, not sure if they are under
   free-to-publish license.
2. They may constitute a trademark or other sort of intellectual property.

These probably needs ACK by Fedora legal.

Note: there may be a precedence if Firefox search plugin
(browser/locales/en-US/searchplugins/{amazondotcom,bing,...}.xml) but one
would suppose Mozilla takes care about these things properly.

Comment 3 Jan Pokorný [poki] 2015-01-21 15:48:00 UTC
FWIW, similar issue was discussed in the past:

Comment 4 Jan Pokorný [poki] 2015-01-21 16:21:04 UTC
It becomes interesting :-/

Following file looks like a direct StarCraft II rip-off:

Compare, e.g., with:

I don't have resources to check all the media/content included, so far
I was only guided by the file names.

Comment 5 Jan Pokorný [poki] 2015-01-21 17:47:41 UTC
Raised at Fedora Legal ML:

Comment 6 Tom "spot" Callaway 2015-01-21 17:58:13 UTC
Yeah, I think these artwork files will be problematic. Can someone explain their intended purpose/use?

Comment 7 Mamoru TASAKA 2015-01-22 04:16:20 UTC
I just remove these items for now as there are not important.


* Thu Jan 22 2015 Mamoru TASAKA <mtasaka@fedoraproject.org> - 3.4.0-10
- Some may-be-problematic contents removed (bug 1178912)

Comment 8 Mamoru TASAKA 2015-01-22 04:17:38 UTC
Two macro-in-comment rpmlint will be fixed afterwards.

Comment 9 Mamoru TASAKA 2015-01-22 07:41:44 UTC
By the way as you are trying to review my 2 packages, if you have another package waiting for review please let me know it.

Comment 10 Jan Pokorný [poki] 2015-01-22 20:01:49 UTC
Actually not, I just considered your two packages as "cairo-dock bundle"
where single/multi SRPM has little to no significance (note that making it
a single SRPM would also prevent an "initial bootstrap" issue, although
it would be a bigger burden from maintenance POV, would mean unnecessary
components being rebuilt and perhaps it is also against the spirit of
granular packaging).
But I welcome your prompt, may be handy at some point actually :)


Looks like the cairo-dock-plug-ins project has extremely loose governance
when it comes to legal background of the included graphics; I managed to
pick another legal nit:


which is actually an old Eclipse logo [1], likely a subject of The Eclipse
Foundation guidelines [2].


Anyway, thanks for providing a solution for questionable files reported
so far -- I can only approve blocking out Scooby-Doo plugin for the time
being as its logo[*] (and naming, perhaps) is questionable on its own [3].

[*] cairo-dock-plug-ins-unstable-3.4.0-9.fc21.x86_64.rpm/usr/share/cairo-dock/plug-ins/Scooby-Do/icon.png

[1] http://www.eclipse.org/artwork/old_artwork.php
[2] http://www.eclipse.org/legal/logo_guidelines.php
[3] https://en.wikipedia.org/wiki/Scooby_doo

Comment 11 Jan Pokorný [poki] 2015-01-22 22:53:35 UTC
Only MUST items for now:

> ===== 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.

All unversioned, but in private subdir (plugins).

> [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 successfully compiles and builds into binary rpms on at least one
>      supported primary architecture.
>      Note: Using prebuilt packages
> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.

Hopefully all infringing files (graphics) were pointed out.

> [x]: 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)", "GPL (v3 or later)", "Unknown or generated", "*No
>      copyright* GPL (v3 or later)", "LGPL (v2 or later) (with incorrect FSF
>      address)", "GPL (v3 or later) (with incorrect FSF address)", "GPL (v2 or
>      later) (with incorrect FSF address)", "GPL (v3)". 18 files have unknown
>      license. Detailed output of licensecheck in /home/mock/build-10/cairo-
>      dock-plug-ins-3.4.0-10.fc/cairo-dock-plug-ins/licensecheck.txt
> [!]: License file installed when any subpackage combination is installed.

Technically, this is not true: only cairo-dock-plug-ins binary RPM contains
the %license files, other subpackages are not.

> [x]: %build honors applicable compiler flags or justifies otherwise.
> [x]: All build dependencies are listed in BuildRequires, except for any that
>      are listed in the exceptions section of Packaging Guidelines.
>      Note: Using prebuilt rpms.
> [x]: Package contains no bundled libraries without FPC exception.
> [x]: Changelog in prescribed format.
> [ ]: Sources contain only permissible code or content.

See "Package is licensed with an open-source compatible [...]"

> [x]: Package contains desktop file if it is a GUI application.
> [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).
> [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.
> [x]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> [x]: Requires correct, justified where necessary.
> [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 215040 bytes in 28 files.

Personally I would be considering whether demo code shouldn't be placed in
extra -devel package, but seems fine also under the root package.

> [x]: Package complies to the Packaging Guidelines
> [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]: 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 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 do not use a name that already exist
> [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
> Python:
> [x]: Python eggs must not download any dependencies during the build process.
> [x]: A package which is used by another package via an egg interface should
>      provide egg info.
> [x]: Package meets the Packaging Guidelines::Python
> [x]: Package contains BR: python2-devel or python3-devel
> [x]: Binary eggs must be removed in %prep

Comment 12 Mamoru TASAKA 2015-01-23 07:29:16 UTC

> Hopefully all infringing files (graphics) were pointed out.
> See "Package is licensed with an open-source compatible
- For now broken.png removed (see cairo-dock-plug-ins-create-fedora-tarball.sh

> Technically, this is not true
- Fixed

> For demo files
- I want to package these files in some binary rpm - for now I chose
  the current one.

- Plug-ins non-version so files are okay.

* Fri Jan 23 2015 Mamoru TASAKA <mtasaka@fedoraproject.org> - 3.4.0-11
- Another may-be-problematic contents removed (bug 1178912)
- Make sure that licenses files are always installed

Comment 13 Jan Pokorný [poki] 2015-01-23 14:41:53 UTC
As for cairo-dock-plug-ins-common (only license files for now, which is
OK), I think it should rather be norach (and, consequently, internal
Requires without %{?_isa}).

Comment 14 Jan Pokorný [poki] 2015-01-23 14:58:55 UTC
cairo-dock-python[23] and cairo-dock-ruby should be noarch as well.

Comment 15 Mamoru TASAKA 2015-01-23 15:17:58 UTC
For may-be-noarch subpackage, I will check afterwards.

Comment 16 Mamoru TASAKA 2015-01-23 15:22:52 UTC
(What is difficult for noarch is that if B.noarch depends on A.%{?_isa}, and C%{?_isa} depends on B.noarch, then the %{?_isa} dependency between A and C becomes obscure.)

Comment 17 Mamoru TASAKA 2015-01-23 15:25:07 UTC
Sorry for sequent post, however so for now I want to make all %_isa specific because dependency is not simple.

Comment 18 Jan Pokorný [poki] 2015-01-23 15:28:35 UTC
> ===== SHOULD items =====
> Generic:
> [x]: Reviewer should test that the package builds in mock.
> [ ]: If the source package does not include license text(s) as a separate file
>      from upstream, the packager SHOULD query upstream to include it.
> [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-
>      plug-ins-xfce , cairo-dock-plug-ins-kde , cairo-dock-plug-ins-webkit ,
>      cairo-dock-plug-ins-unstable , cairo-dock-python2 , cairo-dock-python3 ,
>      cairo-dock-ruby , cairo-dock-vala , cairo-dock-vala-devel

something slightly related (perhaps unnecessary arch-specificity) is being
discussed above

> [x]: Package functions as described.
> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
> [x]: Scriptlets must be sane, if used.
> [ ]: Description and summary sections in the package spec file contains
>      translations for supported Non-English languages, if available.
> [ ]: Package should compile and build into binary rpms on all supported
>      architectures.
> [ ]: %check is present and all tests pass.
> [x]: Packages should try to preserve timestamps of original installed files.
> [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: Sources can be downloaded from URI in Source: tag
> [x]: Buildroot is not present
> [x]: Package has no %clean section with rm -rf %{buildroot} (or
> [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.

Comment 19 Mamoru TASAKA 2015-01-23 15:40:34 UTC
> something slightly related (perhaps unnecessary arch-specificity) is being
discussed above

This is okay (and anyway with the existence of -common, EVRs of subpackages installed are all the same)

Comment 20 Jan Pokorný [poki] 2015-01-23 15:42:07 UTC
Maybe I would suggest grouping BuildRequires with respective subpackage
(e.g., ruby-devel under "-n cairo-dock-ruby") to make the relationships
more obvious.

> (What is difficult for noarch is that if B.noarch depends on A.%{?_isa}, and
> C%{?_isa} depends on B.noarch, then the %{?_isa} dependency between A and C
> becomes obscure.)
> [...]
> for now I want to make all %_isa specific because dependency is not simple

This relates to cairo-dock-python2 only, or am I missing something?

Comment 21 Mamoru TASAKA 2015-01-23 15:51:52 UTC
(In reply to Jan Pokorný from comment #20)
> Maybe I would suggest grouping BuildRequires with respective subpackage
> (e.g., ruby-devel under "-n cairo-dock-ruby") to make the relationships
> more obvious.

Well, actually I object to this and I always recommend to write BRs in the
same place (some other reviewers also suggests so)

> > (What is difficult for noarch is that if B.noarch depends on A.%{?_isa}, and
> > C%{?_isa} depends on B.noarch, then the %{?_isa} dependency between A and C
> > becomes obscure.)
> > [...]
> > for now I want to make all %_isa specific because dependency is not simple
> This relates to cairo-dock-python2 only, or am I missing something?

Anyway using %_isa is much safer. Or is this a blocker?

Comment 22 Jan Pokorný [poki] 2015-01-23 16:34:35 UTC
Not just %_isa but marking subpackages as noarch properly.  IIUIC, it would
save some space on the mirrors, etc. so in case there is no strong argument
not to make it noarch (which may still be the case and cairo-dock-python2
may qualify) I don't like much.  Not a blocker, but would like to know what
issues you've hit that are being prevented by not using noarch.

Comment 23 Mamoru TASAKA 2015-01-23 16:51:57 UTC
See comment 16, one can install A.i686 B.noarch, C.x86_64 manually using rpm -i (normal install using default "yum" option won't do that, dnf - I don't know well) and this won't work because actually C.x86_64 needs A.i686.

One can write "Requires: A%{?_isa}" on C explicitly, however this is redundant. If B is _VERY_ large (like game data), it is preferable to make it noarch, however this case I see no gain.

Comment 24 Mamoru TASAKA 2015-01-28 14:15:19 UTC
Would you point out what is currently a blocker? Thank you!

Comment 25 Mamoru TASAKA 2015-02-03 09:22:28 UTC
Jan, Would you point out what is currently a blocker?

Comment 26 Jan Pokorný [poki] 2015-02-09 17:44:23 UTC
Mamoru, sorry for stalling the review, had some other urgencies + meetings
piggybacking Devconf.cz conference.

I've thought over your "avoiding noarch" reasoning and now it makes
sense to me ... apparently I am not an experienced packager but trying
to get there :)

So my current objections divided into strong and weak ones:


1. It seems unfortunate to split CDBashApplet.{py,sh}.


while, if I read the code correctly, CDBashApplet.py depends on
CDBashApplet.sh being present in the very same directory, i.e.,
the current arrangement seems a bit futile/illogic.

So at the packaging level, these two files should IMHO go together
in a single subpackage (you decide which, if any).

Wrt. the exact location of these two files it's similar: they should
IMHO go to the same directory, perhaps which Python compiled file would
depend on (i.e., conditionally) which Python is a default one in
particular distribution version (you decide which dir, if any).

(again, I apologize for not being able to systematically discover any
other similar inter-file dependencies, this was a matter of coincidence,
furthermore I am not 100% sure about that)

2. I cannot find a reason why cairo-dock-vala-devel needs to depend
   on cairo-dock-plug-ins;  it already indirectly depends on
   cairo-dock-plug-ins-common and cairo-dock-core


3. (releated especially to [comment 13]) still, cairo-dock-plug-ins-common
   deserves to be noarch, it's a terminal in the dependency chain
   and nothing platform specific in here -- or are you considering
   future arch-specificity or restructuring the dependencies?

   - looking at the dependency graph carefully, it might be sort
     of an optimization (a bit forced, though) if
     cairo-dock-plug-ins-common depended on cairo-dock-core as there
     would be no factual change in the dependencies[*], but plenty (9)
     of Requires lines could be dropped ... certainly, in that case
     it would have to be arch-specific and %{?_isa} macro used

[*] provided that nobody wants to install cairo-dock-plug-ins-common
    in isolation, which provided that it carries only the licenses,
    simply won't happen

I hope these are good points.

Comment 27 Mamoru TASAKA 2015-02-11 07:00:22 UTC
> 1. It seems unfortunate to split CDBashApplet.{py,sh}.
Well, this makes more complicated packaging... Splitting these
must not be avoidable because python2/3 bindings are provided.

Instead -dbus subpackage is newly created.

> 2. I cannot find a reason why cairo-dock-vala-devel needs 
> to depend on cairo-dock-plug-ins

Directory ownership, but actually depends on dbus (changed on -12)


* Wed Feb 11 2015 Mamoru TASAKA <mtasaka@fedoraproject.org> - 3.4.0-12
- Split out Dbus subpackage, modify internal dependency
- Make some package noarch

Comment 28 Mamoru TASAKA 2015-02-15 23:45:42 UTC

Comment 29 Mamoru TASAKA 2015-02-23 13:04:02 UTC
ping again? I would really want to import this package.

Comment 30 Jan Pokorný [poki] 2015-02-23 20:50:32 UTC
I do apologize, Mamoru, more pressing stuff keeps coming and it
is not easy to estimate the time needed to review this convoluted

Now, I've started to looking into that again, overall the structure
looks better modulo these spots:

- %description at -dbus
  s/This packagcontains/This package contains/

- two identical Provides at cairo-dock-python2
  (cairo-dock-python = %{version}-%{release})

- changelog
  s/Make some package noarch/Make some packages noarch/

No need to rush with the fixes, I should have finished the review
by tomorrow.

Comment 31 Jan Pokorný [poki] 2015-02-24 11:49:39 UTC

- I would suggest making c-d-p-i-base -- rather than c-d-p-i directly --
  dependent on c-d-python2 (it would be pulled by installing c-d-p-i
  transitively via c-d-p-i-base anyway and the dependency chain would
  be more transparent, IMHO)

- I am still not too familiar with the file location inter-dependencies
  but it seems to me either of following steps should be made to make
  CDBashApplet.py useful in any way:

  a. patch "self.app_folder = [...]" line in CDBashApplet.py file
     in c-d-python{2,3} packages so as to point to the directory
     containing CDBashApplet.sh from c-d-p-i-dbus (which is already
     required by the "python" packages)

  b. make a symlink under site-packages subdir in c-d-python{2,3}
     packages to CDBashApplet.sh from c-d-p-i-dbus
    (requirement-already-satisfied note applies here as well)

- more out of curiosity, I'd like to verify that

> # Modify CDApplet.h
> sed -i .%{_datadir}/cairo-dock/plug-ins/Dbus/CDApplet.h \
> 	-e '\@def@s|__.*\(DBUS_INTERFACES_VALA_SRC_CDAPPLET_H__\)|__\1|'

part has been added as a matter of "purity" (i.e., suppressing
identifiers accidentally leaking some info about who run "Vala compiler"
(not familiar with that)

- suggested cosmetics in Requires:
  cairo-dock-plug-ins-common -> %{name}-common
  ditto dbus subpackage (+perhaps more)

Otherwise happy about the current state of the package.  Will give it an
install-and-run try, but do not expect any issue there.

Comment 32 Mamoru TASAKA 2015-02-24 12:25:15 UTC
So please make it clear what is currently a blocker?

Comment 33 Jan Pokorný [poki] 2015-02-24 12:28:35 UTC
In case it's not apparent, please consider [comment 30] and [comment 31].

Comment 34 Mamoru TASAKA 2015-02-24 12:36:26 UTC
* Description typos or so can be fixed when importing git.
* cosmetics can be fixed later
* Suggestions can be considered, however if unless you have some
  reason it _MUST_, I want to leave this for now.

Comment 35 Mamoru TASAKA 2015-02-24 12:40:54 UTC
i.e. to be clear, I don't see any blocker issue here.

Comment 36 Mamoru TASAKA 2015-02-25 10:07:15 UTC
* For CDBashApplet.sh placement it is no problem, (at least
  from executing demo_bash sample plug-in, I actually tried
  using this demo) (moving it may cause unexpected error)

* "-base" should not depend on -python2 or 3. I would keep
  this dependency for now

* For Modify CDApplet.h, some comments for this will be added

* All others are cosmetic changes.

Anyway thank you for reviewing this again. Will update.

Comment 37 Mamoru TASAKA 2015-02-25 10:31:01 UTC
Cosmetic changes and modifying CDApplet.h explanation added

c-d-p-i-base dependency and CDBashApplet.py left as before (again
CDBashApplet.py is actually working)


I will review your clufer package.

Comment 38 Jan Pokorný [poki] 2015-02-25 19:50:42 UTC
ACK, it's in a good shape now.

Also checked it (c-d + c-d-p-i together) works well on F21/LXDE.

re CDBashApplet.sh:
Ok, there is apparently more going on, also when a comment for
"INSTALL_PREFIX = os.path.abspath("..")" in CDApplet.py taken
into account.

Note on the demo_* applets usage:
$ cp -r /usr/share/doc/cairo-dock-plug-ins-dbus/Dbus/demos/* \
and restarted cairo-dock.  I am able to add these applets to the main
dock but they seem to be only half-working for me.

Per, e.g., demo_python description:

> This is a distant applet
> It simulates a counter:
>  Scroll up/down to increase/decrease the counter,
>  Click/middle-click to increase/decrease the counter by 10
>  Drop some text to set it as the label.

I am observing no such behaviour, just a plain icon of two gears.
Is it expected?

Comment 39 Mamoru TASAKA 2015-02-26 05:13:00 UTC
* For demo_python
- the description is somewhat wrong (will ping upstream), however
  at first please make it sure that you have changed the permission
  of "demo_python" script to make owner-executable
  - because rpmlint complains that document files should not have
    executable permission, I explicitly dropped it - you have to
    enable the permission manually

* Will check CDBashApplet.sh

Thank you for reviewing!

Comment 40 Mamoru TASAKA 2015-02-26 05:29:13 UTC
Unblocking FE-Legal.

Comment 41 Mamoru TASAKA 2015-02-26 05:30:40 UTC
This one is a new package

New Package SCM Request
Package Name: cairo-dock-plug-ins
Short Description: Plug-ins files for Cairo-Dock
Upstream URL: http://glx-dock.org/
Owners: mtasaka
Branches: f22 f21 f20

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

Comment 43 Mamoru TASAKA 2015-02-27 18:05:30 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.