Bug 957573 - Review Request: php-aws-sdk - amazon web services sdk for php
Summary: Review Request: php-aws-sdk - amazon web services sdk for php
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Gregor Tätzner
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-04-29 04:03 UTC by Joseph Marrero
Modified: 2013-10-06 01:32 UTC (History)
4 users (show)

Fixed In Version: php-aws-sdk-2.4.5-2.fc19
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-09-22 23:55:57 UTC
Type: ---
gregor: fedora-review+


Attachments (Terms of Use)
pre-review (5.70 KB, text/plain)
2013-05-01 07:46 UTC, Gregor Tätzner
no flags Details
diff 1.6.2-3 1.6.2.4 (2.92 KB, text/plain)
2013-05-02 20:30 UTC, Gregor Tätzner
no flags Details

Description Joseph Marrero 2013-04-29 04:03:46 UTC
Spec URL: http://jmarrero.fedorapeople.org/packages/php-aws-sdk/php-aws-sdk.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/php-aws-sdk/php-aws-sdk-1.6.2-1.fc19.src.rpm
Description: Amazon webservices framework for php
Fedora Account System Username: jmarrero

Comment 1 Joseph Marrero 2013-04-29 04:10:25 UTC
successful koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5311290

Comment 2 Gregor Tätzner 2013-04-29 17:13:59 UTC
PRE-REVIEW

- please rename channel.xml to %{name}.xml and adjust the scriptlets accordingly

"License:        Apache-v2"
- you just made that up, did you ;)? Use 'ASL 2.0'

- all folders located in %{pear_phpdir}/AWSSDKforPHP/ beginning with underscore should be moved to %{pear_docdir} I don't know why just a couple of html files are marked as doc in the package.xml.

- also this should be fixed:
php-aws-sdk.noarch: E: non-executable-script /usr/share/pear/AWSSDKforPHP/_compatibility_test/sdk_compatibility_test_cli.php 0644L /usr/bin/env
php-aws-sdk.noarch: E: non-executable-script /usr/share/pear/AWSSDKforPHP/_samples/cli-s3_progress_bar.php 0644L /usr/bin/env

- watch your %doc macro

- "Source0:        Source0: http://pear.amazonwebservices.com/get/sdk-%{version}.tgz" leads to unexpected results ;)

Comment 3 Gregor Tätzner 2013-04-29 17:17:46 UTC
oh, and the description should be more verbose

Comment 4 Joseph Marrero 2013-04-30 01:57:49 UTC
the package.xml is renamed to {pear_name}.xml
%{name}.xml is used when is a pear channel package. Even the template provided with rpmdev-newspec -t php-pear php-pear-foo   uses pear_name macro

-licence FIXED (sorry, I should have fixed that before submiting) 

- Documentation moved to the correct folder ({pear_docdir}/%{pear_name})

-%doc macro FIXED(removed doubled $doc)

Source0 Leads to unexpected results?
http://pear.amazonwebservices.com/get/sdk-1.6.2.tgz

-Description FIXED

How do you get??? (rpmlint and install don't complain) "  php-aws-sdk.noarch: E: non-executable-script /usr/share/pear/AWSSDKforPHP/_compatibility_test/sdk_compatibility_test_cli.php 0644L /usr/bin/env
php-aws-sdk.noarch: E: non-executable-script /usr/share/pear/AWSSDKforPHP/_samples/cli-s3_progress_bar.php 0644L /usr/bin/env   "

Spec URL: http://jmarrero.fedorapeople.org/packages/php-aws-sdk/php-aws-sdk.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/php-aws-sdk/php-aws-sdk-1.6.2-2.fc19.src.rpm

old packages and specs at: http://jmarrero.fedorapeople.org/packages/php-aws-sdk/old/

Comment 5 Gregor Tätzner 2013-04-30 06:33:33 UTC
(In reply to comment #4)
> the package.xml is renamed to {pear_name}.xml
> %{name}.xml is used when is a pear channel package. Even the template
> provided with rpmdev-newspec -t php-pear php-pear-foo   uses pear_name macro

the template from rpmdev is just a recommendation. Our guidelines are mandatory. At the end of the day 'sdk.xml' is just too short.

http://fedoraproject.org/wiki/Packaging:PHP#PEAR_Modules

> Source0 Leads to unexpected results?

may I draw your attention on the double source tag: 'Source0:        Source0: ...' :)

> -Description FIXED

thanks

> How do you get??? (rpmlint and install don't complain) " 
> php-aws-sdk.noarch: E: non-executable-script
> /usr/share/pear/AWSSDKforPHP/_compatibility_test/sdk_compatibility_test_cli.
> php 0644L /usr/bin/env
> php-aws-sdk.noarch: E: non-executable-script
> /usr/share/pear/AWSSDKforPHP/_samples/cli-s3_progress_bar.php 0644L
> /usr/bin/env   "

just a regular rpmlint run on f18. those script should be executable

Comment 6 Gregor Tätzner 2013-04-30 06:38:52 UTC
(In reply to comment #4)
> How do you get??? (rpmlint and install don't complain) " 
> php-aws-sdk.noarch: E: non-executable-script
> /usr/share/pear/AWSSDKforPHP/_compatibility_test/sdk_compatibility_test_cli.
> php 0644L /usr/bin/env
> php-aws-sdk.noarch: E: non-executable-script
> /usr/share/pear/AWSSDKforPHP/_samples/cli-s3_progress_bar.php 0644L
> /usr/bin/env   "

though on a second build I couldn't reproduce this issues. probably we're fine.

Comment 7 Gregor Tätzner 2013-04-30 06:42:28 UTC
and please clear the empty doc folder in '/usr/share/pear/AWSSDKforPHP/_docs'

Comment 9 Gregor Tätzner 2013-05-01 07:46:08 UTC
Created attachment 742106 [details]
pre-review

issues left:

- insufficient requires; see output of 'phpci print --recursive --report extension ...AWSSDKforPHP'

- the tarball ships multiple licenses. I spotted ASL and BSD. Please identify them, adjust the license tag and leave a comment about the location of the files

Comment 11 Gregor Tätzner 2013-05-02 20:30:06 UTC
Created attachment 742915 [details]
diff 1.6.2-3 1.6.2.4

allright, thanks for the quick response

APPROVED

Comment 12 Joseph Marrero 2013-05-02 20:34:09 UTC
New Package SCM Request
=======================
Package Name: php-aws-sdk
Short Description: Amazon web services sdk for php
Owners: jmarrero
Branches: f17 f18 f19 el6
InitialCC:

Comment 13 Gwyn Ciesla 2013-05-03 11:22:13 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2013-05-03 15:16:50 UTC
php-aws-sdk-1.6.2-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/php-aws-sdk-1.6.2-4.fc18

Comment 15 Fedora Update System 2013-05-03 15:17:00 UTC
php-aws-sdk-1.6.2-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/php-aws-sdk-1.6.2-4.el6

Comment 16 Fedora Update System 2013-05-03 15:17:16 UTC
php-aws-sdk-1.6.2-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/php-aws-sdk-1.6.2-4.fc17

Comment 17 Fedora Update System 2013-05-03 15:17:29 UTC
php-aws-sdk-1.6.2-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/php-aws-sdk-1.6.2-4.fc19

Comment 18 Fedora Update System 2013-05-03 23:58:52 UTC
php-aws-sdk-1.6.2-4.fc17 has been pushed to the Fedora 17 testing repository.

Comment 19 Remi Collet 2013-05-04 06:44:14 UTC
Sorry to get here a bit late. But I really thinkg this package shouldn't have been approved.


1/ a lot of file are docs.

Better solution to move docs in docs, is to change "role" in the package.xml, this allow to keep pear metadata consistent (check "pear list aws/sdk")

So, for example:

sed -e '/_samples/s/role="php"/role="doc"/' \
    -e '/_docs/s/role="php"/role="doc"/' \
    -e '/_compatibility_test/s/role="php"/role="doc"/' \
    -e '/_sql/s/role="php"/role="doc"/' \
    -e '/LICENSE/s/role="php"/role="doc"/' \
    -e '/README/s/role="php"/role="doc"/' \
    package.xml >%{pear_name}-%{version}/%{name}.xml

Of course, this is an upstream bug which should be reported and fixed upstream.


2/ bundled libraries
- AWSSDKforPHP/lib/cachecore
- AWSSDKforPHP/lib/dom
- AWSSDKforPHP/lib/yaml
- AWSSDKforPHP/lib/requestcore

Guidelines prohibit this.
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

At least symfony/yaml is available in pear channel so upstream so use a pear dependency.

And this will solves the Licensing issue.


3/ cacert.pem

I think this shouldn't be package, isn't the system one enough (ca-certificates) ?

Comment 20 Remi Collet 2013-05-04 06:55:11 UTC
This package can't even be installed... as it requires 2 opcode cache (xcache and apc), and, by design, only one can be installed.

I think this should be a admin option (not a mandatory requires)

As this are used for user data cache (not for opcode cache), I think requiring a user data only cache will be better (APCU will be a dropin replacement for APC, when approved, as APC is going to be removed from repo).

Comment 21 Remi Collet 2013-05-04 07:02:54 UTC
Please also check phpci output

-------------------------------------------------------------------------------
PHP COMPAT INFO EXTENSION SUMMARY
-------------------------------------------------------------------------------
  EXTENSION                             EXT min/Max       PHP min/Max     COUNT
-------------------------------------------------------------------------------
  Core                                  4.0.0             4.0.0            3019
  PDO                                                     5.1.0               5
  Reflection                                              5.0.0               2
  SPL                                   5.1.0             5.0.0              97
  SimpleXML                                               5.0.0              22
  XCache                                1.0               4.3.0               4
  apc                                   3.0.13            4.3.0               4
  ctype                                                   4.0.4               2
  curl                                                    4.0.2             257
  date                                                    4.0.0              59
  dom                                                     5.0.0              13
  hash                                  1.1               4.0.0              13
  json                                  5.2.0             5.2.0              49
  libxml                                5.1.0             5.0.0               2
  mbstring                                                4.0.6               2
  memcache                              0.2               4.3.11              3
  memcached                             0.1.0             5.2.0               2
  openssl                                                 4.0.4               7
  pcre                                                    4.0.0              60
  session                                                 4.0.0               9
C sqlite3                               0.7               5.3.0               1
  standard                              4.0.0             4.0.0            1690
  zlib                                  1.0               4.0.0              16
-------------------------------------------------------------------------------


Per PHP Guidelines : "PHP extensions must have a Requires on all of the dependent extensions (php-date, php-gd, php-mbstring, ...). These extensions are virtual Provides of the php sub-packages."

For example,  as this library requires "dom", you must requires php-dom (not php-xml).
Especially in Fedora >= 19, with php 5.5, there is a lot of change in extension/package split.

Comment 22 Gregor Tätzner 2013-05-07 06:26:08 UTC
thanks Remi, I've applied for commit access and will fix the remaining issues myself.

Comment 23 Joseph Marrero 2013-05-07 12:28:21 UTC
thanks Remi, I unpushed the updates as soon as you posted the comments. 
Gregor, thanks I am in middle of finals I can not work on this until the 15th of May.

Comment 24 Gwyn Ciesla 2013-05-07 13:17:55 UTC
Unsetting flag.

Comment 25 Gregor Tätzner 2013-05-08 09:44:05 UTC
* Wed May 08 2013 Gregor Tätzner <brummbq@fedoraproject.org> - 1.6.2-5
- unbundle sfyaml
- fix requires
- mark doc in package.xml

(In reply to comment #19)
> 2/ bundled libraries

yaml is out. these 3 are left:

> - AWSSDKforPHP/lib/cachecore
> - AWSSDKforPHP/lib/dom
> - AWSSDKforPHP/lib/requestcore

Do you want to create review requests?


> 3/ cacert.pem
> 
> I think this shouldn't be package, isn't the system one enough
> (ca-certificates) ?

probably, but we should handle this in our hypothetical php-requestcore package.

Comment 26 Fedora Update System 2013-09-09 23:41:41 UTC
php-aws-sdk-2.4.5-2.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/php-aws-sdk-2.4.5-2.fc20

Comment 27 Fedora Update System 2013-09-10 16:23:46 UTC
Package php-aws-sdk-2.4.5-2.fc20:
* should fix your issue,
* was pushed to the Fedora 20 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing php-aws-sdk-2.4.5-2.fc20'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-16275/php-aws-sdk-2.4.5-2.fc20
then log in and leave karma (feedback).

Comment 28 Fedora Update System 2013-09-16 02:01:59 UTC
php-aws-sdk-2.4.5-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/php-aws-sdk-2.4.5-2.fc19

Comment 29 Fedora Update System 2013-09-22 23:55:57 UTC
php-aws-sdk-2.4.5-2.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 30 Fedora Update System 2013-10-06 01:32:43 UTC
php-aws-sdk-2.4.5-2.fc19 has been pushed to the Fedora 19 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.