Bug 784958 - Review Request: ktp-text-ui - Telepathy text chat handler
Review Request: ktp-text-ui - Telepathy text chat handler
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: nucleo
Fedora Extras Quality Assurance
:
Depends On: 784945
Blocks: kde-reviews kde-telepathy-0.3
  Show dependency treegraph
 
Reported: 2012-01-26 14:02 EST by Rex Dieter
Modified: 2012-02-19 04:33 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-02-19 04:33:04 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
alekcejk: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
copyright file from ubuntu deb (6.00 KB, text/plain)
2012-02-09 21:10 EST, nucleo
no flags Details
patch for Renkoo LICENSE.txt (7.04 KB, patch)
2012-02-11 11:01 EST, nucleo
no flags Details | Diff
second try (7.28 KB, patch)
2012-02-11 11:12 EST, nucleo
no flags Details | Diff

  None (edit)
Comment 1 nucleo 2012-02-06 20:52:25 EST
name: ok
summary: ok
handling locale files: ok

BuildRequires:
BR: telepathy-qt4-devel is not needed here because ktp-common-internals-devel requires it.

/sbin/ldconfig in %post/%postun:
There is libktpchat.so installed in libdir so /sbin/ldconfig is needed.
Other libktpchat.so related issue is invalid-soname rpmlint error.

license:
Code looks like is GPLv2+ but
there is in data/styles/renkoo.AdiumMessageStyle/Contents/Resources/Renkoo LICENSE.txt:

The images, css and html is dual licensed under the BSD and AFL license.
The source files for the bubbles can be found at http://www.itorrey.com/adiumx/

The fading javascript is not covered in this license. The code is fadomatic and is covered under its own license as set by its author.


in data/styles/simkete/Contents/README:
This Adium style is released under MIT/X11 license

rpmlint output:
ktp-text-ui.i686: E: invalid-soname /usr/lib/libktpchat.so libktpchat.so
ktp-text-ui.i686: E: zero-length /usr/share/kde4/apps/ktelepathy/styles/simkete/Contents/Resources/Header.html
ktp-text-ui.i686: E: zero-length /usr/share/kde4/apps/ktelepathy/styles/simkete/Contents/Resources/Footer.html
3 packages and 0 specfiles checked; 3 errors, 0 warnings.
Comment 2 nucleo 2012-02-06 20:56:57 EST
Optional dependency TelepathyLoggerQt4 can be packaged or it is not ready yet?
Comment 3 nucleo 2012-02-06 21:01:10 EST
There is no desktop files installed in /usr/share/applications so
BuildRequires: desktop-file-utils is not needed.
Comment 4 Kevin Kofler 2012-02-06 21:07:56 EST
"The images, css and html is dual licensed under the BSD and AFL license."

"BSD" is quite vague without the exact license text… Is there nothing more explicit?

> The fading javascript is not covered in this license. The code is fadomatic and
> is covered under its own license as set by its author.

If it's the same as this:
https://github.com/phl/Fadomatic/blob/master/fadomatic.js
that's MIT.


The License tag should apparently be:
License: GPLv2+ and (BSD or AFL) and MIT
Comment 5 nucleo 2012-02-06 21:17:30 EST
(In reply to comment #4)
> > The fading javascript is not covered in this license. The code is fadomatic and
> > is covered under its own license as set by its author.
> 
> If it's the same as this:
> https://github.com/phl/Fadomatic/blob/master/fadomatic.js
> that's MIT.

Looks like this is it:
https://projects.kde.org/projects/extragear/network/telepathy/ktp-text-ui/repository/revisions/master/entry/data/styles/renkoo.AdiumMessageStyle/Contents/Resources/Footer.html
Comment 6 Kevin Kofler 2012-02-06 22:24:26 EST
This needs the copyright notice and MIT license added, otherwise it is in violation of the license.
Comment 7 Kevin Kofler 2012-02-06 22:27:31 EST
The remainder of that style is under (BSD and AFL) alright:
https://projects.kde.org/projects/extragear/network/telepathy/ktp-text-ui/repository/revisions/master/entry/data/styles/renkoo.AdiumMessageStyle/Contents/Resources/Renkoo LICENSE.txt
but the fadomatic stuff needs to be properly attributed.
Comment 8 Rex Dieter 2012-02-07 09:25:19 EST
Spec URL: http://rdieter.fedorapeople.org/rpms/telepathy-kde/ktp-text-ui.spec
SRPM URL:
http://rdieter.fedorapeople.org/rpms/telepathy-kde/ktp-text-ui-0.3.0-2.fc16.src.rpm

%changelog
* Tue Feb 07 2012 Rex Dieter <rdieter@fedoraproject.org> 0.3.0-2
- drop BR: desktop-file-utils telepathy-qt4-devel
- %%post/%%postun ldconfig scriptlets
- License: GPLv2+ and (BSD or AFL) and MIT


Re: Optional dependency TelepathyLoggerQt4 can be packaged or it is not ready yet?

I left the comment in the .spec simply because it's still a TODO item.  It can be packaged, I just haven't had time to do it myself yet.


Re: fadomatic.js license attribution?
So, what concretely do we need to do to satisify this?  I assume while we're at it, notify upstream too?
Comment 9 Kevin Kofler 2012-02-07 09:53:31 EST
Check if what's being shipped is the same JavaScript as the upstream fadomatic.js I linked to, and if yes, patch in the copyright header and license.

And yes, ktp-text-ui upstream needs to fix that too!
Comment 10 Rex Dieter 2012-02-09 13:53:10 EST
Ressetting reviewer.
Comment 11 nucleo 2012-02-09 20:26:16 EST
There is error:

cp: cannot stat `./data/styles/renkoo.AdiumMessageStyle/Contents/Resources/Renkoo': No such file or directory
cp: cannot stat `LICENSE.txt': No such file or directory

This should fix it:

%doc ./data/styles/renkoo.AdiumMessageStyle/Contents/Resources/Renkoo\ LICENSE.txt
Comment 13 nucleo 2012-02-09 21:10:49 EST
Created attachment 560743 [details]
copyright file from ubuntu deb

MIT License also used for files in data/styles/simkete/, license text file  data/styles/simkete/Contents/README (can be added in %dos) so maybe it is enough for us?

This Adium style is released under MIT/X11 license:

Copyright (c) 2009,2011 Matěj Cepl <mcepl at redhat dot com>

Permission is hereby granted, free of charge, to any person obtaining a copy of
this software and associated documentation files (the "Software"), to deal in
the Software without restriction, including without limitation the rights to
use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
the Software, and to permit persons to whom the Software is furnished to do so,
subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

Or for modified Footer.html needed yet one MIT license with fadomatic.js author?
Comment 14 Kevin Kofler 2012-02-09 23:07:24 EST
Where did you find that license? The license I see is this:
https://projects.kde.org/projects/extragear/network/telepathy/ktp-text-ui/repository/revisions/master/entry/data/styles/renkoo.AdiumMessageStyle/Contents/Resources/Renkoo LICENSE.txt
Either way, the fadomatic.js code needs to be properly attributed to its true author.
Comment 16 Kevin Kofler 2012-02-09 23:19:52 EST
Ah, that's the simkete style, the other style shipped in the package. That one is clearly MIT. But it's unrelated to the Fadomatik code in the renkoo style, which is missing its copyright notice and at least a reference to the license (but since it's the MIT license, a full copy of the exact license is better, it's very short and there are subtle differences in wording between the variants).
Comment 17 nucleo 2012-02-10 18:29:08 EST
Upstream informed about Footer.html license
http://adium.im/pipermail/devel_adium.im/2012-February/008866.html
http://mail.kde.org/pipermail/kde-telepathy/2012-February/005432.html

So maybe it is enough to add notice about this and add in %doc MIT license from simkete style README?
Comment 18 Rex Dieter 2012-02-10 20:08:03 EST
Spec URL: http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui.spec
SRPM URL: http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui-0.3.0-3.fc16.src.rpm

%changelog
* Fri Feb 10 2012 Rex Dieter <rdieter@fedoraproject.org> 0.3.0-3
- %%doc data/styles/simkete/Contents/README
- fix %%doc Renkoo\ LICENSE.txt
- License: clarify MIT for data/styles/simkete too
Comment 19 Rex Dieter 2012-02-10 20:43:50 EST
Spec URL: http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui.spec
SRPM URL:
http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui-0.3.0-4.fc16.src.rpm

%changelog
* Fri Feb 10 2012 Rex Dieter <rdieter@fedoraproject.org> 0.3.0-4
- mac2unix '.../Renkoo LICENSE.txt'
Comment 20 Rex Dieter 2012-02-10 21:17:11 EST
Spec URL: http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui.spec
SRPM URL:
http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui-0.3.0-5.fc16.src.rpm

%changelog
* Fri Feb 10 2012 Rex Dieter <rdieter@fedoraproject.org> 0.3.0-5
- -devel: Requires: ktp-common-internals-devel
Comment 21 Rex Dieter 2012-02-10 21:23:03 EST
Spec URL: http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui.spec
SRPM URL:
http://rdieter.fedorapeople.org/rpms/ktp/ktp-text-ui-0.3.0-6.fc16.src.rpm

%changelog
* Fri Feb 10 2012 Rex Dieter <rdieter@fedoraproject.org> 0.3.0-6
- -devel: fix typo in Requires
Comment 22 nucleo 2012-02-10 22:05:31 EST
http://trac.adium.im/wiki/mathuaerknedam ansered me on #adium-devl that he will look on fadomatic issue, ktp will follow for adium changes http://mail.kde.org/pipermail/kde-telepathy/2012-February/005434.html

MUST Items:
+ rpmlint output
  $ rpmlint ktp-text-ui-0.3.0-6.fc16.i686.rpm ktp-text-ui-0.3.0-6.fc16.src.rpm ktp-text-ui-debuginfo-0.3.0-6.fc16.i686.rpm ktp-text-ui-devel-0.3.0-6.fc16.noarch.rpm
    ktp-text-ui.i686: E: invalid-soname /usr/lib/libktpchat.so libktpchat.so (ignored for now until upstream made it versioned)
    ktp-text-ui.i686: E: zero-length /usr/share/kde4/apps/ktelepathy/styles/simkete/Contents/Resources/Header.html (can be ignored because files can be used somehow)
    ktp-text-ui.i686: E: zero-length /usr/share/kde4/apps/ktelepathy/styles/simkete/Contents/Resources/Footer.html
    ktp-text-ui.i686: W: file-not-utf8 /usr/share/doc/ktp-text-ui-0.3.0/Renkoo LICENSE.txt (no easy way to fix it because file was made from files with different encodings in wrong way)
    ktp-text-ui-devel.noarch: W: no-documentation
    4 packages and 0 specfiles checked; 3 errors, 2 warnings.
+ named and versioned according to the Package Naming Guidelines.
  Package name match the upstream tarball name ktp-text-ui-0.3.0.tar.bz2
+ spec file name ktp-text-ui.spec matches base package name
+ complies with all the legal guidelines:
  + License: GPLv2+ and (BSD or AFL) and MIT valid, matches actual license (added notices for parts under different licenses)
  + No known patent problems
  + No emulator, no firmware, no binary-only or prebuilt components
+ COPYING (GNU GENERAL PUBLIC LICENSE Version 2), README (MIT license), Renkoo LICENSE.txt (BSD and AFL license) packaged as %doc
+ source matches upstream:
  MD5: 893b1eeb962ef2ba79244147c7051e0e  ktp-text-ui-0.3.0.tar.bz2
  SHA1: b9ba195904d470835e404f37073ed36fc510c791  ktp-text-ui-0.3.0.tar.bz2
  SHA256: f735708db55367ab37aa9b21af48ae7390022ba3a9b7c2ee9b369d430df79fa0  ktp-text-ui-0.3.0.tar.bz2
+ builds on at least one arch
  build from mock is in F16 kde-unstable repo
+ no known non-working arches, so no ExcludeArch needed
+ no missing BuildRequires (builds in mock)
+ locales are handled properly by using %find_lang %{name} --all-name --with-kde macro
+ ldconfig call used (needed for %{_kde4_libdir}/libktpchat.so shared library)
+ no duplicated system libraries
+ package not relocatable (no Prefix tag)
+ directory ownership correct (doesn't own directories owned by another package, owns all package-specific directories)
+ no duplicate files in %files
+ permissions correct, %defattr(-,root,root,-) not needed now, executables have executable permissions
+ macros used where possible (%{name}, %{version}, %{buildroot}, %{_target_platform}, %{cmake_kde4}, %{_kde4_libexecdir}, %{_kde4_libdir}, %{_kde4_datadir}, %{_datadir})
+ non-code content: only permitted content, chat theme, have open source compatible licenses
+ no large documentation files, so no -doc package needed
+ no %doc files required at runtime
+ header files packaged in -devel subpackage
+ no static libraries, so no -static package needed
+ no devel symlinks which would need to be in a -devel subpackage (in noarh -devel only headres)
+ devel packages must require the base package
+ no .la files
+ no .desktop file needed in /usr/share/applications for this KDE Telepathy internal module
+ desktop-file-validate call not needed for service type .dsktop files installed in %{_kde4_datadir}/kde4/services
+ all filenames are valid UTF-8
+ other packaging guidelines:
  + complies with the Filesystem Hierarchy Standard (all files in %{_kde4_libexecdir}, %{_kde4_libdir} and %{_datadir})
  + proper changelog, tags, BuildRequires, Summary, Description (used the only available description from upstream)
  + no non-UTF-8 characters (except Renkoo LICENSE.txt which have mostly cosmetic defects)
  + all relevant documentation included as %doc (COPYING README, Renkoo LICENSE.txt)
  + RPM_OPT_FLAGS are used in %{cmake_kde4} macro
  + debuginfo package is valid (contains stripped symbols from ELF binary and source code related to it)
  + no rpaths (no check-rpaths error)
  + no configuration files, so %config guideline doesn't apply
  + no init scripts, so init script guideline doesn't apply
  + timestamps are preserved
  + %{?_smp_mflags} used
  + not a web application, so web application guideline doesn't apply
  + no conflicts

SHOULD Items:
+ license already included upstream
+ no translations for description and summary provided by upstream
+ package builds in mock (built for kde-unstable)
- successfully tested the package functionality (no testing yet)
+ scriptlets are sane (%post -p /sbin/ldconfig, %postun -p /sbin/ldconfig)
+ subpackages other than devel should require the base package using a fully versioned dependency (no subpackages other than devel, devel have fully versioned dependency)
+ no .pc files, so "placement of .pc files" is irrelevant
+ no file dependencies
+ no binaries/scripts that needs man pages
Comment 23 nucleo 2012-02-10 22:06:17 EST
APPROVED
Comment 24 Rex Dieter 2012-02-10 22:12:53 EST
New Package SCM Request
=======================
Package Name: ktp-text-ui
Short Description: Telepathy text chat handler
Owners: jreznik rdieter
Branches: f16
Comment 25 Kevin Kofler 2012-02-11 01:32:01 EST
Uhm, IMHO:
* The missing attribution (copyright notice) and license statement for Fadomatic is a blocker.
* The non-UTF-8 Renkoo LICENSE.txt is also a blocker. (The encoding used is probably MacRoman.)
Comment 26 nucleo 2012-02-11 08:08:33 EST
$ iconv -l | grep -i mac
CSISO111ECMACYRILLIC//
CSMACINTOSH//
ECMACYRILLIC//
MAC-CENTRALEUROPE//
MAC-CYRILLIC//
MAC-IS//
MAC-SAMI//
MAC-UK//
MAC//
MACCYRILLIC//
MACINTOSH//
MACIS//
MACUK//
MACUKRAINIAN//
MS-MAC-CYRILLIC//

I tried to convert to UTF-8 from all of this encodings but no one gives readable characters.
The only non UTF-8 characters there: "Renkoo�6�3", "U.S.C. �0�30��0���0�3".

There is answer from adium theme developer: http://adium.im/pipermail/devel_adium.im/2012-February/008869.html
Comment 27 Kevin Kofler 2012-02-11 10:07:10 EST
I think that if in Renkoo LICENSE.txt, instead of or in addition to this vague statement:

> The fading javascript is not covered in this license. The code is fadomatic and > is covered under its own license as set by its author.

an actual copy of the copyright and license header of:

https://github.com/phl/Fadomatic/blob/master/fadomatic.js

was inserted, that should be enough to solve the issue.
Comment 28 Kevin Kofler 2012-02-11 10:28:46 EST
As for the non-UTF-8 characters:
* The first one after "Renkoo" appears to be the ℠ (U+2120 / SM / Service Mark) character encoded in the Chinese GB18030 encoding. (It is autodetected as such by KWrite, and the statement "Renkoo is a service mark of Renkoo, Inc." below the license is supportive of this interpretation.) (Note that the character doesn't appear to be rendered correctly in the font used in this Bugzilla. It should be a superscript SM similar to the well-known ™ (TM) sign.)
* The second sequence was apparently mangled by improper character set conversion, but thankfully this is part of the text of the Academic Free License version 2.1. The text should read "17 U.S.C. § 101 et seq.", i.e. "the U.S. Copyright Act, 17 U.S.C. § 101 et seq., the equivalent laws of other countries". (It looks like a lot of garbage was produced by repeated misconversion of the "§" sign (U+00A7 / section/paragraph sign).)
Comment 29 nucleo 2012-02-11 10:37:09 EST
SM can be fixed with
iconv -f GB18030 -t UTF-8 Renkoo\ LICENSE.txt

but what to do with garbage?
Comment 30 Kevin Kofler 2012-02-11 10:40:57 EST
(Looks like the ℠ sign is displayed correctly when viewing the Bugzilla comment, but not when typing it.)
Comment 31 Kevin Kofler 2012-02-11 10:41:45 EST
Patch the file and get upstream to fix it. It needs to be edited anyway to add the Fadomatic license instead of that vague reference.
Comment 32 nucleo 2012-02-11 11:01:56 EST
Created attachment 561105 [details]
patch for Renkoo LICENSE.txt

Fixes ℠, § and adds text of fadomatic license.
Should be applied after mac2unix converting.
Is this patch enough?
Comment 33 nucleo 2012-02-11 11:04:04 EST
This is strange - I added "§" but in patch "§" - how it should be?
Comment 34 nucleo 2012-02-11 11:05:09 EST
And § again broken :(
Comment 35 nucleo 2012-02-11 11:05:25 EST
And ℠ again broken :(
Comment 36 nucleo 2012-02-11 11:12:27 EST
Created attachment 561108 [details]
second try

So now it resolves all issues?
Comment 37 nucleo 2012-02-11 11:14:08 EST
After patching:

$ file Renkoo\ LICENSE.txt 
Renkoo LICENSE.txt: UTF-8 Unicode English text, with very long lines
Comment 38 Rex Dieter 2012-02-11 11:17:26 EST
Since that license file needs so much fixing, maybe just include a fixed version as a separate Source, and send that upstream for them to use too.

Anyway, its largely cosmetic, let's fix it after review.
Comment 39 Kevin Kofler 2012-02-11 11:20:01 EST
Yes, with those changes, it looks OK to me. Please also send this upstream.

(The ordering is a bit suboptimal, but I'm not sure what the clearest ordering of the paragraphs and licenses is.)
Comment 41 Jon Ciesla 2012-02-11 14:37:45 EST
Git done (by process-git-requests).

Added f17.
Comment 42 Rex Dieter 2012-02-19 04:33:04 EST
imported

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