Bug 517191 - Review Request: php-symfony-symfony - Open-Source PHP Web Framework
Summary: Review Request: php-symfony-symfony - Open-Source PHP Web Framework
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 517643 546020
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-08-12 21:20 UTC by Christof Damian
Modified: 2011-01-29 17:24 UTC (History)
7 users (show)

Fixed In Version: php-symfony-symfony-1.4.8-2.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-01-29 17:24:54 UTC
tibbs: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Christof Damian 2009-08-12 21:20:16 UTC
Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.2.8-1.fc11.src.rpm
Description: 
Symfony is a complete framework designed to optimize the development of web
applications by way of several key features. For starters, it separates a web
application's business rules, server logic, and presentation views.
It contains numerous tools and classes aimed at shortening the development time
of a complex web application. Additionally, it automates common tasks so that
the developer can focus entirely on the specifics of an application.
The end result of these advantages means there is no need to reinvent the wheel
every time a new web application is built!

Comment 1 Christof Damian 2009-08-12 21:25:18 UTC
two remarks:

rpmlint complains about these errors/warnings: zero-length, hidden-file-or-dir, htaccess-file . As these are files needed for symfony I don't think this should be a problem.

I called the package "php-symfony-symfony", because there is a "php-channel-symfony" and this package is build from the symfony pear package. Another choice would be "php-symfony" or just "symfony" and then use the normal tgz from the symfony website which would also be possible, but this might be more in line with the packaging guidelines.

Comment 2 Andrew Colin Kissa 2009-08-13 11:52:35 UTC
I think you can fix the .htaccess errors by using an apache config file %{_sysconfdir}/httpd/conf.d/symfony for all the configurations provided by the .htaccess. This is recommended because some sites will not allow overriding configurations using .htaccess files.

Comment 3 Christof Damian 2009-08-13 12:38:32 UTC
I don't think they need to be in the Apache configuration, because they are either related to tests:

./test/functional/fixtures/project/web/.htaccess
./lib/plugins/sfDoctrinePlugin/test/functional/fixtures/web/.htaccess
./lib/plugins/sfPropelPlugin/test/functional/fixtures/web/.htaccess
./lib/plugins/sfCompat10Plugin/test/functional/fixtures/web/.htaccess

or are used on project creation as a template.

./lib/task/generator/skeleton/project/web/.htaccess

The symfony library directory itself is usually not accessible from the web anyway.

Comment 4 Andrew Colin Kissa 2009-08-13 12:45:35 UTC
Oh okay thats fine then.

Comment 5 Jason Tibbitts 2009-08-13 17:47:20 UTC
Is this essentially the same thing as bug 351441?  Do the same issues which caused the closure of that review still apply?

Comment 6 Christof Damian 2009-08-13 19:23:51 UTC
(In reply to comment #5)
> Is this essentially the same thing as bug 351441?  Do the same issues which
> caused the closure of that review still apply?  

I didn't see that bug. But I guess they still apply. I had a quick grep through the sources and found these in the vendor directories:

./lib/plugins/sfCompat10Plugin/lib/vendor: phpmailer
./lib/plugins/sfDoctrinePlugin/lib/vendor: doctrine
./lib/plugins/sfPropelPlugin/lib/vendor: phing,  propel,  propel-generator
./lib/vendor: lime

- phpmailer, phing, propel and propel-generator are already available in Fedora. 
- doctrine isn't.
- lime is just used by symfony anyway, as far as I know. It is just one file.

Personally I think it is not a big problem, because symfony requires certain versions of the libraries. For example if I create a doctrine package of the current 1.1 it probably won't work with symfony 1.2. 

I would suggest bringing symfony into Fedora as is, with the plan to remove these duplicated libraries in one of the next releases where possible. I think this was how it was done in other packages which bundled libraries.

Then I probably would:
- remove the sfCompat10Plugin with the ancient phpmailer completely. 
- create the doctrine package, which I planned anyway.
- check if symmfony works with the fedora propel & phing packages. If it doesn't I would remove that plugin too.
- create a patch to make symfony look at the default library locations instead of vendor directories.

Thank god I don't have to think about EPEL, because RHEL is still stuck at PHP 5.1 .

Comment 7 Jason Tibbitts 2009-08-14 20:39:20 UTC
I would certainly not approve a package at is with bundled libraries.  The packaging guidelines say this:
http://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_of_multiple_projects
and https://fedoraproject.org/wiki/No_Bundled_Libraries.

Perhaps some other reviewer feels differently.

Comment 8 Christof Damian 2009-08-15 14:13:27 UTC
(In reply to comment #7)
> I would certainly not approve a package at is with bundled libraries. 
>
> Perhaps some other reviewer feels differently.  

No, you are obviously right. 

I will create packages and review requests for php-channel-doctrine and php-doctrine-doctrine and then see if I can fix this one.

Comment 9 Christof Damian 2009-08-15 14:38:32 UTC
I created these now:

bug 517641 : Review Request: php-channel-doctrine - Adds doctrine project channel to PEAR

bug 517643 : Review Request: php-doctrine-Doctrine - PHP Object Relational Mapper

I hope these are less controversial. If they are approved I will fix the spec file of this request to use the separate libraries for Doctrine and Propel and remove the Symfony 1.0 support with the ancient phpmailer.

Comment 10 Christof Damian 2009-08-26 06:06:02 UTC
I changed the spec file to remove the included libraries (except of lime) and added a patch for one problem with Doctrine 1.1:

Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.2.8-2.fc11.src.rpm

I am still missing a reviewer for bug 517643 : Review Request: php-doctrine-Doctrine , before this can go through though.

Comment 11 Christof Damian 2009-09-02 19:22:15 UTC
Another small change: I added a README.fedora file detailing the changes to the upstream tgz distribution.

Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.2.8-3.fc11.src.rpm

Comment 13 Christof Damian 2009-09-28 20:38:21 UTC
update to upstream 1.2.9, no other changes

Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.2.9-1.fc11.src.rpm

Comment 14 Alexander Kahl 2009-12-09 21:09:05 UTC
Hi Christof,

I am the author of bug 351441 you've already discovered in the course of this request and want to warn you since I had to deal with Symfony upstream two times before leaving behind really negative impressions about cooperativeness and open-mindedness. Don't expect any support from the maintainer(s).

If you want Propel support: I'm its package maintainer (surprise, surprise) but haven't updated it for ages - please drop me a line if you need an update, alternatively the package (actually they're two) could use a co-maintainer ;)

Regards

Comment 15 Christof Damian 2009-12-09 21:38:11 UTC
(In reply to comment #14)
> I am the author of bug 351441 you've already discovered in the course of this
> request and want to warn you since I had to deal with Symfony upstream two
> times before leaving behind really negative impressions about cooperativeness
> and open-mindedness. Don't expect any support from the maintainer(s).

I noticed that Symfony is certainly not the nicest software to package. 

> If you want Propel support: I'm its package maintainer (surprise, surprise) but
> haven't updated it for ages - please drop me a line if you need an update,
> alternatively the package (actually they're two) could use a co-maintainer ;)

I thought I skip the propel support in the first release, because it would need an update to work with symfony 1.4 and Doctrine is the default now anyway.

The good thing is that this makes me package other useful PHP software, even if Symfony never makes it :-) Doctrine is worth the effort at least.

Comment 16 Alexander Kahl 2009-12-10 09:14:58 UTC
(In reply to comment #15)
> I noticed that Symfony is certainly not the nicest software to package. 
Definitely.

> I thought I skip the propel support in the first release, because it would need
> an update to work with symfony 1.4 and Doctrine is the default now anyway.
To me it seems the same in general, no one asks for Propel anymore because everyone migrated over to Doctrine. 

> The good thing is that this makes me package other useful PHP software, even if
> Symfony never makes it :-) Doctrine is worth the effort at least.  
Absolutely!

Comment 17 Christof Damian 2009-12-13 21:04:48 UTC
update to upstream 1.4.1

Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.4.1-1.fc12.src.rpm  

I update the spec file to the symfony 1.4 release, which makes some things easier because some of the compatibility libraries are removed and a newer Doctrine version is used. But now the Swift Mailer is required, which I also submitted for review.

I currently just target the use of doctrine, as propel needs an update too. The bundled propel is removed though. 

rpmlint just complains about zero length and htaccess files for skeleton and template files.

This is probably a package for Fedora 13 and not older versions, because of the dependencies.

Comment 18 Christof Damian 2010-05-14 21:14:15 UTC
update to upstream 1.4.4

Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.4.4-1.fc12.src.rpm 

I also use sed to fix up the paths instead of a patch, that means I can use %{pear_phpdir} to point to the correct pear directory. It also makes maintaining the package a bit easier.

Swift Mailer is now available as php-swift-Swift, so that is used.

I guess this means Fedora 14 then :-)

Comment 19 Jason Tibbitts 2010-11-23 02:03:37 UTC
I was going to take a look at this, but then I realized that the last comment was six months ago and the current version upstream seems to be 1.4.8.  Not sure if the posted package should be reviewed or not, but I can make some random comments:

Is it normal for .pkgxml/symfony.xml to be over half a meg?  I guess there are some two thousand files in the package, so perhaps that's not surprising.

As far as I can tell, lime really isn't a bundled library.  It seems to have been written as part of symfony though it can be used standalone as it has no external dependencies.  I guess you could package it separately, but that's not much different from many other libraries that are part of a larger package.

I do kind of wish we had just allowed "php-channelname" instead of "php-channelname-packagename" when the channel name and package name were the same.  Unfortunately repeating it looks kind of dumb.

I wonder if it's worth including the Propel stuff when Propel itself is no longer in the distribution.  (It seems to have been orphaned and while it is in f13 it's not in f14 or rawhide.)  Or perhaps I'm misunderstanding what the sfPropelPlugin stuff is for, since you mention that you're only targeting doctrine.

Comment 20 Christof Damian 2010-11-23 07:11:08 UTC
(In reply to comment #19)
> I was going to take a look at this, but then I realized that the last comment
> was six months ago and the current version upstream seems to be 1.4.8.  Not
> sure if the posted package should be reviewed or not, but I can make some
> random comments:

I haven't updated it because nobody seems to be interested in reviewing it and everybody who does gives up after a while. The symfony project itself also doesn't seem to be interested in a package. I will update it this week if possible.

> Is it normal for .pkgxml/symfony.xml to be over half a meg?  I guess there are
> some two thousand files in the package, so perhaps that's not surprising.

Yes, I think it is normal.

> As far as I can tell, lime really isn't a bundled library.  It seems to have
> been written as part of symfony though it can be used standalone as it has no
> external dependencies.  I guess you could package it separately, but that's not
> much different from many other libraries that are part of a larger package.

OK, that is good. It is shipped in the "vendor" directory, which is usually reserved for external stuff.
 
> I do kind of wish we had just allowed "php-channelname" instead of
> "php-channelname-packagename" when the channel name and package name were the
> same.  Unfortunately repeating it looks kind of dumb.

yes, but this is quite common. Maybe the package could at least provide php-symfony? But this is a package guideline problem and not really a can of worms I want to open here.

> I wonder if it's worth including the Propel stuff when Propel itself is no
> longer in the distribution.  (It seems to have been orphaned and while it is in
> f13 it's not in f14 or rawhide.)  Or perhaps I'm misunderstanding what the
> sfPropelPlugin stuff is for, since you mention that you're only targeting
> doctrine.

the sfPropelPlugin provides Propel support for symfony and also includes a copy of the Propel library. I removed the whole thing, because I don't want to package Propel and new symfony project are usually using Doctrine anyway.

Comment 21 Jason Tibbitts 2010-11-23 14:38:46 UTC
(In reply to comment #20)

> I haven't updated it because nobody seems to be interested in reviewing it and
> everybody who does gives up after a while.

Well, the other side is that if people see that it's out of date perhaps they don't want to spend time reviewing it.  So it works both ways.

> The symfony project itself also doesn't seem to be interested in a package.

If they don't want their software packaged, perhaps we shouldn't be packaging it?

> yes, but this is quite common.

I know; as a member of the packaging committee, I'm just lamenting the fact that the guideline we wrote makes for dumb-looking package names.

I can't promise a review in the future but I'll watch this ticket.

Comment 22 Christof Damian 2010-11-23 19:38:05 UTC
(In reply to comment #21)
> (In reply to comment #20)
> 
> > I haven't updated it because nobody seems to be interested in reviewing it and
> > everybody who does gives up after a while.
> 
> Well, the other side is that if people see that it's out of date perhaps they
> don't want to spend time reviewing it.  So it works both ways.

I agree, I have updated it to the current release:

Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.4.8-1.fc13.src.rpm

> > The symfony project itself also doesn't seem to be interested in a package.
> 
> If they don't want their software packaged, perhaps we shouldn't be packaging
> it?

I don't think they are an exception. A lot of project are of the opinion that the best way to distribute software is a build directly from them, ideally with all approved versions of libraries bundled. 

> > yes, but this is quite common.
> 
> I know; as a member of the packaging committee, I'm just lamenting the fact
> that the guideline we wrote makes for dumb-looking package names.
> 
> I can't promise a review in the future but I'll watch this ticket.

Thanks, me too. As I mentioned above, this review request has brought some other packages into Fedora so it is a win anyway. I understand that it is a difficult beast and maybe not with a big audience.

Comment 23 Ruediger Landmann 2010-11-24 00:20:59 UTC
Hi Christof, I tried to build this a few days ago, but could not build this locally: 

$ rpmbuild -ba SPECS/php-symfony-symfony.spec
error: Failed build dependencies:
        php-channel(pear.symfony-project.com) is needed by php-symfony-symfony-1.4.4-1.fc14.noarch

When I installed php-channel-symfony, the package built fine. Is this expected?

I'll now have a go at the new version of the package and hopefully contribute a review unless wiser and more experienced heads beat me to it. :)

Comment 24 Jason Tibbitts 2010-11-24 02:48:41 UTC
It is expected that you'll have to manually install dependencies of packages when you build with rpmbuild.  mock will of course install all of the specified build dependencies into the chroot as part of the build process.

Comment 25 Ruediger Landmann 2010-11-24 02:56:06 UTC
(In reply to comment #24)
> It is expected that you'll have to manually install dependencies of packages
> when you build with rpmbuild.  mock will of course install all of the specified
> build dependencies into the chroot as part of the build process.

D'oh! :) Of course; sorry, I was misled by the package name.

Comment 26 Jason Tibbitts 2011-01-08 19:44:21 UTC
I was hoping someone else would review this.  Honestly I think it's pretty clean now, but before I dredge up the time to review it, I wonder how this package works with upstream's recommendation that every project should have a dedicated symfony installation.  Does it just incorporate the same thing you'd get if you did "pear install", which upstream specifically says is not recommended?  If so, what problems does this cause?

Comment 27 Christof Damian 2011-01-08 20:00:18 UTC
(In reply to comment #26)
> I wonder how this
> package works with upstream's recommendation that every project should have a
> dedicated symfony installation.  Does it just incorporate the same thing you'd
> get if you did "pear install", which upstream specifically says is not
> recommended?  If so, what problems does this cause?

The package behaves just like pear install would, with the exception of the removed libraries. 
The problems and advantages are the same as with other libraries. You will have one single install of symfony for all your websites and you will need to check that these still work after a symfony upgrade (or a Doctrine/YAML/... upgrade). 

I would expect that this is mostly useful for either development or for people who would like to deploy symfony onto multiple machines by RPM. Advanced users probably include symfony with all plugins and pear packages in their revision control repository. 

It is easy to switch a project to a bundled symfony, even if you set it up with the RPM.

Comment 28 Jason Tibbitts 2011-01-13 21:27:50 UTC
OK, let me try to get this taken care of, then.  (It's the second oldest untaken review ticket, and I'm tired of seeing it on the list.)

Building is really noisy and installing in a fresh system is pretty noisy as well, though I expect it's due to some PHP issue or something weird about my local mock setup:

PHP Warning:  date(): It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'America/Chicago' for 'CST/-6.0/no DST' instead in /usr/share/pear/PEAR/Registry.php on line 1012

with a stack trace any time PHP gets called at all.  Since nobody else has complained of this I suspect I'm the only one seeing it.

The source in the srpm differs from the tarball I get from the upstream web site.

I note that this spec manages to break vim's highlighting so everyhing after the first sed call in %install turns up pinkish-purple.  I guess it's the escaped single quotes in those sed lines.

So, outside of the oddity with the tarball, I think this is OK.

X source files do not match upstream.  sha256sum:
  40afb5979abf1e06ce559e11a587ab61c8ad079a61fb32cfb65a37025cf586a4
   symfony-1.4.8.tgz (downloaded)
  58f71c2b8f7e72573a25d67f2b9af7af5536f873a98e8e74d9b6a8e9dddbe458
   symfony-1.4.8.tgz (in package)

* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
   php-pear(pear.symfony-project.com/symfony) = 1.4.8
   php-symfony-symfony = 1.4.8-1.fc15
  =
   /bin/sh  
   /usr/bin/env  
   /usr/bin/pear  
   php >= 5.2.4
   php-channel(pear.symfony-project.com)  
   php-doctrine-Doctrine >= 1.2.0
   php-dom  
   php-pear(PEAR)  
   php-pear(pear.swiftmailer.org/Swift) >= 4.0.5
   php-pear-phing >= 1.0.0
   php-simplexml  

* no bundled libraries (as far as I can tell).
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* scriptlets are OK (pear module registration).
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Comment 29 Christof Damian 2011-01-13 23:45:17 UTC
(In reply to comment #28)
> PHP Warning:  date(): It is not safe to rely on the system's timezone settings.
> You are *required* to use the date.timezone setting or the
> date_default_timezone_set() function. In case you used any of those methods and
> you are still getting this warning, you most likely misspelled the timezone
> identifier. We selected 'America/Chicago' for 'CST/-6.0/no DST' instead in
> /usr/share/pear/PEAR/Registry.php on line 1012
> 
> with a stack trace any time PHP gets called at all.  Since nobody else has
> complained of this I suspect I'm the only one seeing it.

No, this is fixable. We do it for other PHP packages too. Just people who develop in PHP have usually set this in the global config already. I obviously didn't check the mock logs.

> 
> The source in the srpm differs from the tarball I get from the upstream web
> site.

There might be a difference in the tarball from the website and the one from the pear channel, I will check.

> I note that this spec manages to break vim's highlighting so everyhing after
> the first sed call in %install turns up pinkish-purple.  I guess it's the
> escaped single quotes in those sed lines.

I will also check if this is fixable.

Comment 30 Christof Damian 2011-01-14 16:14:00 UTC
Spec URL: http://rpms.damian.net/SPECS/php-symfony-symfony.spec
SRPM URL: http://rpms.damian.net/SRPMS/php-symfony-symfony-1.4.8-2.fc13.src.rpm

- time zone bit is fixed
- so is the vi syntax colouring problem
- new tar file and new URL

The symfony project has many different tar balls, all with the same name. The one I am using now is the same you get with "pear download symfony/symfony" and I changed the URL in the spec file to point to this one too.

Comment 31 Jason Tibbitts 2011-01-20 01:34:47 UTC
Note that the SRPM URL is incorrect; seems it was built on f14 so the s/13/14/.

Everything looks good to me; building is much quieter due to the timezone fix though I still get some of those when installing (though I'm not sure that's fixable, and at least some of it is coming from the dependencies).

The source also matches what's downloaded:
  58f71c2b8f7e72573a25d67f2b9af7af5536f873a98e8e74d9b6a8e9dddbe458
  symfony-1.4.8.tgz
though I can't help but comment on the complete insanity of having multiple tarballs named the same.  How do you know if the one you downloaded was trojaned somehow?  Would they even notice?

Anyway, upstream stupidity aside, this package looks good.

APPROVED

Comment 32 Christof Damian 2011-01-20 07:05:26 UTC
(In reply to comment #31)
> though I can't help but comment on the complete insanity of having multiple
> tarballs named the same.  How do you know if the one you downloaded was
> trojaned somehow?  Would they even notice?

I know, I will try to tell the symfony guys.

> Anyway, upstream stupidity aside, this package looks good.
> 
> APPROVED

Thanks

New Package SCM Request
=======================
Package Name: php-symfony-symfony
Short Description: Open-Source PHP Web Framework
Owners: cdamian
Branches: f14 el6
InitialCC:

Comment 33 Jason Tibbitts 2011-01-20 14:39:04 UTC
Git done (by process-git-requests).

Comment 34 Fedora Update System 2011-01-20 19:22:52 UTC
php-symfony-symfony-1.4.8-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/php-symfony-symfony-1.4.8-2.fc14

Comment 35 Fedora Update System 2011-01-21 22:59:39 UTC
php-symfony-symfony-1.4.8-2.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update php-symfony-symfony'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/php-symfony-symfony-1.4.8-2.fc14

Comment 36 Fedora Update System 2011-01-29 17:24:45 UTC
php-symfony-symfony-1.4.8-2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.


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