Bug 176591 - Review Request: php-json: An extremely fast PHP extension for JSON
Review Request: php-json: An extremely fast PHP extension for JSON
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dmitry Butskoy
David Lawrence
http://www.aurore.net/projects/php-json/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-12-27 03:01 EST by Ignacio Vazquez-Abrams
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-12-31 06:48:31 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


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

  None (edit)
Description Ignacio Vazquez-Abrams 2005-12-27 03:01:21 EST
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 08:49:45 EST
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 16:29:05 EST
Updated.
Comment 3 Dmitry Butskoy 2005-12-28 08:57:22 EST
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 08:59:37 EST
Created attachment 122613 [details]
suggested changes for the spec file
Comment 5 Ignacio Vazquez-Abrams 2005-12-28 19:01:32 EST
(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 07:19:00 EST
> 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-30 22:31:00 EST
(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 06:04:27 EST
> 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 06:48:31 EST
Built on FC4 and devel

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