Bug 1046121 (php-doctrine-cache)

Summary:

Review Request: php-doctrine-cache - Doctrine Cache

Product:

[Fedora] Fedora

Reporter:

Shawn Iwinski <shawn>

Component:

Package Review

Assignee:

Remi Collet <fedora>

Status:

CLOSED RAWHIDE

QA Contact:

Fedora Extras Quality Assurance <extras-qa>

Severity:

medium

Docs Contact:

Priority:

medium

Version:

rawhide

CC:

fedora, package-review

Target Milestone:

---

Flags:

fedora: fedora-review+
gwync: fedora-cvs+

Target Release:

---

Hardware:

All

OS:

Linux

Whiteboard:

Fixed In Version:

Doc Type:

Bug Fix

Doc Text:

Story Points:

---

Clone Of:

Environment:

Last Closed:

2014-01-06 17:43:43 UTC

Type:

---

Regression:

---

Mount Type:

---

Documentation:

---

CRM:

Verified Versions:

Category:

---

oVirt Team:

---

RHEL 7.3 requirements from Atomic Host:

Cloudforms Team:

---

Target Upstream Version:

Embargoed:

Bug Depends On:

Bug Blocks:

1046125, 1047109

Attachments:

Description	Flags
phpci.log	none
review.txt	none

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).