Bug 823043 - Review Request: php-symfony2-Console - Symfony2 Console Component
Summary: Review Request: php-symfony2-Console - Symfony2 Console Component
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Christof Damian
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: php-channel-symfony2
Blocks: 823073 837668 837669
TreeView+ depends on / blocked
 
Reported: 2012-05-18 21:20 UTC by Shawn Iwinski
Modified: 2012-07-04 16:25 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-06-26 21:32:52 UTC
Type: ---
Embargoed:
christof: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Shawn Iwinski 2012-05-18 21:20:39 UTC
Spec URL:
http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Console.spec

SRPM URL:
http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Console-2.0.14-1.fc16.src.rpm

Description:
The Console component eases the creation of beautiful and testable command line
interfaces.

The Console component allows you to create command-line commands. Your console
commands can be used for any recurring task, such as cronjobs, imports, or
other batch jobs.

Comment 1 Remi Collet 2012-05-19 06:31:57 UTC
Just a quick notes

- LICENSE and README file are installed in /usr/share/pear.

Probably a good idea to ask upstream, this files should be tagged as "doc" as so installed in /usr/share/doc/pear (and avoid to be duplicated in the package)

- dependencies
# phpci print --recursive --report extension Symfony
-------------------------------------------------------------------------------
PHP COMPAT INFO EXTENSION SUMMARY
-------------------------------------------------------------------------------
  EXTENSION                                        PECL   VERSION         COUNT
-------------------------------------------------------------------------------
  Core                                                    4.0.0             214
  mbstring                                                4.0.6               4
  pcre                                                    4.0.0              14
  posix                                           306939  4.0.0               1
  readline                                         2.0.1  4.0.0               6
  standard                                                4.0.0             367
-------------------------------------------------------------------------------

Reading the code, mbstring and posix are optional (detected at runtime), but I think it could be usefull to add them in the package requires.

- Requires: php >= 5.3.2

Please, don't do that.
Requiring php will pull apache.
A library (especially a "console" one) don't need a webserver.

To set the minimal php version, please use
Requires: php-common >= 5.3.2

Comment 2 Shawn Iwinski 2012-05-19 15:46:52 UTC
I would prefer to add all of the extensions phpci found instead of determining which ones are included in core or other packages so would the following be acceptable?:

Requires: php-mbstring --> separate package
Requires: php-pcre --> included in php-common package
Requires: php-posix --> included in separate php-process package
Requires: php-readline --> included in separate php-cli package



Since this is a console library, would it make sense to require "php-cli" instead of "php-common" since "php-cli" provides /usr/bin/php for "#!/usr/bin/php" or "#!/usr/bin/env php" script usage?

Comment 3 Christof Damian 2012-05-19 19:15:39 UTC
(In reply to comment #2)
> I would prefer to add all of the extensions phpci found instead of determining
> which ones are included in core or other packages so would the following be
> acceptable?:
> 
> Requires: php-mbstring --> separate package
> Requires: php-pcre --> included in php-common package
> Requires: php-posix --> included in separate php-process package
> Requires: php-readline --> included in separate php-cli package

ok with me, I would prefer this too, You never know when an extension might be moved out of core in the future.

> Since this is a console library, would it make sense to require "php-cli"
> instead of "php-common" since "php-cli" provides /usr/bin/php for
> "#!/usr/bin/php" or "#!/usr/bin/env php" script usage?

No, php-common is still better. This might for example be running in a unit testing web environment, which doesn't have php-cli.

Comment 4 Shawn Iwinski 2012-05-19 23:11:40 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > I would prefer to add all of the extensions phpci found instead of determining
> > which ones are included in core or other packages so would the following be
> > acceptable?:
> > 
> > Requires: php-mbstring --> separate package
> > Requires: php-pcre --> included in php-common package
> > Requires: php-posix --> included in separate php-process package
> > Requires: php-readline --> included in separate php-cli package
> 
> ok with me, I would prefer this too, You never know when an extension might
> be moved out of core in the future.

Glad we think alike


> > Since this is a console library, would it make sense to require "php-cli"
> > instead of "php-common" since "php-cli" provides /usr/bin/php for
> > "#!/usr/bin/php" or "#!/usr/bin/env php" script usage?
> 
> No, php-common is still better. This might for example be running in a unit
> testing web environment, which doesn't have php-cli.

Note that phpci found the readline extension dependency which would be provided by php-readline which is a virtual package of php-cli [1].  I'll keep the php-common requirement, but I just wanted to note that the php-cli package would be installed anyway.

[1] http://pkgs.fedoraproject.org/gitweb/?p=php.git;a=blob;f=php.spec;h=a224e69538d4b848bbebec39ce8c352c8be80d78;hb=HEAD#l143

Comment 5 Shawn Iwinski 2012-05-19 23:29:15 UTC
(In reply to comment #1)
> - LICENSE and README file are installed in /usr/share/pear.
> 
> Probably a good idea to ask upstream, this files should be tagged as "doc"
> as so installed in /usr/share/doc/pear (and avoid to be duplicated in the
> package)

I will request the change upstream.  If they refuse or take a very long to make the change, is this a blocker?  Is it acceptable to use a sed command in the spec file to modify the package.xml file to change the roles of those files to doc?

Comment 6 Remi Collet 2012-05-20 06:36:13 UTC
About Requires:

Yes, using the wirtual provides is better, (it's the reallity), and PHP sub-package spliting could change in the future (and have changed in the past)

The only problem is that  most virtual are not versionned (could be consider as a bug in PHP, but not so simple), so you can requires php-mbstring >= 5.3.2 but you can only requires php-posix (without version). Be carefull with php-xml (xml extension is provided by php-common, this package provides other extensions: dom, wddx, xmlreader, xmlwriter and xsl)


About License:

Yes, "sed" is ok. We don't have to wait for a new upstream release.
It would be very suprising that they refuse. (except for a very good reason, which must be documented, and then, apply to the spec)

Good pratice about such patch/change is to add a comment in the spec with a link to upstream answer (ML archive, commit, ...)

About script:

If a script use the php shebang, I think it will be autodetect by rpmbuild and added to the package dependencies (so using "env" is a bad solution) => to be checked (I can't because AFK)

Comment 7 Christof Damian 2012-05-20 11:32:22 UTC
I wonder how to handle the directories. All of the components contain something like:

%dir %{pear_phpdir}/Symfony/Component
%dir %{pear_phpdir}/Symfony

Ideally only one of them would own these directories, but as they are all independent of each other this doesn't really make sense.

Comment 8 Remi Collet 2012-05-20 15:18:20 UTC
(In reply to comment #7)
> I wonder how to handle the directories. All of the components contain
> something like:
> 
> %dir %{pear_phpdir}/Symfony/Component
> %dir %{pear_phpdir}/Symfony
> 
> Ideally only one of them would own these directories, but as they are all
> independent of each other this doesn't really make sense.

The Guidelines are "quite" clear 

MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that director
MUST: Packages must not own files or directories already owned by other packages. 

So, as this package have no dependencies on other php-symfony2-* it MUST own this folders.

See my comment on #823071. php-symfony2-Form must not own this dir.

Probably more than one package will own this dir. This is not a problem.

Comment 9 Christof Damian 2012-05-20 15:21:21 UTC
(In reply to comment #8)
> MUST: A package must own all directories that it creates. If it does not
> create a directory that it uses, then it should require a package which does
> create that director
> MUST: Packages must not own files or directories already owned by other
> packages. 
 
> Probably more than one package will own this dir. This is not a problem.

If more than one package owns a directory, then the second MUST doesn't make sense. That was confusing me.

Comment 10 Shawn Iwinski 2012-05-20 17:38:04 UTC
Updates per comments

- Removed BuildRoot
- Changed php require to php-common
- Added the following requires based on phpci results:
  php-mbstring, php-pcre, php-posix, php-readline
- %description update
- Removed %defattr from %files section

SPEC URL:
http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Console.spec

SRPM URL:
http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Console-2.0.14-2.fc16.src.rpm

Comment 11 Christof Damian 2012-05-20 17:53:16 UTC
(In reply to comment #10)
> SPEC URL:
> http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Console.spec
> 
> SRPM URL:
> http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Console-2.0.14-
> 2.fc16.src.rpm

[!]: MUST Package does not contain duplicates in %files.
     Note: warning: File listed twice:
     /usr/share/pear/Symfony/Component/Config/LICENSE

I think the LICENSE, README.md and composer.json should be moved to the phpdoc directory.

Everything else is OK.

Comment 12 Shawn Iwinski 2012-05-20 18:40:36 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > SPEC URL:
> > http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Console.spec
> > 
> > SRPM URL:
> > http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Console-2.0.14-
> > 2.fc16.src.rpm
> 
> [!]: MUST Package does not contain duplicates in %files.
>      Note: warning: File listed twice:
>      /usr/share/pear/Symfony/Component/Config/LICENSE
> 
> I think the LICENSE, README.md and composer.json should be moved to the
> phpdoc directory.
> 
> Everything else is OK.

I need to request upstream to make this change in their package.xml file.  In the meantime, would you like me to do one of the following?:
1) Use sed (or cat and awk) to update the package.xml file in the %prep section (should not have to be recreated for future updates)
2) Create a patch for the package.xml file (may have to be recreated for future updates)
3) "Manually" move the files to the "correct" location in the %install section (should not have to be recreated for future updates)

(Same question as https://bugzilla.redhat.com/show_bug.cgi?id=823041#c4)

Comment 14 Christof Damian 2012-05-20 21:03:36 UTC
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated

==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[-]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[-]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[-]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

==== PHP ====
[x]: MUST Package requires php-common instead of php.

ACCEPT

Comment 15 Shawn Iwinski 2012-05-31 17:07:09 UTC
Updated to upstream version 2.0.15 & updates per bug #817303

- Removed "BuildRequires: php-pear >= 1:1.4.9-1.2"
- Updated %prep section
- Removed cleaning buildroot from %install section
- Removed documentation move from %install section (fixed upstream)
- Removed %clean section
- Updated %doc in %files section

SPEC URL: http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Console.spec

SRPM URL: http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Console-2.0.15-1.fc16.src.rpm

Comment 16 Shawn Iwinski 2012-06-07 21:38:39 UTC
New Package SCM Request
=======================
Package Name: php-symfony2-Console
Short Description: Symfony2 Console Component
Owners: siwinski
Branches: f16 f17 el6
InitialCC:

Comment 17 Gwyn Ciesla 2012-06-08 12:40:13 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2012-06-11 00:51:08 UTC
php-symfony2-Console-2.0.15-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/php-symfony2-Console-2.0.15-1.fc17

Comment 19 Fedora Update System 2012-06-11 00:51:19 UTC
php-symfony2-Console-2.0.15-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/php-symfony2-Console-2.0.15-1.fc16

Comment 20 Fedora Update System 2012-06-11 00:51:29 UTC
php-symfony2-Console-2.0.15-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/php-symfony2-Console-2.0.15-1.el6

Comment 21 Fedora Update System 2012-06-12 00:32:47 UTC
php-symfony2-Console-2.0.15-1.fc16 has been pushed to the Fedora 16 testing repository.

Comment 22 Fedora Update System 2012-06-26 21:32:52 UTC
php-symfony2-Console-2.0.15-1.fc16 has been pushed to the Fedora 16 stable repository.

Comment 23 Fedora Update System 2012-06-26 21:37:52 UTC
php-symfony2-Console-2.0.15-1.fc17 has been pushed to the Fedora 17 stable repository.

Comment 24 Fedora Update System 2012-07-01 01:32:31 UTC
php-symfony2-Console-2.0.15-1.el6 has been pushed to the Fedora EPEL 6 stable repository.


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