[pkg-php-pear] Symfony package review (was: Let's reconsider the way Symfony2 Components are packaged for Debian)

Sun Sep 21 22:09:21 UTC 2014

Hi Daniel,

Le 21/09/2014 14:14, Daniel Beyer a écrit :

> an other un-replied mail from the depth of this thread.

Thanks for your reply, some more details bellow (Don’t get tricked by
the actual object of this message, I’ve not yet started to review your
latests changes to the packaging).

> On Mon, 2014-09-08 at 10:04 -0400, David Prévot wrote:
>> Le 08/09/2014 02:35, Daniel Beyer a écrit :
>>> On Sun, 2014-09-07 at 16:27 -0400, David Prévot wrote:
>>>> Le 07/09/2014 15:28, Daniel Beyer a écrit :
>>
>>>> - you may wish to regroup paragraphs about the same license.

>> I actually meant “factorize”. E.g.:

> Licensing should be clear now and has been discussed in an other thread:
> http://lists.alioth.debian.org/pipermail/pkg-php-pear/2014-September/003520.html

The licensing is very clear IMHO, thanks for your work on it and the
detailed comments. I believe it’s OK to keep this level of details as
you did, especially before an actual review by our ftpmasters. I just
want to point out that it can be shorten:

E.g. the following parts of d/copyright:

--------------------%<--------------------------

Files: *
Copyright: 2004-2014 Fabien Potencier
License: Expat

Files: src/Symfony/Component/HttpKernel/HttpCache/*.php
Copyright: 2004-2014 Fabien Potencier
           2008 Ryan Tomayko
License: Expat
Comment: Partially based on the Rack-Cache library.
 The Rack-Cache library is copyright Ryan Tomayko and released under the
 MIT (Expat) license.

Files: src/Symfony/Component/Translation/Loader/MoFileLoader.php
Copyright: 2004-2014 Fabien Potencier
           2010 Union of RAD
License: Expat

Files: src/Symfony/Component/Translation/Loader/PoFileLoader.php
Copyright: 2004-2014 Fabien Potencier
           2012 Clemens Tolboom
           2010 Union of RAD
License: Expat

-------------------->%--------------------------

could simply have been:

--------------------%<--------------------------

Files: *
Copyright: 2004-2014 Fabien Potencier
           2008 Ryan Tomayko
           2010 Union of RAD
           2012 Clemens Tolboom
License: Expat

-------------------->%--------------------------

as allowed by the current format specification. We don’t need to comment
about the reason of the copyright/license anyway. I used to keep this
initial level of detail myself, but the copyright file can be huge in
some cases (yes, owncloud, I’m looking at you), and thus took advantage
of the shorter form to make it easier to read (at the cost of being a
bit more tricky to update when files are deleted, or added…).

In short: I believe the form you chose is fine, and there is no need to
remove all the explanations currently present, but you may consider
using a shorter form in the future, if it suits you better.

>> - are you sure the “Test” (without final “s”) directories should be
>>   stripped away?
> 
> Yes, I double checked and they only contain helper code related to tests
> and are not needed during run time.

Haven’t yet looked into how you managed to make the Test(s) directory
available in the PHP path while testing the actually installed content
of the packages for the DEP-8 tests, I hope you found something nicer
that the hacks I pushed into the standalone php-symfony-console or
php-symfony-process for example, but that will be for another day (I’m
fed up with looking into the Symfony code for today ;-).

> It inspired me to reduce complexity of d/rules even more - see commit
> 6faab753dc0dba49d3a110c275b949a2b5118b30 for details:

I overlooked it earlier today, and believe it’s a good move, thanks for
that! (I didn’t dare to suggest it earlier because it meant to almost
restart fresh.)

>> - some README.md are useless and thus shouldn’t be shipped in the
>>   package (e.g., src/Symfony/Component/PropertyAccess/README.md); maybe
>>   the file size reflects its usefulness;
> 
> You're right. Since I did not want to blow up d/rules again, I simply
> patched those out of upstreams code for now.

Glad to see your comment (especially the “for now” part): as for the
copyright part, I think your approach is overly careful (e.g., I
personally wouldn’t have bothered to strip away the PHPUnit
documentation out of the upstream README, but agree that’s a clever
thing to do). The patch thing may be a pain to maintain in the long
term, but that can be dealt later (no need to implement an easier
solution right now, since an extensive and accurate job has been done).
Disclaimer: I’ve not yet looked into the specifics of your changes, just
commenting after my initial overlook.

>> - some package provides executables, maybe they should be moved in the
>>   PATH if they are useful (with extra care to keep them working), or
>>   not shipped at all if they aren’t (see Component/Intl/Resources/bin/);
> 
> I took a look at them and came to the result that they all are not
> useful during runtime at all. Some of them even would not work due to
> missing vendor/autoload.php normally created by composer. Since they are
> not needed during build time, I patched them out of upstream's source,
> too. 

Maybe will it be simpler to handle the removal with the -X switch of
dh_install (and simpler to maintain too). Please note that the autoload
stuff can be worked-around using class-loader (dogfooding FTW), or even
phpab, but removal is fine if they are not useful in the binary package.

>> - I kinda liked the idea of a meta symfony package, what changed your
>>   mind about it? (Thinking about doctrine and zendframework, I think it
>>   will be nice to keep those (empty) packages once split too, but maybe
>>   am I seeing it wrong);
> 
> I think php-symfony-framework-bundle does serve this purpose already.
> But I'm open to re-add such a meta package again. But we maybe at least
> do not depend on php-symfony-locale in it, since it is deprecated
> starting with 2.3.
> I'm not adding such a meta package back again for now, but if you still
> think it should, fell free to open up a new thread.

To be continued (will have to make up my mind about it), but a binary
package with a name as simple as symfony may be easier to find than
php-symfony-framework-bundle if people are willing to install it.

> I think it now is not too frightening anymore. :-)

I’ll try and review it more precisely within a few days (prioritizing it
against the php-symfony-* standalone packages that won’t hopefully be
needed to update after all). Another thing on the top of my head: in
5b6fa15f (Update versioned Replaces: and Breaks: fields in d/control)
the version (“Breaks: php-symfony-classloader (<<2.3.19~)”) won’t work
if we actually upload a 2.3.19-1~sid1 version, maybe you wish to use
“2.3.19-1~t” instead, since dpkg will understand the following if I’m
not mistaken.
	2.3.19~ < 2.3.19-1~sid1 < 2.3.19-1~t < 2.3.19-1

On the other hand, I’m not sure a version is mandatory here (but I may
be too tired to think it through).

Regards

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-php-pear/attachments/20140921/3d16ff18/attachment.sig>