|Summary:||Review Request: php-szymach-c-pchart - A project bringing composer support and PSR standards to pChart 2.0|
|Product:||[Fedora] Fedora||Reporter:||Randy Barlow <randy>|
|Component:||Package Review||Assignee:||Remi Collet <fedora>|
|Status:||CLOSED CANTFIX||QA Contact:||Fedora Extras Quality Assurance <extras-qa>|
|Version:||rawhide||CC:||fedora, package-review, tcallawa|
|Fixed In Version:||Doc Type:||If docs needed, set a value|
|Doc Text:||Story Points:||---|
|Last Closed:||2017-03-31 02:24:28 UTC||Type:||---|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Bug Depends On:|
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:32 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:30 UTC
Created attachment 1260013 [details] system-fonts.patch New version, instead of always use DejaVu, use a substitution map.
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:32 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:07 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://firstname.lastname@example.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 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.