Quick notes:

See https://github.com/rackspace/php-opencloud/blob/master/composer.json

There are some additional dependencies.
guzzle/http (mandatory)
guzzle/plugin-mock
guzzle/plugin-log
psr/log

As this library is PSR-0 compliant, it will be cleaner to install it so any PSR-0 aware app can load it, simply relying on include patch [1], so 
   /usr/share/php/OpenCloud

You can drop the "lib/php-opencloud.php" (this file is composer specific).

composer.json don't need to be installed (so can add it in %doc if you want, as it provides some interesting info).


[1] See http://fedoraproject.org/wiki/Packaging:PHP#File_Placement

(In reply to Remi Collet from comment #1)
> Quick notes:
> As this library is PSR-0 compliant, it will be cleaner to install it so any
> PSR-0 aware app can load it, simply relying on include patch [1], so 
>    /usr/share/php/OpenCloud
> 
> You can drop the "lib/php-opencloud.php" (this file is composer specific).

makes sense, but what to do with the autoloader? Note that the old 1.6.0 contains some more files in lib.

owncloud, on the other side, requires lib/openstack.php which registers all classes, I assume.

Ok, my mistake, I have look in master (so 1.8.x).

1.6 is not really PSR-0 compliant because of those files

In all case for 1.8 you will have to either
- provide a working autoloader (not relying on composer)
- adapt owncloud autoloader to be able to load this PSR-0 library

Second seems better .

lib/openstack.php => load php-opencloud.php => load Autoload.php (which is the composer autoloader, "bundled"), so really should drop all those files.

I've been looking into making OC's loading of 3rdparty libs for apps sane today. AFAICS, OwnCloud's autoloader doesn't handle libs in apps/some_app/3rdparty directories. It only handles core stuff. It might be nice to make it handle this well - something like look in apps/some_app/3rdparty when something from apps/some_app tries to access a function - but I don't think it's trivial, and I'm not sure they'd want to take patches which simply added apps/some_app/3rdparty/foo to the autoloader *unconditionally*.

right now I'm working on a patch set which would make all apps consistent in how they do things: add some_app/3rdparty to the include_path, then require_once whatever they need to require relative to that. But for us not to carry patches downstream, that does require us to use the same layout in /usr/share/php or /usr/share/pear or whatever that OC has in some_app/3rdparty .

Upstream is very responsive, though, and I doubt they particularly *care* about the directory layouts under 3rdparty/ directories. I suspect we might get mileage out of submitting patches upstream to make *their* layout match *ours*, when we don't want to make *our* layout match *theirs*. Especially if we can make a good case for the change, like here, for PSR-0 compliance.

Hmm... shouldn't both layout match upstream layout ? ;)

So it should only be a matter of root tree path ?
Have you specific case with layout issue ?

"shouldn't both layout match upstream layout ? ;)"

I'm not entirely sure about that, actually, and we should probably have a policy. I find layouts like this pretty ugly:

/usr/share/php/foo/lib/monkeys/hellolookshiny/here_are_the_damn_libraries_at_last/*.php

but that's how upstreams frequently provide things - there's a bunch of bumpf in one, two or even three directory levels above where the actual bit we care about is. In theory we could strip those.

But probably it's safest just to replicate the layout of upstream's most preferred static distribution method for non-PSR0 stuff, I guess, and ensure Debian and OC do the same (with the 'root' as /usr/share/php for distros and owncloud/3rdparty for OC, of course). Which is what Gregor did for php-opencloud 1.6.0 indeed.

Oh, I do see an issue here, though not necessarily ours: Debian stripped the directory structure in their 1.6.0 package. They dropped the lib/ directory and moved the autoloader and the legacy entry points up to /usr/share/php/php-opencloud .

OwnCloud keeps the lib/ directory, like we do, so we can unbundle cleanly with this layout, but we're inconsistent with Debian, and I'm hoping to keep those lined up.

It should all go away for 1.8, though, so possibly not a huge deal.

Spec URL: http://brummbq.fedorapeople.org/php-opencloud.spec
SRPM URL: http://brummbq.fedorapeople.org/php-opencloud-1.6.0-2.fc19.src.rpm

I've followed remis suggestion from comment 1 and moved the lib to php/OpenStack. The autoloader files aren't part of this pkg any more.

@adam
as it turns out the owncloud autoloader is quite capable of loading psr-0 libs. Thanks to your autoloader-relative branch the loader picks the openstack classes up without a problem. You just need to set the proper include_path.

Gregor: I'm still buried deep in autoloader theory, but I'm starting to think my approach is bad and yours is good. The convention seems to be to be relatively conservative with autoloading: only use it on classes that are explicitly registered with it, and have the autoloader implement some kind of prefix handling rather than using the include_path.

I'll probably send an alternative PR which is closer to your approach, but tries to be efficient for the case where upstream's 3rdparty directory is present. I'm currently studying the Composer autoloader, which is quite an advanced one, for hints.

I *do* think any reasonable autoloader implementation ought to try loading the PSR-0 (or PSR-4! look that one up :/) class path relative to the defined include path as a fallback if it can't find the class any other way, which is roughly the approach you took. At least, this morning I do. Maybe this afternoon I'll find another layer of this onion and change my mind :/

My new PR for OC's autoloader is https://github.com/owncloud/core/pull/6628 . It should still find anything that's in a PSR-0 compliant location wrt the include_path, and so does your patch that we currently use. So packaging it this way should be fine for OC, but might possibly be a problem for anything else which tries to include php-opencloud's autoloader and doesn't have its own autoloader. still, upstream 1.8 expects you to bring your own autoloader (possibly via composer) anyway, so doesn't seem like a huge issue.

Oh, for composer stuff - composer's autoloader has the ability to fall back to trying to load the PSR-0 class path relative to the system include path (which is what we're implicitly expecting when we package things such that they're PSR-0 compliant wrt /usr/share/php) but it has a boolean that controls that function and it defaults to false. Still, that just means we'd have to flip one switch for anything that uses composer's autoloader to find our system copies of things.

Would it be sensible to include upstream's samples/ and docs/ as docs?

(In reply to Adam Williamson from comment #14)
> Would it be sensible to include upstream's samples/ and docs/ as docs?

What do you mean? I'm doing that right now in doc subpkg, not?

"What do you mean? I'm doing that right now in doc subpkg, not?"

Whoops, indeed you are - I missed that.

As Remi doesn't seem to be taking the review, I guess I will.

rpmlint
-------

php-opencloud-doc.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/php-opencloud-doc/docs/api/css/jquery.treeview.css
php-opencloud.src: W: strange-permission php-opencloud-1.6.0.tar.gz 0600L

not blockers, but worth fixing.

phpcompatinfo gives date, hash, json and pcre extensions, all listed in the spec - looks fine.

https://fedoraproject.org/wiki/Packaging:SourceURL#Github seems fairly clear that a commit ID should be used for github sourced projects if they don't have some kind of 'external to github' tarball distribution, though I'm not sure if it's really *meant* to.

Aside from the above, looks good. c#17 and c#18 are not blockers. c#19 may be, at least I'd like you to consider it and give an opinion before approving the review. If you disagree with the guideline, we might want to ask FPC to clarify it.

https://fedorahosted.org/fpc/ticket/233

does seem to suggest they explicitly considered the question of whether using tarballs generated from tags was OK and seem to have decided 'no'. I still might suggest that they make that _absolutely_ clear in the text, though.

I should note - I checked it built (for F20), installed it, and manually unbundled php-opencloud from owncloud: I dropped the manual loading of php-opencloud's autoloader, and wiped owncloud's copy of php-opencloud. Then I checked that I could still add an openstack/rackspace external storage item in OC configuration. I don't have an actual openstack space to test with, but I would've expected it to barf before getting to that point if this was broken.

IOW, it looks like owncloud's class loader as patched by us is capable of autoloading this copy of php-opencloud, as we expected.

Spec URL: http://brummbq.fedorapeople.org/php-opencloud.spec
SRPM URL: http://brummbq.fedorapeople.org/php-opencloud-1.6.0-3.fc19.src.rpm

okay adjusted source url according to guidelines and fixed some wee issues.

(In reply to Adam Williamson from comment #21)
> IOW, it looks like owncloud's class loader as patched by us is capable of
> autoloading this copy of php-opencloud, as we expected.

cool, so we should be fine

Looks good to me with changes from c#22 - all MUST items in the guidelines pass. Review is approved.

New Package SCM Request
=======================
Package Name: php-opencloud
Short Description: PHP SDK for OpenStack/Rackspace APIs
Owners: brummbq
Branches: f20 el6
InitialCC:

actually I think I forgot to add the proper obsoletes for php-cloudfiles. Well, I'll add this before import and retire cloudfiles afterwards.

Git done (by process-git-requests).

imported

Package Change Request
======================
Package Name: php-opencloud
New Branches: epel7
Owners: siwinski
InitialCC:

Git done (by process-git-requests).

Package Change Request
======================
Package Name: php-opencloud
New Branches: epel7
Owners: siwinski
InitialCC:

Git done (by process-git-requests).