Spec URL: https://www.happyassassin.net/reviews/php-bantu-ini-get-wrapper/php-bantu-ini-get-wrapper.spec SRPM URL: https://www.happyassassin.net/reviews/php-bantu-ini-get-wrapper/php-bantu-ini-get-wrapper-1.0.1-1.fc22.src.rpm Description: Convenience wrapper around ini_get(). Fedora Account System Username: adamwill
This is one of the new 3rdparty deps that ownCloud 8.x will require, I'm trying to get out ahead of them. We still haven't (I don't think?) decided on an official strategy for packaging and unbundling PSR-4 autoloaded libraries. However, one possibility is perhaps to do something with Composer's ${vendorDir} setting. If you look at composer/autoload_psr4.php for ownCloud 3rdparty git master, you see: $vendorDir = dirname(dirname(__FILE__)); $baseDir = $vendorDir; return array( 'bantu\\IniGetWrapper\\' => array($vendorDir . '/bantu/ini-get-wrapper/src'), 'Punic\\' => array($vendorDir . '/punic/punic/code'), ); so I decided as a first cut to install this package such that the path /bantu/ini-get-wrapper/src - that is, /(vendor)/(name)/(PSR-4 base directory) - is valid relative to /usr/share/php . As a first cut that sort of intuitively feels like the most appropriate thing to do, to me. The 'autoloader' for purpose of running the unit tests is of course ridiculous and would break the moment upstream touches pretty much anything. A more elegant approach would, I suppose, be to have some sort of PSR-4 autoloader implementation available for this kind of purpose, and some scriptlets to use it in a particular package build.
(In reply to Adam Williamson (Fedora) from comment #1) > We still haven't (I don't think?) decided on an official strategy for > packaging and unbundling PSR-4 autoloaded libraries. I think PSR-4 have no sense ;) So common practice (for already some packaged libraries) is to restore / provides PSR-0 tree As IniGetWrapper.php provides bantu\IniGetWrapper\IniGetWrapper Should be installed as /usr/share/php/bantu/IniGetWrapper/IniGetWrapper.php (and /usr/share/php/bantu/ini-get-wrapper/src/IniGetWrapper.php seems messy ;) Additional tips sed -e 's,colors="true",colors="false" => no more needed with latest version ;) -d date.timezone="UTC" => also no more needed
(In reply to Adam Williamson (Fedora) from comment #1) > The 'autoloader' for purpose of running the unit tests is of course > ridiculous and would break the moment upstream touches pretty much anything. I use a lot the "phpab" command which allow to create a very simple autoloader. For example, for this library: $ phpab --output bootstrap.php --exclude *Test.php --basedir . src tests phpab 1.16.2 - Copyright (C) 2009 - 2014 by Arne Blankerts Scanning directory src Scanning directory tests Autoload file bootstrap.php generated. $ phpunit --bootstrap bootstrap.php PHPUnit 4.4.0 by Sebastian Bergmann. Configuration read from /tmp/php-bantu-ini-get-wrapper-1.0.1-1.fc22.src/php-ini-get-wrapper-1.0.1/phpunit.xml.dist ............................................................ Time: 55 ms, Memory: 4.75Mb OK (60 tests, 60 assertions) In some case, it could even make sense to provide a generated autoload.php, which could make it simpler for consumers of the library (see all the PHPUnit stack for example). $ phpab --output src/autoload.php src At least, this autoloader is correct (not like the tricky composer one, which is not even not PSR-0 compliant...)
I forget to say, big plus for using a PSR-0 tree, it can be used by PSR-0 and PSR-4 autoloaders ;)
My only real problem with PSR-0ing PSR-4 libraries is, as I said in email, that we can't entirely rely on them being uniquely named purely by class path, or about upstream caring if we do wind up hitting collisions (because they'll just say 'oh, well, it's named by composer vendor'). But if you want to do that, we can go for it until the problem happens ;) At least PSR-4 requires the classes to be namespaced, which helps. It may never happen. "I forget to say, big plus for using a PSR-0 tree, it can be used by PSR-0 and PSR-4 autoloaders ;)" As I read it, this isn't strictly true. PSR-4: "When loading a file that corresponds to a fully qualified class name ... A contiguous series of one or more leading namespace and sub-namespace names, not including the leading namespace separator, in the fully qualified class name (a "namespace prefix") corresponds to at least one "base directory"." It requires "one or more" "namespace and sub-namespace names" to "corresponds to at least one "base directory". By my reading, loading a class strictly by class path isn't part of PSR-4, and you could write a PSR-4 autoloader which didn't do it. The Composer autoloader, which is the one we're going to encounter most often, (optionally) does, though, of course. (I agree with you that PSR-4 is a horrible thing, apart from anything else you could drive a bus through the ambiguities and assumptions in the spec...) Thanks for the phpab tip, I'd seen that in a spec or two but forgot :) I'll submit a revised version with PSR-0 layout and phpab loader, and the phpunit changes (I stole them from a spec I had lying around, as you can probably guess).
spec updated, new .src.rpm: https://www.happyassassin.net/reviews/php-bantu-ini-get-wrapper/php-bantu-ini-get-wrapper-1.0.1-1.fc21.src.rpm (fc21 not fc22, building from my laptop).
I really think PSR-4 have no collision risk. All libraries/classes name use a <prefix> vendor (as in PSR-0, even if some libraries doesn't have it...) PSR-4 is only a shorcut for distribution (simplified src tree) PSR4: you find vendor\floo classes in /usr/share/php/vendor/foo so vendor\foo\bar => /usr/share/php/vendor/foo/bar.php PSR-0 you find vendor\floo classes in /usr/share/php so vendor\foo\bar => /usr/share/php/vendor/foo/bar.php PSR-4 have just dropped the compat case for not namespaced classes (from old PEAR time)
Quick notes: I understand why you were confused by PSR-4: you forget the <vendor> part in the installation tree ;) %global php_vendor bantu %global php_namespace %{php_vendor}/IniGetWrapper mkdir -p %{buildroot}%{_datadir}/php/%{php_namespace} cp -pr src/* %{buildroot}%{_datadir}/php/%{php_namespace} %files ... %{_datadir}/php/%{php_vendor} From PHP Guidelines => https://fedoraproject.org/wiki/Packaging:PHP#Composer_registered_Packages "Packages must not require any php-pear(foo), but should use php-composer(pear/foo). " This is designed to be able to follow pear requirement, and when pear will totally disappear. So -BuildRequires: php-pear(pear.phpunit.de/PHPUnit) +BuildRequires: php-phpunit-PHPUnit or +BuildRequires: %{_bindir}/phpunit (php-composer(phpunit/phpunit) is not yet provided in all branches, and the library is not really required, so I think BR the command is the better) From FPC approved Guidelines, even if I think Wiki have never been updated... :( => https://fedorahosted.org/fpc/ticket/411 %{!?_licensedir:%global license %%doc} %license LICENSE From https://fedoraproject.org/wiki/Packaging:SourceURL#Github Source0 should use "commit" (4770c7feab370c62e23db4f31c112b7c6d90aee2) not the tag (v1.0.1) Please use %{?dist} instead of %{dist}, at least, to make fedora-review happy ;)
"I understand why you were confused by PSR-4: you forget the <vendor> part in the installation tree ;)" Huh. Well, no, I didn't forget it - I was sure I checked the source and it was just namespace IniGetWrapper . But now I look at it again and it's not. Odd. I'd say PSR-4 does not really require the library be namespaced by vendor. It only strictly requires it to have a namespace. It says the top-level namespace is "also known as a "vendor namespace"", but doesn't really make the leap to *requiring* the top-level namespace name to be a 'vendor'. It only strictly requires a top-level namespace and a class name. Quote: "A fully qualified class name has the following form: \<NamespaceName>(\<SubNamespaceNames>)*\<ClassName> The fully qualified class name MUST have a top-level namespace name, also known as a "vendor namespace". The fully qualified class name MAY have one or more sub-namespace names. The fully qualified class name MUST have a terminating class name." But we're not really discussing the review, at this point...:) On the github thing, frankly I find the policy extremely ambiguous, and I'm not really sure which way to read it. It says: "Github provides a mechanism to create tarballs on demand, either from a specific commit revision, or from a specific tag. If the upstream does not create tarballs for releases, you can use this mechanism to produce them. If the upstream does create tarballs you should use them as tarballs provide an easier trail for people auditing the packages." There's a thread on packaging list: https://lists.fedoraproject.org/pipermail/packaging/2014-September/010288.html I used to read it as requiring the use of a commit id for all github sources, but I'm really not so sure any more, as that seems a fairly absurd policy and I'm not sure it was ever the intent. The v1.0.1.tar.gz tarball comes from the releases page: https://github.com/bantuXorg/php-ini-get-wrapper/releases I really think the bit of the guideline about "If the upstream does not create tarballs" and "If the upstream does create tarballs" needs clarifying, I don't find it at all easy to interpret. I really kind of hate those commit ID tarballs and would much prefer to use the v1.0.1.tar.gz one if at all possible. I'll fix up the other bits, thanks (I haven't been following FPC changes lately, I did have a quick look at the review guidelines the other day and I don't think the license thing has been changed).
spot: can you clarify the intent of the github Source0 policy? is it not allowed to use https://github.com/bantuXorg/php-ini-get-wrapper/archive/v1.0.1.tar.gz ? Thanks.
new spec and .src.rpm up with things other than the tarball addressed; I'm leaving the tarball as-is for now pending clarification.
Created attachment 968360 [details] phpci.log phpCompatInfo version 3.7.0 static analyze results
Created attachment 968361 [details] review.txt Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
MUST [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/share/php/bantu [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/php/bantu SHOULD [!]: Dist tag is present (not strictly required in GL). Directory ownership is the only blocker.
Since upstream made that release tarball, you can use it without issue. The intent of the Github section here (https://fedoraproject.org/wiki/Packaging:SourceURL) is to refer to manually generated tarballs.
tom: would it be possible to clarify the guidelines a bit? I've seen this question come up before, and I read the guidelines the same way as Remi at first. I can try and work up a draft if it would be of interest. thanks for the info.
No, please, don't fix our guidelines, I couldn't bear it! </sarcasm>
Well, pants, now I find the references for this stuff: https://fedorahosted.org/fpc/ticket/252 https://fedorahosted.org/fpc/ticket/233 https://fedorahosted.org/fpc/ticket/233#comment:9 which suggest the intent really *is* 'don't ever use github-generated tarballs from tags', the justification being that "Yes, the problem is that what commit a version points to can change while a commitid can't change. So if you want to download the same tarball you can't use a version." Since the guideline was drafted github invented the 'Releases' workflow, which is really just a bit of extra metadata on top of a tag AFAICT. The guideline also don't seem to have taken into account the difference between lightweight and annotated tags, because - as discussed in the thread I referred to earlier, https://lists.fedoraproject.org/pipermail/packaging/2014-September/010288.html - 'parse-rev' doesn't give you a commit ID for an annotated tag, it gives you the tag object's ID. But neither of those makes a difference to that justification (or the file mtime justification). I just checked and you can edit both annotated tags and github 'Releases' after creating them, so neither is immutable. And github still generates the tarballs on the fly, so they have their mtime set to whenever you download them. I'm probably not willing to get up on the horse and challenge the 'tags are mutable' rationale for not allowing github-generated tag tarballs, so I'll respect the apparent intent of the existing guideline and switch to a commit-based tarball (grmph). The wording about "If the upstream does create tarballs you should use them", when read in context in #233, appears to have been added by Toshio as he was worried the guideline would be read as applying to *any project hosted in github* even if it maintained a release archive with curated tarballs. I suppose the guideline still needs updating to provide a correct command for finding the commit ID for annotated tags, and perhaps to clarify this stuff since we (or at least I...) seem to keep tripping over it.
OK, revised (hopefully) one more time: Spec URL: https://www.happyassassin.net/reviews/php-bantu-ini-get-wrapper/php-bantu-ini-get-wrapper.spec SRPM URL: https://www.happyassassin.net/reviews/php-bantu-ini-get-wrapper/php-bantu-ini-get-wrapper-1.0.1-1.fc22.src.rpm Using github commit ID this time, and globals for the namespacing tweaked *again* so directory ownership should be right now.
Yes all is about immutable source URL... [x]: Package must own all directories that it creates. SHOULD [x]: Dist tag is present (not strictly required in GL). === APPROVED ===
New Package SCM Request ======================= Package Name: php-bantu-ini-get-wrapper Short Description: Convenience wrapper around PHP's ini_get() function Upstream URL: https://github.com/bantuXorg/php-ini-get-wrapper Owners: adamwill Branches: f20 f21 el6 epel7 InitialCC:
Git done (by process-git-requests).