Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 176591 - Review Request: php-json: An extremely fast PHP extension for JSON
Summary: Review Request: php-json: An extremely fast PHP extension for JSON
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dmitry Butskoy
QA Contact: David Lawrence
URL: http://www.aurore.net/projects/php-json/
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-12-27 08:01 UTC by Ignacio Vazquez-Abrams
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2005-12-31 11:48:31 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
suggested changes for the spec file (1.21 KB, patch)
2005-12-28 13:59 UTC, Dmitry Butskoy
no flags Details | Diff

Description Ignacio Vazquez-Abrams 2005-12-27 08:01:21 UTC
Spec Name or Url: http://fedora.ivazquez.net/files/extras/php-json.spec
SRPM Name or Url: http://fedora.ivazquez.net/files/extras/php-json-1.1.0-1.src.rpm
Description: php-json is an extremely fast PHP C extension for JSON (JavaScript Object Notation) serialisation.

Comment 1 Dmitry Butskoy 2005-12-27 13:49:45 UTC
As this package can be built with different "main" phps, IMHO it is better to do
"phpize" (i.e., "phpize --clean" after untar to clear, and then "phpize" to
create needed stuff).
PHP-4.3.11 (FC3), PHP-5.0.4 (FC4) and PHP-5.1.1 (FC5) can have different phpize
results.

It seems that upstream developers already run some "phpize" (like an analogue of
autotools), but it is more robust to rerun it for different php versions.

Hint:

%prep
%setup -q -n php-json-ext-%{version}
phpize --clean
phpize

%build
%configure
make %{?_smp_mflags}


Comment 2 Ignacio Vazquez-Abrams 2005-12-27 21:29:05 UTC
Updated.

Comment 3 Dmitry Butskoy 2005-12-28 13:57:22 UTC
OK

Remarks & nitpicks:
- autodetection of extdir and version can be simplified a little (as good
php-config always present in the Fedora's php-devel package)
- Use %{?dist} in the release field (as this package hardly depends to specific
distro's php version)
- Remove leading "An" from the summary
- Some %check section would be useful (at least to check that this module is
successfully attached at run-time and is visible by the /usr/bin/php).

See all these suggestions in the patch below.

I am confused a little by the fact there is no any documentation in this
package. Perhaps some data can be obtained from the upstream's site? (By simple
copying some *.html files etc.)

Comment 4 Dmitry Butskoy 2005-12-28 13:59:37 UTC
Created attachment 122613 [details]
suggested changes for the spec file

Comment 5 Ignacio Vazquez-Abrams 2005-12-29 00:01:32 UTC
(In reply to comment #3)
> - autodetection of extdir and version can be simplified a little (as good
> php-config always present in the Fedora's php-devel package)

Except when mock does its depsolving.

> - Use %{?dist} in the release field (as this package hardly depends to
specific distro's php version)

I always add that once it's in CVS.

> - Remove leading "An" from the summary

Sorry, but I refuse to turn the Summary into grammatical garbage. Bring it up on
the mailing list if you wish to discuss this.

Updated.

Comment 6 Dmitry Butskoy 2005-12-29 12:19:00 UTC
> Except when mock does its depsolving.
Not understood how mock can have issues here, but it is not a blocker :)

> I always add that once it's in CVS.
Why not before? IMO the PackageGuidelines requires it at review stage... Anyway,
all other guys do it before CVS... 

- installed pam-json.html has execution bit set. Actually, "x" bit is set
immediately after "install -p %{SOURCE1}". Perhaps changing "install" to "cp"
can solve this.

- Initially, I resisted to increase release numbers after each trivial change.
But eventually various reviewers have forced me to do it always... ;)

Comment 7 Ignacio Vazquez-Abrams 2005-12-31 03:31:00 UTC
(In reply to comment #6)
> > Except when mock does its depsolving.
> Not understood how mock can have issues here, but it is not a blocker :)

mock looks at the bare spec file and evaluates it, possibly before any BRs have
been installed. Try it some day.

> > I always add that once it's in CVS.
> Why not before? IMO the PackageGuidelines requires it at review stage... Anyway,
> all other guys do it before CVS... 

Most all other guys don't have %dist defined on their system, and it gives the
buildsystem conniptions if I import it with %dist as .fc4 and then try to tag in
FC-4.

> - installed pam-json.html has execution bit set. Actually, "x" bit is set
> immediately after "install -p %{SOURCE1}". Perhaps changing "install" to "cp"
> can solve this.

No, this is because I misused install. Fixed.

> - Initially, I resisted to increase release numbers after each trivial change.
> But eventually various reviewers have forced me to do it always... ;)

It's not supposed to be used by the public yet, so I figure no harm, no foul.

Updated.

Comment 8 Dmitry Butskoy 2005-12-31 11:04:27 UTC
> it gives the buildsystem conniptions if I import it with %dist as .fc4 and
then > try to tag in FC-4.
make tag TAG_OPTS="-F" ... :)

OK
APPROVED.

Comment 9 Ignacio Vazquez-Abrams 2005-12-31 11:48:31 UTC
Built on FC4 and devel


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