Bug 1178912

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

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

From a first sight, there are legally questionable images in
cairo-dock-plug-ins-unstable-3.4.0-9.fc21.x86_64.rpm/usr/share/cairo-dock/plug-ins/Scooby-Do:
- 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.

FWIW, similar issue was discussed in the past:
https://lists.fedoraproject.org/pipermail/legal/2011-April/001604.html

It becomes interesting :-/

Following file looks like a direct StarCraft II rip-off:
cairo-dock-plug-ins-3.4.0-9.fc21.x86_64.rpm/usr/share/cairo-dock/plug-ins/desklet-rendering/starcraft2.png

Compare, e.g., with:
http://3.bp.blogspot.com/-WSW_CkPsNcI/TahvjAfCHhI/AAAAAAAAAUE/mLIdqR7ujTw/s1600/starcraft2.jpg

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

Raised at Fedora Legal ML:
https://lists.fedoraproject.org/pipermail/legal/2015-January/002556.html

Yeah, I think these artwork files will be problematic. Can someone explain their intended purpose/use?

I just remove these items for now as there are not important.

https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins.spec
https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins-3.4.0-10.fc.src.rpm

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

Two macro-in-comment rpmlint will be fixed afterwards.

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.

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:

cairo-dock-plug-ins-3.4.0-10.fc21.x86_64.rpm/usr/share/cairo-dock/plug-ins/weather/broken.png

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

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

Well,

> 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
  change)

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

https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins-3.4.0-11.fc.src.rpm
https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins.spec
* Fri Jan 23 2015 Mamoru TASAKA <mtasaka> - 3.4.0-11
- Another may-be-problematic contents removed (bug 1178912)
- Make sure that licenses files are always installed

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}).

cairo-dock-python[23] and cairo-dock-ruby should be noarch as well.

For may-be-noarch subpackage, I will check afterwards.

(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.)

Sorry for sequent post, however so for now I want to make all %_isa specific because dependency is not simple.

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

> 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)

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?

(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?

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.

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.

Would you point out what is currently a blocker? Thank you!

Jan, Would you point out what is currently a blocker?

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:

Strong
------

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

Currently:
./cairo-dock-plug-ins-3.4.0-11.fc21.x86_64.rpm/usr/share/cairo-dock/plug-ins/Dbus/CDBashApplet.sh
./cairo-dock-python3-3.4.0-11.fc21.x86_64.rpm/usr/lib/python3.4/site-packages/CDBashApplet.py

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


Weak
----

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.

> 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)

https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins.spec
https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins-3.4.0-12.fc.src.rpm

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

ping?

ping again? I would really want to import this package.

I do apologize, Mamoru, more pressing stuff keeps coming and it
is not easy to estimate the time needed to review this convoluted
package.

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.

...continued:

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

So please make it clear what is currently a blocker?

In case it's not apparent, please consider [comment 30] and [comment 31].

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

i.e. to be clear, I don't see any blocker issue here.

Well,
* 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.

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)

https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins.spec
https://mtasaka.fedorapeople.org/Review_request/cairo-dock/cairo-dock-plug-ins-3.4.0-13.fc.src.rpm

I will review your clufer package.

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:
Performed
$ cp -r /usr/share/doc/cairo-dock-plug-ins-dbus/Dbus/demos/* \
  ~/.config/cairo-dock/third-party
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?

* 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!

Unblocking FE-Legal.

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
InitialCC:

Git done (by process-git-requests).

Rebuilt on all branches, push request on F-22/21/20, now closing.

Thank you for review and git preocedure.