Bug 912834 - Review Request: php-dropbox-php-Dropbox - Library for integrating dropbox with PHP
Review Request: php-dropbox-php-Dropbox - Library for integrating dropbox wit...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Shawn Iwinski
Fedora Extras Quality Assurance
:
Depends On: 912833
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-19 14:36 EST by Gregor Tätzner
Modified: 2014-10-30 08:19 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-03-27 16:35:29 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
shawn: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
fedora-review (6.12 KB, text/plain)
2013-03-07 14:55 EST, Shawn Iwinski
no flags Details
phpci (16.44 KB, text/x-log)
2013-03-07 14:56 EST, Shawn Iwinski
no flags Details

  None (edit)
Description Gregor Tätzner 2013-02-19 14:36:09 EST
Spec URL: http://brummbq.fedorapeople.org/php-pear-Dropbox.spec
SRPM URL: http://brummbq.fedorapeople.org/php-pear-Dropbox-1.0.0-1.fc17.src.rpm
Description: This PHP library allows you to easily integrate dropbox with PHP.
The library makes use of OAuth.
Fedora Account System Username: brummbq
Comment 1 Shawn Iwinski 2013-02-24 00:27:42 EST
Joseph -- Do you still plan on doing this review? If not, I can take it.
Comment 2 Gregor Tätzner 2013-02-27 12:06:07 EST
resetting assignee - Shawn you can take it if you want
Comment 3 Shawn Iwinski 2013-02-27 13:34:45 EST
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
Comment 4 Gregor Tätzner 2013-02-27 14:33:09 EST
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
Comment 5 Joseph Marrero 2013-02-27 19:41:20 EST
sorry for the lack of response, been assigned some extra work I was not expecting, Iwinski, it is great that you took it :)
Comment 6 Gregor Tätzner 2013-03-04 13:04:56 EST
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?
Comment 7 Shawn Iwinski 2013-03-05 14:26:51 EST
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.
Comment 8 Shawn Iwinski 2013-03-05 14:31:12 EST
(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.
Comment 9 Gregor Tätzner 2013-03-06 13:28:46 EST
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!
Comment 10 Shawn Iwinski 2013-03-06 14:19:42 EST
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).
Comment 11 Shawn Iwinski 2013-03-06 14:31:11 EST
Oooh, another optional one:

[SHOULD] For "Dropbox/OAuth/Wordpress.php", add a note in %description about the optional Wordpress dependency.
Comment 12 Shawn Iwinski 2013-03-07 09:15:34 EST
Sorry, another must:

[MUST] Add missing group -- "Group: Development/Libraries"
Comment 13 Gregor Tätzner 2013-03-07 09:46:30 EST
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
Comment 14 Shawn Iwinski 2013-03-07 10:29:13 EST
(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.
Comment 15 Shawn Iwinski 2013-03-07 14:55:55 EST
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
Comment 16 Shawn Iwinski 2013-03-07 14:56:31 EST
Created attachment 706780 [details]
phpci
Comment 17 Shawn Iwinski 2013-03-07 15:00:08 EST
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 =====
Comment 18 Gregor Tätzner 2013-03-08 13:17:11 EST
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:
Comment 19 Jon Ciesla 2013-03-08 13:29:38 EST
Git done (by process-git-requests).
Comment 20 Fedora Update System 2013-03-09 05:44:26 EST
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
Comment 21 Fedora Update System 2013-03-09 19:53:36 EST
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.
Comment 22 Fedora Update System 2013-03-27 16:35:32 EDT
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.
Comment 23 Shawn Iwinski 2014-10-29 22:24:59 EDT
Package Change Request
======================
Package Name: php-dropbox-php-Dropbox
New Branches: epel7
Owners: siwinski
InitialCC:
Comment 24 Jon Ciesla 2014-10-30 08:19:52 EDT
Git done (by process-git-requests).

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