Bug 1046121 (php-doctrine-cache) - Review Request: php-doctrine-cache - Doctrine Cache
Summary: Review Request: php-doctrine-cache - Doctrine Cache
Keywords:
Status: CLOSED RAWHIDE
Alias: php-doctrine-cache
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: php-doctrine-annotations php-doctrine-common
TreeView+ depends on / blocked
 
Reported: 2013-12-23 18:33 UTC by Shawn Iwinski
Modified: 2014-01-06 17:43 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-01-06 17:43:43 UTC
Type: ---
fedora: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
phpci.log (21.62 KB, text/plain)
2014-01-03 06:52 UTC, Remi Collet
no flags Details
review.txt (10.88 KB, text/plain)
2014-01-03 06:53 UTC, Remi Collet
no flags Details

Description Shawn Iwinski 2013-12-23 18:33:04 UTC
Spec URL: https://raw.github.com/siwinski/rpms/58afbf34df09331c037ce42a9f83d089708ec0ac/php-doctrine-cache.spec

SRPM URL: http://siwinski.fedorapeople.org/SRPMS/php-doctrine-cache-1.3.0-1.fc20.src.rpm

Description:
Cache component extracted from the Doctrine Common project.


Fedora Account System Username: siwinski

Comment 1 Remi Collet 2014-01-03 06:52:51 UTC
Created attachment 844836 [details]
phpci.log

Comment 2 Remi Collet 2014-01-03 06:53:13 UTC
Created attachment 844837 [details]
review.txt

Comment 3 Remi Collet 2014-01-03 06:59:58 UTC
Must:
[!]: Package does not generate any conflict.
  Conflicts:     php-pear(pear.doctrine-project.org/DoctrineCommon) < 2.4
  This is ok, buth worth a comment to explain this is a split-off DoctrineCommon

[!]: Dist tag is present (not strictly required in GL).
  Please use %{?dist}

Cosmetic: 
[!]: %check is present and all tests pass.
  Also remove tests/Doctrine/Tests/Common/Cache/CouchbaseCacheTest.php
  which fails when couchbase extension is installed


About the sub-package, I don't know what is better, to say "If you wand memcached handler, you need to install:
- php-doctrine-cache-memcached (will pull php-pecl-memcached)
OR only
- php-pecl-memcached

Both are acceptable, first is perhaps more confusing for user.

I think you will have an issue when pecl/riak or pecl/couchbase will come to fedora repo (already in my repo, but not ready for review, mostly because of bundled lib) as you will have to move the file from main package to a new sub-package ? (this could create upgrade issue for users)

So perhaps it will be simpler to keep all the handlers in the main package.
This stay your packager choice.

Comment 4 Shawn Iwinski 2014-01-03 19:28:40 UTC
- Conditional %{?dist}
- Removed sub-packages -- This does make more sense and is easier this way... I don't know why I originally sub-packaged
- Skip all tests requiring a server to connect to

Diff: https://github.com/siwinski/rpms/commit/8087060ca33da63e33848809b3215dc4334b7bcd



Spec URL: https://raw.github.com/siwinski/rpms/8087060ca33da63e33848809b3215dc4334b7bcd/php-doctrine-cache.spec

SRPM URL: http://siwinski.fedorapeople.org/SRPMS/php-doctrine-cache-1.3.0-2.fc20.src.rpm

Comment 5 Remi Collet 2014-01-04 06:48:02 UTC
[x]: Package does not generate any conflict.
=> comment ok.

[x]: Dist tag is present (not strictly required in GL).

[x]: %check is present and all tests pass.

- Removed sub-packages -- This does make more sense and is easier this way... I don't know why I originally sub-packaged
=> Ok, simpler and cleaner.

No blocker

== APPROVED ==

Comment 6 Shawn Iwinski 2014-01-04 15:32:39 UTC
THANKS for the review!


New Package SCM Request
=======================
Package Name: php-doctrine-cache
Short Description: Doctrine Cache
Owners: siwinski
Branches: f19 f20 el6
InitialCC:

Comment 7 Gwyn Ciesla 2014-01-06 13:18:50 UTC
Git done (by process-git-requests).


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