Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1362490 - Review Request: libphutil - A collection of PHP utility classes
Summary: Review Request: libphutil - A collection of PHP utility classes
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Andreas Schneider
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1362487 1362491
TreeView+ depends on / blocked
 
Reported: 2016-08-02 11:04 UTC by Jeroen van Meeuwen
Modified: 2020-07-10 08:10 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-07-10 08:10:11 UTC
Type: ---
Embargoed:
asn: fedora-review?


Attachments (Terms of Use)
phpci.log (deleted)
2016-08-09 06:05 UTC, Remi Collet
no flags Details

Description Jeroen van Meeuwen 2016-08-02 11:04:44 UTC
Spec URL: https://kanarip.fedorapeople.org/phabricator/libphutil.spec
SRPM URL: https://kanarip.fedorapeople.org/phabricator/libphutil-20160727.git5fd2cf9-3.1.el7.src.rpm
Description: A collection of PHP utility classes used with phabricator
Fedora Account System Username: kanarip

Comment 1 Jeroen van Meeuwen 2016-08-02 11:12:14 UTC
Please note that this package may need a bunch of work, including but not limited to the items already collected through analysis from Tim.

https://bitbucket.org/tflink/phabricator-dist/raw/c21a9d58b00f94fce779ba8b2d01bd54bae8195a/fedora-packaging-notes.rst

Comment 2 Tim Flink 2016-08-03 11:58:44 UTC
I just went back to check and the note I made about arcanist needing a local copy of pep8 is no longer valid.

Comment 3 Remi Collet 2016-08-09 06:03:00 UTC
Quick look.


1/    cp -a * %{buildroot}%{_datadir}/%{name}/

This look very ugly, I think lot of files are unneeded at runtime (documentation, license, tests...)

2/ JsonLInt is available as system library: php-jsonlint-1.4.0

    $jsonlint_root = phutil_get_library_root('phutil').'/../externals/jsonlint';
    require_once $jsonlint_root.'/src/Seld/JsonLint/JsonParser.php';
    require_once $jsonlint_root.'/src/Seld/JsonLint/Lexer.php';
    require_once $jsonlint_root.'/src/Seld/JsonLint/ParsingException.php';
    require_once $jsonlint_root.'/src/Seld/JsonLint/Undefined.php';
=>
    require_once '/usr/share/php/Seld/JsonLint/autoload.php';

3/ Requires:   php-common >= 5

See PHP Guidelines, and attached phpci.log (phpcompatinfo static analysis result)
You must not use package names, but extensions name. Version doesn't make sense (we don't have anything < 5)

Requires: php-reflection
Requires: php-simplexml
Requires: php-ctype
Requires: php-curl
Requires: php-date
...

apc, memcache, xhprof are very probably optional, need code check.

4/ you cannot keep jsonlint in the archive, you have to remove it. Non-free software must go in the repo, even in .src.rpm.

Comment 4 Remi Collet 2016-08-09 06:05:03 UTC
Created attachment 1189040 [details]
phpci.log

phpCompatInfo version 5.0.1 DB version 1.11.0 built Jul 26 2016 06:33:42 CEST

Generated by: phpcompatinfo analyser:run  libphutil-5fd2cf9d5ddd38424a54a8fba02398d527639970/ --output phpci.log --no-ansi

Results give good information but are not 100% reliable, need to be checked.

Comment 5 Remi Collet 2016-08-09 06:06:18 UTC
Of course, I mean : "Non-free software must NOT go in the repo, even in .src.rpm"

Comment 6 Jeroen van Meeuwen 2016-08-09 12:10:01 UTC
== Bundling ==

(In reply to Remi Collet from comment #5)
> Of course, I mean : "Non-free software must NOT go in the repo, even in
> .src.rpm"

I understand this to mean vendorized or bundled rather than non-Free, or am I misunderstanding something? I would like sufficient clarification to distinguish between needing to repack the upstream sources excluding the externals/jsonlint/ directory from what is shipped as the Source0 in the SRPM, or alternatively just not install it with the binary package.

In any case, I agree we need to not ship it in the binary package and instead use the system php-jsonlint. I'll be working on a patch for it.

I think I might have it sorted for the next .spec/.srpm I include

== Seemingly Superfluous Requirements ==

The requirements could be split up between a set that is at the very core, a set that is needed for arcanist, the client-side command-line (Bug #1362487) and a set that is needed for phabricator, the server-side web interface and API (xhprof, apc|apcu, opcache, etc., Bug #1362491). We can do one worse by also sub-packaging phabricator-devel (xhprof comes to mind).

Note that, currently, arcanist and phabricator currently list their own sets of dependencies satisfying the needs of the corresponding parts actually in libphutil. These would be cleaned up.

== Requiring MySQL ==

I'm not certain what is the recommended route for requiring mysql.

'php-mysql' used to be provided by both the php-mysql as well as the php-mysqlnd sub-packages, but today unless I clause %{?rhel} out I cannot require 'php-mysql' in rawhide. I choose 'php-mysqli' which seems to still be provided by either or both of the packages across platforms. Note that the 'Obsoletes: php-mysql < x.y.z' in the current php-7 package in rawhide does not provide the equivalent nevra for 'php-mysql'.

== Versioning Scheme ==

Upstream does not use versioning. They maintain a level of compatibility between days, weeks or months of releases of libphutil, arcanist and phabricator, but basically only ever move forward with deprecation warnings.

I would like to use the Version to have packages depend on YYYY, MM and DD (of the last commit made to the stable branches of each GIT repository), so that I can let other packages depend on a dated version greater than some months ago and before some months ahead.

At Flock however, jsmith re-iterated a packaging requirement that non-versioned software should have a version of 0, and a release of 0.$x.$date.git$id.%{?dist}, which I believe would void the requires and provides scenario I'm proposing we use for libphutil, arcanist and phabricator.

However, perhaps we could/should move the git%{git_short_version_hash} over to the Release tag.

Spec URL: https://kanarip.fedorapeople.org/phabricator/libphutil.spec
SRPM URL: https://kanarip.fedorapeople.org/phabricator/libphutil-20160727.git8f8e02d-1.fc26.src.rpm

Comment 7 Remi Collet 2016-08-09 12:18:47 UTC
Sorry, I missread, I was thinking there is "jsmin"... 
So, not no-free issue.

Comment 8 Remi Collet 2016-08-09 12:25:31 UTC
1/ XHprof is a dead project (damned because first developed by Facebook, which forgive it as they use HHVM... and nobody can take ownership because of a strange CLA). Have been removed from Fedora (BTW various fork exists, but none are packaged for now)

2/ Mysql

Need code inspection.

If this package really requires "mysql" extension, you have an issue, this is dead (deprecated for years, removed from PHP 7) and no more available in Fedora.

> Note that the 'Obsoletes: php-mysql < x.y.z' in the current php-7 package in rawhide does not provide the equivalent nevra for 'php-mysql'.

This is for the update from php-mysql package to php-mysqlnd package (with 5.6 IIRC). Can probably be cleaned, in all case can't be provided.

MySQLi is a different extension.

Comment 9 Remi Collet 2016-08-09 13:25:49 UTC
> Requires:   php-pecl-apcu

From phpcompatinfo reports, this app use APC old API.

So you really need to requires this API

Requires:   php-pecl(APC)

Which is provided by php-pecl-apc (PHP 5.3) or php-pecl-apcu (php 5.x) or php-pecl-apcu-bc (php 7.x)


> Requires:   php-pecl-zendopcache

IMHO opcache should be an administrator choice (so suggested or recommended). And use php-pecl(opcache) or php-opcache (we have a virtual provides for most extensions as php-<foo>... the only reliable stuff for ext moving from pecl to php or from php to pecl... I should really try to adapt the Guidelines about this)

Comment 10 Jeroen van Meeuwen 2016-08-09 15:47:50 UTC
Thanks Remi!

(In reply to Remi Collet from comment #8)
> 1/ XHprof is a dead project (damned because first developed by Facebook,
> which forgive it as they use HHVM... and nobody can take ownership because
> of a strange CLA). Have been removed from Fedora (BTW various fork exists,
> but none are packaged for now)
> 

No problem, I can just remove most of it.

> 2/ Mysql
> 
> Need code inspection.
> 
> If this package really requires "mysql" extension, you have an issue, this
> is dead (deprecated for years, removed from PHP 7) and no more available in
> Fedora.
> 
> > Note that the 'Obsoletes: php-mysql < x.y.z' in the current php-7 package in rawhide does not provide the equivalent nevra for 'php-mysql'.
> 
> This is for the update from php-mysql package to php-mysqlnd package (with
> 5.6 IIRC). Can probably be cleaned, in all case can't be provided.
> 
> MySQLi is a different extension.
> 

The code doesn't really require the -mysql extension (and no other mysql equivalent), I'm just wondering what the Requires: line should look like for it to require 'anything functionally equivalent to -mysql' across the different release streams (-5.3, -5.4, -5.6 and -7.x).

(In reply to Remi Collet from comment #9)
> > Requires:   php-pecl-apcu
> 
> From phpcompatinfo reports, this app use APC old API.
> 
> So you really need to requires this API
> 
> Requires:   php-pecl(APC)
> 
> Which is provided by php-pecl-apc (PHP 5.3) or php-pecl-apcu (php 5.x) or
> php-pecl-apcu-bc (php 7.x)
> 

I'll fix this.

> > Requires:   php-pecl-zendopcache
> 
> IMHO opcache should be an administrator choice (so suggested or
> recommended). And use php-pecl(opcache) or php-opcache (we have a virtual
> provides for most extensions as php-<foo>... the only reliable stuff for ext
> moving from pecl to php or from php to pecl... I should really try to adapt
> the Guidelines about this)
> 

I'd elect to use php-opcache then, since the application will report setup problems if the extension is missing, and would proceed to report setup problems if it is not also optimized for production. I cannot suggest nor recommend packages on %{?rhel}, as far as I know, so I'd prefer to give Fedorians the same experience.

However, I would let the phabricator package require this, rather than the libphutil package (libphutil is also required by the command-line arcanist). Same goes for -mysql, -ldap, -memcached, -apcu, and so forth.

Comment 11 Jeroen van Meeuwen 2016-08-09 18:17:32 UTC
I've split the server (phabricator) dependencies from the client (arcanist) dependencies, removed all references to xhprof, and adjusted the dependences as per Remi's comments.

Spec URL: https://kanarip.fedorapeople.org/phabricator/libphutil.spec
SRPM URL: https://kanarip.fedorapeople.org/phabricator/libphutil-20160727.git8f8e02d-2.fc26.src.rpm

Comment 12 Remi Collet 2016-08-10 05:46:29 UTC
> The code doesn't really require the -mysql extension (and no other mysql
> equivalent), I'm just wondering what the Requires: line should look like for
> it to require 'anything functionally equivalent to -mysql' across the
> different release streams (-5.3, -5.4, -5.6 and -7.x).

This doesn't work this way.
'anything functionally equivalent to -mysql' can't exists.


$ grep -rl mysqli_ src
src/aphront/storage/connection/mysql/AphrontMySQLiDatabaseConnection.php

So this part of the code use mysqli extension (http://php.net/mysqli)

=> Requires: php-mysqli


$ grep -rl mysql_ src
./src/aphront/storage/connection/mysql/AphrontMySQLDatabaseConnection.php

So this part of the code use the old deprecated mysql extension (http://php.net/manual/en/book.mysql.php)

=> Requires: php-mysql

Of course, this is a problem, as php-mysql is deprecated for years and doesn't exists anymore. Best is to drop this part, after checking it is not required.

Some app offers choice between mysql and mysqli (e.g. phpMyadmin), and so our package only offers

Comment 13 Remi Collet 2016-08-10 05:48:09 UTC
Some app offers choice between mysql and mysqli (e.g. phpMyadmin), and so our package only offers mysqli, no choice.

Comment 14 Jeroen van Meeuwen 2016-08-10 06:56:08 UTC
(In reply to Remi Collet from comment #12)
> > The code doesn't really require the -mysql extension (and no other mysql
> > equivalent), I'm just wondering what the Requires: line should look like for
> > it to require 'anything functionally equivalent to -mysql' across the
> > different release streams (-5.3, -5.4, -5.6 and -7.x).
> 
> This doesn't work this way.
> 'anything functionally equivalent to -mysql' can't exists.
> 

If I read your argument correctly, then the --with-mysql=shared,mysqlnd is finally deprecated in PHP 7. As such, I'll work on a patch to remove the MySQL connection adapter from Phabricator for 0%{?fedora} >= 25 and only offer MySQLi, and change the default.

Comment 15 Jeroen van Meeuwen 2016-08-10 07:02:00 UTC
Turns out it'll default to MySQLi if the extension is installed, so perhaps all I would need to do is depend on it ;-)

Comment 16 Remi Collet 2016-08-10 07:11:19 UTC
(In reply to Jeroen van Meeuwen from comment #14)
> If I read your argument correctly, then the --with-mysql=shared,mysqlnd is
> finally deprecated in PHP 7.

Yes: http://php.net/manual/en/migration70.removed-exts-sapis.php

Comment 17 Remi Collet 2016-08-10 07:12:38 UTC
Sorry, I means: No: deprecated since 5.5, "removed" in 7.0 ;)

Comment 19 Remi Collet 2016-08-10 11:37:21 UTC
> Requires:   php-mysqlnd

But... why this one ?
The worst possible. You DON'T use mysqlnd extension (not even an real extension, more a "driver" used by other ext: mysql, mysqi, pdo_mysql)

Again, if you need "foo" extension (from code), requires: php-foo (never rely on package name, or on one extension being part of a package, the layout may change)

Comment 20 Jeroen van Meeuwen 2016-08-16 07:29:36 UTC
You told me requiring 'php-mysql' was wrong in comment #12. I don't know what to do here any more, it seems everything is wrong for some or the other reason.

Tell me what to set it to, please, so we can get back to the business of getting this over with.

Comment 21 Tim Flink 2016-09-06 18:51:26 UTC
Remi - if 'php-mysql' and 'php-mysqlnd' are both incorrect dependencies, what is the correct one? From poking around a bit, the pyp-mysqlnd package provides 'php-mysql' and I'm not seeing any other obvious candidates to fulfill the need for a mysql interface

Comment 22 Remi Collet 2016-09-07 06:04:56 UTC
Again, if you need 'mysql' extension, as this have been removed from PHP, you cannot package this (in Fedora)

So you have to drop AphrontMySQLDatabaseConnection from the package (and perhaps ensure app will not try to use it)

As AphrontMySQLiDatabaseConnection use mysqli extension, you have to requires php-mysqli.

Again, you must require all used extensions, and never package name.

Comment 23 Tim Flink 2016-09-13 16:37:31 UTC
I've changed the mysql requires to use php-mysqli, removed AphrontMySQLDatabaseConnection during prep and removed references to it in the library map.

Spec URL: https://tflink.fedorapeople.org/packages/phabricator-review/libphutil/libphutil.spec
SRPM URL: https://tflink.fedorapeople.org/packages/phabricator-review/libphutil/libphutil-20160806.git8f8e02d-2.fc26.src.rpm

Comment 24 Jared Smith 2016-09-23 20:15:27 UTC
You're going to need to adjust the license tag on this package.  Licensecheck found several licenses other than Apache in parts of the code base:

GPL (v3 or later)
-----------------
libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/support/xhpast/parser.yacc.cpp
libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/support/xhpast/parser.yacc.hpp

MIT/X11 (BSD like)
------------------
libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/externals/jsonlint/LICENSE

You should also set the Version: tag to 0 and the Release: tag to 0.1.20160806.git%{git_short_version_hash}%{dist} 

If you were to do a second build with that same git hash, it would then be 0.2.20160806.git%{git_short_version_hash}%{dist}, and so on.

This way, if/when a version 1.0 comes out, it will be greater than version 0.x.2016whatever.

Comment 25 Tim Flink 2016-09-23 20:44:57 UTC
(In reply to Jared Smith from comment #24)
> You're going to need to adjust the license tag on this package. 
> Licensecheck found several licenses other than Apache in parts of the code
> base:
> 
> GPL (v3 or later)
> -----------------
> libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/support/xhpast/parser.
> yacc.cpp
> libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/support/xhpast/parser.
> yacc.hpp

Bah, I thought those were covered under the special GPL exception for bison output but I missed the part about "as long as it isn't used as part of a parser generator'.

> MIT/X11 (BSD like)
> ------------------
> libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/externals/jsonlint/LICENSE

Why couldn't this be distributed as APL2? Ignoring the GPL3 issue with the bison output, I mean.

> You should also set the Version: tag to 0 and the Release: tag to
> 0.1.20160806.git%{git_short_version_hash}%{dist} 
> 
> If you were to do a second build with that same git hash, it would then be
> 0.2.20160806.git%{git_short_version_hash}%{dist}, and so on.
> 
> This way, if/when a version 1.0 comes out, it will be greater than version
> 0.x.2016whatever.

That makes sense but I'm going to see if I can get more out of Jeroen WRT the potential requires/provides  issue he mentioned. I'm not sure if that's been tested at all or if there was a plan to work around the concerns.

Comment 26 Jared Smith 2016-09-26 14:18:04 UTC
(In reply to Tim Flink from comment #25)
> > MIT/X11 (BSD like)
> > ------------------
> > libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/externals/jsonlint/LICENSE
> 
> Why couldn't this be distributed as APL2? Ignoring the GPL3 issue with the
> bison output, I mean.

Because it's not licensed under the APL2 license.  It's licensed under an MIT-like license.  You're not the copyright holder, so you can't change the license on the software.

Comment 27 Jeroen van Meeuwen 2016-12-01 10:41:14 UTC
* Removed externals/jsonlint/ from the built RPM

* Changed the versioning to 0.$date.git$hash

Spec URL: https://kanarip.fedorapeople.org/phabricator/libphutil.spec 
SRPM URL: https://kanarip.fedorapeople.org/phabricator/libphutil-0.20161126.git89984ac-2.el7.src.rpm

Comment 28 Jeroen van Meeuwen 2016-12-01 13:57:02 UTC
(In reply to Jared Smith from comment #26)
> (In reply to Tim Flink from comment #25)
> > > MIT/X11 (BSD like)
> > > ------------------
> > > libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/externals/jsonlint/LICENSE
> > 
> > Why couldn't this be distributed as APL2? Ignoring the GPL3 issue with the
> > bison output, I mean.
> 
> Because it's not licensed under the APL2 license.  It's licensed under an
> MIT-like license.  You're not the copyright holder, so you can't change the
> license on the software.

We should be able to remove the corresponding directory /usr/share/libphutil/externals/jsonlint/ from the installed RPM, would the license still be a concern per the contents of the source tarball?

(In reply to Tim Flink from comment #25)
> (In reply to Jared Smith from comment #24)
> > You're going to need to adjust the license tag on this package. 
> > Licensecheck found several licenses other than Apache in parts of the code
> > base:
> > 
> > GPL (v3 or later)
> > -----------------
> > libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/support/xhpast/parser.
> > yacc.cpp
> > libphutil-8f8e02d47569dce5f035383d8bcbf7a08481e839/support/xhpast/parser.
> > yacc.hpp
> 
> Bah, I thought those were covered under the special GPL exception for bison
> output but I missed the part about "as long as it isn't used as part of a
> parser generator'.
> 

We don't really ship support/, and so effectively it isn't used (let alone as part of a parser generator). This makes this result in the same question as above -- source tarball license(s) or shipped content?

Comment 30 Jared Smith 2016-12-21 17:02:03 UTC
This requires a version of PHP earlier than 7.x, but Rawhide now has 7.1.0.  How would you like to proceed?

Comment 31 Tim Flink 2016-12-21 18:58:20 UTC
From what I've understood, the problem is mostly with 7.0.x and upstream is planning to support 7.1+, skipping direct support of the 7.0.x range.

As far as what to do about F25, I think that the code which doesn't work in 7.0.x is relatively small and could probably be patched out but Jeroen is following the issue more closely than I.

Comment 32 Adam Williamson 2017-02-09 17:05:05 UTC
Upstream: https://secure.phabricator.com/T9640

claims that as of 2017-01-12, Phab supports PHP 7. I suspect Jeroen's most recent snapshot build might just work as-is with only the dep changed, as it is datestamped one day after all the commits landed upstream, but I haven't checked.

Comment 33 Andreas Schneider 2017-06-20 15:47:47 UTC
I'm interested in this package however the stable version works fine with f26 so I've created a spec file for it.

Please take a look at:

Spec URL: https://xor.cryptomilk.org/rpm/php-libphutil/php-libphutil.spec
SRPM URL: https://xor.cryptomilk.org/rpm/php-libphutil/php-libphutil-0.0-0.1.20170605git612619d.fc26.src.rpm

According to https://fedoraproject.org/wiki/Packaging:Versioning the snapshot needs to be part of the release tag.

Comment 34 Robert-André Mauchin 🐧 2017-10-05 15:48:55 UTC
Jeroen van Meeuwen: If you update it for php7, I'll try to review it.

Comment 35 Andreas Schneider 2017-10-19 13:58:05 UTC
Check comment #33 which is php7.

Comment 36 Package Review 2020-07-10 00:55:01 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.


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