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
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
I just went back to check and the note I made about arcanist needing a local copy of pep8 is no longer valid.
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.
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.
Of course, I mean : "Non-free software must NOT go in the repo, even in .src.rpm"
== 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
Sorry, I missread, I was thinking there is "jsmin"... So, not no-free issue.
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.
> 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)
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.
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
> 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
Some app offers choice between mysql and mysqli (e.g. phpMyadmin), and so our package only offers mysqli, no choice.
(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.
Turns out it'll default to MySQLi if the extension is installed, so perhaps all I would need to do is depend on it ;-)
(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
Sorry, I means: No: deprecated since 5.5, "removed" in 7.0 ;)
Spec URL: https://kanarip.fedorapeople.org/phabricator/libphutil.spec SRPM URL: https://kanarip.fedorapeople.org/phabricator/libphutil-20160806.git8f8e02d-1.fc26.src.rpm
> 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)
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.
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
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.
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
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.
(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.
(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.
* 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
(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?
Spec URL: https://kanarip.fedorapeople.org/phabricator/libphutil.spec SRPM URL: https://kanarip.fedorapeople.org/phabricator/libphutil-0.20161203.git3230179-1.el7.src.rpm
This requires a version of PHP earlier than 7.x, but Rawhide now has 7.1.0. How would you like to proceed?
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.
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.
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.
Jeroen van Meeuwen: If you update it for php7, I'll try to review it.
Check comment #33 which is php7.
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.