Bug 1047596 - Review Request: php-channel-phpseclib - Adds the phpseclib channel to PEAR
Summary: Review Request: php-channel-phpseclib - Adds the phpseclib channel to PEAR
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: 1047599 1047600 1047601 1047602 1047603 1047604 1047605 1047607 1047610
TreeView+ depends on / blocked
 
Reported: 2014-01-01 02:08 UTC by Adam Williamson
Modified: 2014-02-25 18:46 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-01-09 22:10:13 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
review.txt (6.51 KB, text/plain)
2014-01-01 06:35 UTC, Remi Collet
no flags Details

Description Adam Williamson 2014-01-01 02:08:17 UTC
Spec URL: https://www.happyassassin.net/reviews/php-channel-phpseclib/php-channel-phpseclib.spec
SRPM URL: https://www.happyassassin.net/reviews/php-channel-phpseclib/php-channel-phpseclib-1.0-1.fc21.src.rpm
Description: This package adds the phpseclib channel which allows PEAR packages
from this channel to be installed.
Fedora Account System Username: adamwill

Cargo-culted from existing PHP channel specs, with the obvious changes.

Comment 1 Remi Collet 2014-01-01 06:35:29 UTC
Created attachment 844042 [details]
review.txt

Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -b 1047596
Buildroot used: fedora-19-x86_64
Active plugins: Generic, PHP, Shell-api
Disabled plugins: Java, C/C++, Python, SugarActivity, Perl, R, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG

Comment 2 Remi Collet 2014-01-01 06:43:53 UTC
Small notes:
- version have no sense for such package, but we are used to take the rest version (so 1.3). 

BuildRequires:       php-pear >= 1:1.4.9-1.2
=> minimal version have no more sense (RHEL-4 is EOL)

mixed-use-of-spaces-and-tabs
=> trivial, could be fixed on import.

As this channel provides a lot of extensions which already exists in standard pear channel (probably all are not packaged) we should be very carefull with each package, to check file conflicts, in peardir, docdir, ...

I will try to go forward on the other reviews.

A possible solution could be to set a specific pear_phpdir, pear_docdir, pear_testdir... for this channel (so in the %post of this package). but I hope this is not required and we can go without.

No blocker.

=== APPROVED ===

Comment 3 Adam Williamson 2014-01-01 07:32:46 UTC
"version have no sense for such package, but we are used to take the rest version"

ah, OK, I took the 'version' from the .xml file.

"minimal version have no more sense (RHEL-4 is EOL)"

cargo culted from the other specs :)

"mixed-use-of-spaces-and-tabs"

more cargo culting...

"As this channel provides a lot of extensions which already exists in standard pear channel (probably all are not packaged) we should be very carefull with each package, to check file conflicts, in peardir, docdir, ..."

We don't actually have much php crypt stuff packaged that I can see. I did check for conflicts other than Blowfish and didn't see any, but a double check would be welcome indeed.

"A possible solution could be to set a specific pear_phpdir, pear_docdir, pear_testdir... for this channel (so in the %post of this package). but I hope this is not required and we can go without."

Yeah, that was more or less my feeling - might be necessary, but let's hope not.

I'll fix the things noted on import, thanks!

Comment 4 Adam Williamson 2014-01-01 07:34:13 UTC
New Package SCM Request
=======================
Package Name: php-channel-phpseclib
Short Description: Adds the phpseclib channel to PEAR
Owners: 
Branches: f19 f20 el6
InitialCC:

Comment 5 Gwyn Ciesla 2014-01-02 12:52:11 UTC
No FAS accounts listed.

Comment 6 Adam Williamson 2014-01-02 18:02:41 UTC
huh, I was sure I put that in.

New Package SCM Request
=======================
Package Name: php-channel-phpseclib
Short Description: Adds the phpseclib channel to PEAR
Owners: adamwill
Branches: f19 f20 el6
InitialCC:

Comment 7 Gwyn Ciesla 2014-01-02 18:12:19 UTC
Git done (by process-git-requests).

Comment 8 Adam Williamson 2014-01-04 23:32:12 UTC
Just a note on the namespace conflict problem: see https://github.com/phpseclib/phpseclib/issues/243 . Upstream has a 'php5' branch they're working on, and they plan to namespace phpseclib as part of that branch (looks like they're planning to switch to Composer). So the conflict will go away when that happens. I'll follow upstream and switch to the php5 branch when it makes sense to do so.

Comment 9 Adam Williamson 2014-01-09 22:10:13 UTC
Built in all branches.

Comment 10 Adam Williamson 2014-02-24 03:19:50 UTC
Package Change Request
======================
Package Name: php-channel-phpseclib
New Branches: epel7

Comment 11 Gwyn Ciesla 2014-02-24 13:18:44 UTC
No owners specified.

Comment 12 Adam Williamson 2014-02-25 18:16:12 UTC
It's a *change* request. Why do I have to list the owners?

(same comment applies to every one of the goddamn change requests I painstakingly did that you goddamn refused, I mean really? You couldn't just ask me on IRC or something?)

Comment 13 Adam Williamson 2014-02-25 18:18:08 UTC
Sigh.

"The Package Name field is mandatory. Please only include other fields which need to be changed or updated. In the Owners field list the branch owner and any comaintainers. Please note that when the new branch is created, ownership or CC information will not be copied to the new branch, so be sure to specify in the request all of the owners and initialCC members the new branch should have."

is possibly the most confusing paragraph of instruction I've ever read. Its meaning changes with each cumulative sentence. This whole process really is like a loaded gun with a random action timer attached to the trigger, and a foot discovery targeting system.

Comment 14 Adam Williamson 2014-02-25 18:22:31 UTC
Package Change Request
======================
Package Name: php-channel-phpseclib
New Branches: epel7
Owners: adamwill

Comment 15 Gwyn Ciesla 2014-02-25 18:46:53 UTC
Git done (by process-git-requests).


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