Bug 1425275 - Review Request: php-szymach-c-pchart - A project bringing composer support and PSR standards to pChart 2.0
Summary: Review Request: php-szymach-c-pchart - A project bringing composer support an...
Status: CLOSED CANTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: FE-Legal
TreeView+ depends on / blocked
 
Reported: 2017-02-21 03:49 UTC by Randy Barlow
Modified: 2017-11-26 16:46 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-03-31 02:24:28 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
randy: fedora-review-


Attachments (Terms of Use)
system-fonts.patch (1.27 KB, patch)
2017-02-24 06:41 UTC, Remi Collet
no flags Details | Diff
system-fonts.patch (1.54 KB, text/plain)
2017-03-05 06:36 UTC, Remi Collet
no flags Details
phpci.log (24.17 KB, text/plain)
2017-03-06 05:40 UTC, Remi Collet
no flags Details
review.txt (6.51 KB, text/plain)
2017-03-06 05:41 UTC, Remi Collet
no flags Details

Description Randy Barlow 2017-02-21 03:49:10 UTC
Spec URL: https://bowlofeggs.fedorapeople.org/php-szymach-c-pchart.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-szymach-c-pchart-2.0.3-1.fc26.src.rpm
Description: A project bringing composer support and PSR standards to the pChart 2.0
library.
Fedora Account System Username: bowlofeggs

Comment 1 Remi Collet 2017-02-21 10:24:49 UTC
Hmm... Sorry, I don't think I can review this one.

I think we have a guidelines about fonts
https://fedoraproject.org/wiki/Packaging:FontsPolicy

And don't think all the bundled fonts are "free" (e.g. Verdana which is a Microsoft one), if some are not-free, you even have to drop them from the source archive.

Perhaps you can package this library without any font.

The only ref to the fonts directory is in loadFont function, and if not found, the 'FontName' will be considered as a system font (GD will search in the default system path)

Default value may need to be fixed (+ __construct)
    public $FontName = "GeosansLight.ttf";

Of course, this may require some additional tweaking in the application which rely on this library (to only use system available fonts)

Comment 2 Remi Collet 2017-02-24 06:41 UTC
Created attachment 1257156 [details]
system-fonts.patch

Here is a simple patch which allow to remove all bundled fonts and switch to a default system provided one (dejavu-sans-fonts need to be a dependency).

Examples (from README.md) work.

Comment 3 Randy Barlow 2017-03-05 01:32:26 UTC
Hello Remi!

Thanks so much for catching that, and for writing that patch for me. That was a huge help!

I noticed that one of the fonts was available in fedora, so I added a symlink to it as well. Here are updated files:

Spec URL: https://bowlofeggs.fedorapeople.org/php-szymach-c-pchart.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-szymach-c-pchart-2.0.3-2.fc26.src.rpm

Comment 4 Remi Collet 2017-03-05 06:34:23 UTC
SRPM URL is broken ;)

BTW, the symlink is not needed and won't work, and from the patch the search ignore the bundled fonts directory.

The patch need to be adapted to identity bundled font name and use either:
- system font name if available in GD default font path
- system font patch

Comment 5 Remi Collet 2017-03-05 06:36 UTC
Created attachment 1260013 [details]
system-fonts.patch

New version, instead of always use DejaVu, use a substitution map.

Comment 6 Remi Collet 2017-03-05 06:40:36 UTC
FYI, see bug #1429157

Comment 7 Remi Collet 2017-03-05 06:45:35 UTC
I forgot to tell, second example (in readme) use Silkscreen, so is a good test for this patch.

Comment 8 Randy Barlow 2017-03-06 03:49:50 UTC
Ah, I think the build changed to fc27 on my dev box and I just copy/pasted the SRPM URL from last time (which had been fc26). Sorry about that!

I've got a new build with your new patch and the symlink dropped:


Spec URL: https://bowlofeggs.fedorapeople.org/php-szymach-c-pchart.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-szymach-c-pchart-2.0.3-3.fc27.src.rpm

Comment 9 Remi Collet 2017-03-06 05:40 UTC
Created attachment 1260267 [details]
phpci.log

     Note: phpCompatInfo version 5.0.4 DB version 1.18.0 built Feb 24 2017
     14:37:58 CET static analyze results

Comment 10 Remi Collet 2017-03-06 05:41 UTC
Created attachment 1260268 [details]
review.txt

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1425275
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, PHP, Shell-api

Comment 11 Remi Collet 2017-03-06 05:41:45 UTC
[x]: Package complies to the Packaging Guidelines


== APPROVED ==

Comment 12 Remi Collet 2017-03-06 05:52:32 UTC
Sorry I was too fast.

We have 2 licensing issues:

* easy fix: bundled ttf should be removed from the .src.rpm (as we are not sure they are all free, and even mostly sure some are not).

=> pull a git snapshot (makesrc.sh in lot of php packages) + clean the fonts dir + create a tarball used in your spec.


* upstream license

See 

It was previously stated that this package is on [MIT](https://opensource.org/licenses/MIT) license, which did not meet the requirements
set by the original author. It is now under the [GNU GPL v3](http://www.gnu.org/licenses/gpl-3.0.html)
license, so if you wish to use it in a commercial project, you need to pay an [appropriate fee](http://www.pchart.net/license).

This restriction in the GPLv3 about commercial use seems to make it non-free.

Can you please try to clarify this, perhaps asking on legal list ?

Comment 13 Randy Barlow 2017-03-06 20:49:07 UTC
I started a discussion on the legal mailing list:

https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/4MDAWLRSVR3QIIYFJLVKMEELS5OPBIMI/

It sounds like I have two options:

0) Try to convince pChart upstream and c-pchart to both reword their license as a pure GPL license (no restrictions on commercial use, so long as they follow the GPL). Since c-pchart is not actively maintained, this could be tricky.

1) Try to convince Ampache to stop using this package, since it is not truly Free Software.

I will probably start both options in parallel. Should we mark this as fedora-review- for now, or should we keep it open to see what happens with the above options? I'm fine with either way.

Comment 14 Remi Collet 2017-03-07 11:51:58 UTC
(In reply to Randy Barlow from comment #13)

> 0) Try to convince pChart upstream and c-pchart to both reword their license
> as a pure GPL license (no restrictions on commercial use, so long as they
> follow the GPL). Since c-pchart is not actively maintained, this could be
> tricky.

+1

> 1) Try to convince Ampache to stop using this package, since it is not truly
> Free Software.

+1

Other option: make use of this library optional in ampache. (but this, of course,  have to be discussed with upstream).

The IMHO ampache have a big issue bundling this lib.

> I will probably start both options in parallel. Should we mark this as
> fedora-review- for now, or should we keep it open to see what happens with
> the above options? I'm fine with either way.

I'm also fine with both way.

Comment 15 Randy Barlow 2017-03-14 04:12:32 UTC
I wrote upstream to ask them to clarify their license. It was difficult to find out how to contact them as their forum doesn't send registration confirmation e-mails (and hasn't had a post in years). I found an e-mail address on their website. So far I haven't heard back. If I don't hear anything within a week, I will begin talking with Ampache about it.

Another possibility might be to find out if this dependency can be easily removed from Ampache downstream, depending on what it is used for and how important that functionality is.

Comment 16 Randy Barlow 2017-03-19 16:38:51 UTC
I have now also filed an issue with c-pchart describing the issue and asking if they know how to contact upstream:

https://github.com/szymach/c-pchart/issues/35

Comment 17 Randy Barlow 2017-03-30 02:26:11 UTC
I have been unable to get in touch with upstream pchart after ~2.5 weeks. Downstream c-pchart says they don't know how to get in touch either, though they suggested trying Linked In so I did.

I have now filed an issue with Ampache (the real reason I want to package this library, since Ampache depends on it) informing them of the problem and suggesting some possible workarounds:

https://github.com/ampache/ampache/issues/1515

What should we do with this ticket? Should we close it for now, and reopen it if we ever do get upstream pchart and downstream c-pchart to both change their licensing statements?

Comment 18 Remi Collet 2017-03-30 04:56:22 UTC
> What should we do with this ticket? Should we close it for now, and reopen
> it if we ever do get upstream pchart and downstream c-pchart to both change
> their licensing statements?

Yes, close it or set "notready" in flags

Comment 19 Tom "spot" Callaway 2017-03-30 14:38:40 UTC
Agree with Remi in Comment 18.

Comment 20 Randy Barlow 2017-11-26 16:46:36 UTC
I spent some time messing with my Ampache installation this morning and I was able to get it to run without c-pchart. In fact, I was unable to find anything in the web ui that makes graphs in the first place (perhaps it is a plugin I don't have enabled), so I am not even able to find functionality that I use that is lost.

Thus, I think I can proceed with packaging Ampache with the caveat that I will have to patch out its code that uses this library.


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