Bug 912834

Joseph -- Do you still plan on doing this review? If not, I can take it.

resetting assignee - Shawn you can take it if you want

Per PHP package naming guidelines [1], please rename this package to "php-ChannelAlias-PackageName" -- i.e. "php-dropbox-php-Dropbox".  Afterwards I will review as soon as I can.

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

Spec URL: http://brummbq.fedorapeople.org/php-dropbox-php-Dropbox.spec
SRPM URL: http://brummbq.fedorapeople.org/php-dropbox-php-Dropbox-1.0.0-2.fc17.src.rpm

sweet, let me also know if I can review something in exchange

sorry for the lack of response, been assigned some extra work I was not expecting, Iwinski, it is great that you took it :)

I was just wondering: dropbox-php includes some tests (don't by themselves) but doesn't install them. Shall I copy them to the testdir or maybe adjust package.xml?

Hi Gregor -- A few updates required:

* MUST: pear_metadata.  Please add "%{!?pear_metadir: %global pear_metadir %{pear_phpdir}}" to the top of your spec.  Also, in %install, change your cleaning up of unnecessary files to "rm -rf %{buildroot}%{pear_metadir}/.??*".

* MUST: Please change source to: "http://%{channelname}/get/%{pear_name}-%
{version}.tgz". Remote source through the actual PEAR host.

* MUST: Change "BuildRequires:  php-pear >= 1:1.4.9-1.2" to "BuildRequires: php-pear(PEAR)".  Always use the virtual provide.

* MUST: "Requires: php-channel(%{channelname})".  This is required so the pear commands work.

* MUST: Remove "Provides: php-pear(%{pear_name}) = %{version}".  Should only virtual provide "channel/pkg".

* MUST: Please use "%{name}.xml" instead of "%{pear_name}.xml"

* MUST: Do not move PEAR documentation.  It may stay in place in pear_docdir, and just marked as %doc in %files (i.e. "%doc %{pear_docdir}/%{pear_name}")

* COULD: After PEAR remote source, %prep could simply be:
        %prep
        %setup -q -c
        
        # package.xml is version 2.0
        mv package.xml %{pear_name}-%{version}/%{name}.xml

* COULD: Include a note about optional dependency HTTP_OAuth (package name php-pear-HTTP-OAuth) in %description, or even include it as a dependency ("Require: php-pear(HTTP_OAuth)") if you think most users will benefit from it being installed for you package.

(In reply to comment #6)
> I was just wondering: dropbox-php includes some tests (don't by themselves)
> but doesn't install them. Shall I copy them to the testdir or maybe adjust
> package.xml?

Please work with upstream to get the tests added to the PEAR package itself (and make sure they are marked with role="test").  When they are available in the PEAR pkg source, the tests must be run in %check -- although note that you may need make updates to not rely on any remote connections for tests.  Since the tests are not available in the PEAR package right now though, no need to worry about them.

Spec URL: http://brummbq.fedorapeople.org/php-dropbox-php-Dropbox.spec
SRPM URL: http://brummbq.fedorapeople.org/php-dropbox-php-Dropbox-1.0.0-3.fc17.src.rpm

(In reply to comment #7)
> 
> * MUST: Please change source to: "http://%{channelname}/get/%{pear_name}-%
> {version}.tgz". Remote source through the actual PEAR host.

cool, no need to fiddle around with github
 
> * MUST: Do not move PEAR documentation.  It may stay in place in
> pear_docdir, and just marked as %doc in %files (i.e. "%doc
> %{pear_docdir}/%{pear_name}")

I was using the template from rpmdev-newspec, probably that should be updated

> 
> * COULD: Include a note about optional dependency HTTP_OAuth (package name
> php-pear-HTTP-OAuth) in %description, or even include it as a dependency
> ("Require: php-pear(HTTP_OAuth)") if you think most users will benefit from
> it being installed for you package.

added to Requires

(In reply to comment #8)
> Please work with upstream to get the tests added to the PEAR package itself
> (and make sure they are marked with role="test").  When they are available
> in the PEAR pkg source, the tests must be run in %check -- although note
> that you may need make updates to not rely on any remote connections for
> tests.  Since the tests are not available in the PEAR package right now
> though, no need to worry about them.

Will do, but I worry upstream is not that active any more.


and thanks for the help!

Good news: I think this package is almost ready!

Bad news: I found another item that needs to be addressed and another optional item.

[MUST] For "Dropbox/OAuth/PHP.php", you need "php-pecl(oauth)" instead of "php-oauth".  The file uses the OAuth class which is provided by the pecl pkg and not the php-oauth pkg.

[SHOULD] For "Dropbox/OAuth/Zend.php", you need "Zend_{Oauth,Json,Uri}" classes. You can just add a note about the optional Zend Framework dependency in %description so your package does not load that entire framework (I'm not sure if the parts you need are sub-packaged or not).

Oooh, another optional one:

[SHOULD] For "Dropbox/OAuth/Wordpress.php", add a note in %description about the optional Wordpress dependency.

Sorry, another must:

[MUST] Add missing group -- "Group: Development/Libraries"

Spec URL: http://brummbq.fedorapeople.org/php-dropbox-php-Dropbox.spec
SRPM URL: http://brummbq.fedorapeople.org/php-dropbox-php-Dropbox-1.0.0-4.fc17.src.rpm

(In reply to comment #10)
> [MUST] For "Dropbox/OAuth/PHP.php", you need "php-pecl(oauth)" instead of
> "php-oauth".  The file uses the OAuth class which is provided by the pecl
> pkg and not the php-oauth pkg.

good catch

(In reply to comment #12)
> Sorry, another must:
> 
> [MUST] Add missing group -- "Group: Development/Libraries"

No, this is purely optional and legacy anyway.

http://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

(In reply to comment #13)
> (In reply to comment #12)
> > Sorry, another must:
> > 
> > [MUST] Add missing group -- "Group: Development/Libraries"
> 
> No, this is purely optional and legacy anyway.
> 
> http://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Indeed you are correct!  Sorry.

Created attachment 706779 [details]
fedora-review

Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review --mock-config fedora-rawhide-x86_64 -b 912834

Created attachment 706780 [details]
phpci

Thanks for dealing with everything so quickly!


[!]: Spec use %global instead of %define.
     Note: %define pear_name %(echo %{name} | sed -e 's/^php-dropbox-php-//'
     -e 's/-/_/g')

     SHOULD: Please fix this after initial import -- s/%define/%global/


No blockers.

===== APPROVED =====

New Package SCM Request
=======================
Package Name: php-dropbox-php-Dropbox
Short Description: Library for integrating dropbox with PHP
Owners: brummbq
Branches: f17 f18 el6
InitialCC:

Git done (by process-git-requests).

php-dropbox-php-Dropbox-1.0.0-4.fc18, php-channel-dropbox-php-1.3-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/php-dropbox-php-Dropbox-1.0.0-4.fc18,php-channel-dropbox-php-1.3-3.fc18

php-dropbox-php-Dropbox-1.0.0-4.fc18, php-channel-dropbox-php-1.3-3.fc18 has been pushed to the Fedora 18 testing repository.

php-dropbox-php-Dropbox-1.0.0-4.fc18, php-channel-dropbox-php-1.3-3.fc18 has been pushed to the Fedora 18 stable repository.

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

Git done (by process-git-requests).