Bug 198706

Created attachment 132340 [details]
The patch

Created attachment 132341 [details]
The template

Ok, I've added a slightly modified version to CVS (fedora-rpmdevtools in the
"fedora" root [0]).  Changes to the template:

- Rename to spectemplate-php-pear.spec (so that people not at all
  knowledgeable about PHP get an idea what's it about), other cosmetics.

- Use %{__pear} consistently everywhere.  I see this actually may have broken
  things somewhat, Requires(foo) will need to be converted to something like
  Requires(foo): %{?__pear}%{!?__pear:/usr/bin/pear}
  Thoughts?

- Drop redundant (isn't it?) Requires: php, pulled in by php-pear(PEAR).

- Add empty %build section to avoid nasty surprises.

- Remove "docdir" at start of %install to make sure --short-circuit builds
  are clean.

Remaining issues/questions:

- Are all php-pear-* packages noarch?

- Unpacking "package.xml" to the top-level build dir is not nice, and may
  result in race conditions and inconsistencies when many php-pear-* packages
  are being simultaneously built.  One possible solution: use -c to %setup,
  and cd into Foo_Bar-%{version} at start of %prep and %install (and %build
  for completeness).

- I cannot test the template because nothing in my FC5 setup provides
  %{__pear} and friends.  Should have a versioned build dependency on the
  package providing the macros, or maybe even a file-based one to the actual 
  /etc/rpm/macros.XXX file

- Do PEAR modules usually/often/ever ship with test suites that could be
  added to %check in the template?

- Are %{pear_testdir}/Foo_Bar, %{pear_datadir}/Foo_Bar and %{pear_phpdir}/Foo
  directories?  I'd like to add trailing '/'s to them to signify that.

[0] http://cvs.fedora.redhat.com/viewcvs/fedora-rpmdevtools/?root=fedora

Created attachment 132611 [details]
Patch eliminating need of macros.pear stuff

Foor for thought: this patch applied over the current rpmdevtools CVS would get
rid of the need to have any pear macros defined at all.  Even though it adds a
bit to the template, I personally think it would be acceptable.  WDYT?

Even better if %post would instead of '-d "$xmldir"' check for '-f
"$xmldir/Foo_Bar.xml"'

Can you give me a patch for the template against the version I uploaded.  I
think we can eliminate some stuff in the newrpmspec patch that is not needed
with your changes.  But I need a patch that will apply cleanly.

Created attachment 132767 [details]
The "unmacroize" changes against the original submitted template

Sure, here goes.  Just out of interest, were there any things you found
controversial in the changes I made before committing my initial slightly
modfied version of the original?  It could be hard to track versions...

In case you're not aware of it, you can grab the rpmdevtools development dir
from CVS, see fedora-rpmdevtools in the fedora root:
:ext:yourusername.redhat.com:/cvs/fedora

Created attachment 132768 [details]
Changes between the submitted and the first committed one

For completeness, here are the changes I made to the original submission before
committing it.	Sorry about that, I should have either asked first, or at least
committed the original version first and then do the changes in a subsequent
commit for tracking purposes.

Okay, I can patch both of these new patches against the original, but I cannot
apply both of them to the same file to get your final result.

Some initial comments though:
Yes, all php-pear packages are noarch and the %build is not needed.  I don't
think it should be added because it should never be used.

I do not understand why you are making a .files section.  And I don't understand
why this includes a defattr line in it, as this is repeated twice it seems. 
What is wrong with just listing the files like I orignally had it, I don't
follow your logic here.

I also don't see why we should define all the directories in the spec file when
they can easily be defined in a macros file with php-pear package.

(In reply to comment #9)
> Yes, all php-pear packages are noarch and the %build is not needed.  I don't
> think it should be added because it should never be used.

I disagree.  We don't know the side effects of that.  One such unexpected side
effect is eg. bug 192422 which doesn't apply here as things are noarch, but
there's nothing that guarantees other similar ones won't happen as it's a matter
of rpm configuration.

> I do not understand why you are making a .files section.

That's one way of avoiding defining the macros, which is the very purpose of the
patch.  Sure, they could be defined within the specfile, but I think this way
it's cleaner.

> And I don't understand why this includes a defattr line in it, as this is
> repeated twice it seems.

That can perhaps be removed.  It's a matter of when rpmbuild reads the filelist
(before parsing the rest of the %files section including the %defattr or after),
will need to check it.

> I also don't see why we should define all the directories in the spec file
> when they can easily be defined in a macros file with php-pear package.

How easily?  They're still not in FC5 and probably never will be in eg. FC4 and
RHEL4 and derivatives, resulting in incompatibilities without much gain at all.

Created attachment 135449 [details]
New Template Incorporating some of Ville's changes

Okay here is a new template that has some of Ville's changes in it.  I did not
incorporate all of the unnecessary stuff because we now have the pear/pecl
macros in the php-pear rpm on FC-5.

Please let me know if there is anything left that needs fixing with this, and
if so please provide clean patches! :)

Created attachment 135451 [details]
Patch to fix Requires

I tested this in mock, and it does not like the %{__pear} in the Requires(post)
and (postun) sections.	This patch reverts back to the original using
/usr/bin/pear

This is the error reported:
error: line 15: Dependency tokens must begin with alpha-numeric, '_' or '/':
Requires(post): %{__pear}

With the latest template + the patch we're back in the situation where
/usr/bin/pear is hardcoded in dependencies but %{__pear} used elsewhere in the
template -- eg. /usr/bin/pear is the dependency for %post and friends scriptlets
which use %{__pear}, not /usr/bin/pear.

So, %{__pear} needs to be conditionally defined in the template, something like:
%{!?__pear: %global __pear /usr/bin/pear} (untested)

Additionally, the BR on php-pear should be bumped from >= 1:1.4.9 to >=
1:1.4.9-4 in order to explicitly require one which is known to contain rest of
the required macro definitions.

Created attachment 135472 [details]
New Template with __pear defined

Okay, here is a new version that has that fixed.

Tested under mock and works.  I forgot to bump the version number, so can you do
this manually before checking it into cvs?

Actually Ville, the version number should be php-pear-1.4.9-1.2 since this is
what it currently is on FC5.

Done, with the small changes I still insist on (at least as long as there's no
designated co-maintainer who will look after the PHP stuff in rpmdevtools): 
%build section, explicit removal of "docdir" at start of %install, whitespace
http://cvs.fedora.redhat.com/viewcvs/fedora-rpmdevtools/?root=fedora&sortby=date

By the way, the 'Unpacking "package.xml" to the top-level build dir is not nice'
issue from comment 3 is still unaddressed, as is the potential cosmetic addition
of trailing '/'s to things in %files that are dirs.  Comments?

Instead of using -c and cp, what if we just do something like:
%prep
%setup -q -n MDB2-%{version}
mv ../package.xml ./%{name}-%{version}.xml

If you insist on adding an empty %build, then I think there should be a comment
above the %build indicating why it is there and empty.

Adding / to the directories does not highlight correctly under vim.  Shouldn't
the syntax highlighting be modified if this is how direcotories are preferred to
be indicated?

(In reply to comment #18)
> Instead of using -c and cp, what if we just do something like:
> %prep
> %setup -q -n MDB2-%{version}
> mv ../package.xml ./%{name}-%{version}.xml

The problem is that package.xml ends up in "..", eg.
$HOME/rpmbuild/BUILD/package.xml instead of
$HOME/rpmbuild/BUILD/MDB2-%{version}/package.xml when %setup unpacks the tarball
- the above would not help, %setup -c would.

> Adding / to the directories does not highlight correctly under vim.  Shouldn't
> the syntax highlighting be modified if this is how direcotories are preferred
> to be indicated?

I know nothing about vim's support for specfile highlighting, but adding
trailing /'s to the %files section didn't seem to change the highlighting at all
here.  Either way, this is not really an issue, just a personal preference.

> The problem is that package.xml ends up in ".."

Must agree.
Another problem is that newest packages use package2.xml (even if package.xml
still present).

What about this ?

%prep
%setup -q -n Log-%{version}
if tar xf %{SOURCE0} package2.xml
then	mv package2.xml Log.xml
elif tar xf %{SOURCE0} package.xml
then	mv package.xml Log.xml
else	echo XML file not found
	exit 1
fi
...

%install
rm -rf %{buildroot} docdir
%{__pear} install --nodeps --packagingroot %{buildroot} Log.xml
...

%{__install} -pm 644 Log.xml %{buildroot}%{pear_xmldir}

Is there a standard defined on what the xml package is named?  I was
unaware that some packages use package2.xml

Or maybe something simpler like this in %prep and always use package2.xml later
during build: [ -f package2.xml ] || mv package.xml package2.xml

How about:

tar xf %{SOURCE0} -O package*.xml > %{name}.xml

More information about package.xml (version 1.0) and package2.xml (version 2.0)
can be found at http://pear.php.net/manual/en/guide.developers.package2.php

"pear makerpm" auto detect which file must be use (using the
package@package2xml@.xml macro in the template).

Created attachment 135621 [details]
Update to CVS version 1.3

This patch is against cvs version 1.3.	It adds a comment to the %build so
users will not get confused wondering if they should put something in there,
and adds the new tar method for extracting the package file.

> tar xf %{SOURCE0} -O package*.xml > %{name}.xml

When package2.xml is present, old package.xml also here.
This merge the 2 files. Not a good thing.

Oh geez why do they do that?

If version one is always going to be there, then why dont we just use:

tar xf %{SOURCE0} -O package.xml > Foo_Bar.xml

Is there any reason why we should use version 2?

Created attachment 135622 [details]
New patch incorporating latest comments

Okay, here is try #2 for a patch against the CVS.  This basically goes back to
Ville's original idea of using -c.

Also, I guess the cd should come before the rm in the %build section.  Ville,
can you modify that by hand when you check this into CVS?

> Is there any reason why we should use version 2?

We should use package2.xml which is designed for laster pear installer release
and is more robust (md5sum included, for example). package.xml is still provided
to allow installation by (very) older pear release.

> [ -f package2.xml ] || mv package.xml package2.xml
> mv package2.xml Foo_Bar-%{version}/Foo_Bar.xml

I think you mean :
> [ -f package2.xml ] || mv package2.xml package.xml
> mv package.xml Foo_Bar-%{version}/Foo_Bar.xml

And I will prefer
[ -f package2.xml ] && \
  mv package2.xml Foo_Bar-%{version}/Foo_Bar.xml || \
  mv package.xml  Foo_Bar-%{version}/Foo_Bar.xml

And, of course :
cd Foo_Bar-%{version}
rm -rf $RPM_BUILD_ROOT docdir
%{__pear} install --nodeps --packagingroot $RPM_BUILD_ROOT Foo_Bar.xml

What about using a macro to define the "class name" (Foo_bar) ?

%define ClassName   Foo_Bar
%define PackageName Foo-Bar
...
Name:           php-pear-%{PackageName}
...
Provides:       php-pear(%{ClassName}) = %{version}
...
%prep
%setup -q -c -n %{ClassName}-%{version}
[ -f package2.xml ] && \
  mv package2.xml %{ClassName}-%{version}/%{ClassName}.xml || \
  mv package.xml  %{ClassName}-%{version}/%{ClassName}.xml

and so on.

I think the idea in comment 32 has some merit -- the fewer times Foo_Bar and
friends occur in the specfile, the better, that way the specfiles between
packages differ less and the template is somewhat easier to use without
rpmdev-newspec (if someone wants to use it that way for whatever reason).

BTW, if -c is used in %setup, there should be no reason to use -n there, patch
from comment 29 applied to CVS as 1.4 with that change and the one from comment
30 (done in both %build and %install for consistency).

I think CVS rev 1.4 looks pretty ok already, but if you think something should
be changed, keep the patches coming.

Ville if you want to add:

%define ClassName   Foo_Bar

and replace all Foo_Bar's with %{ClassName} that is fine by me too.  I agree it
is a good idea.

Otherwise everything looks okay.  When can we expect a new release?

Keep in mind,

%{pear_phpdir}/Foo_Bar.php

Must remain the same in order for the following substitution to take place:

s|^\\(%{pear_phpdir}/\\)$pearname\\(.php\\)|\1$peardirpath\2|

Created attachment 135697 [details]
Latest Patch with %{ClassName}

This is the %{ClassName} patch.  I also added a cd in the %setup section.

Hi

I worked a little on the "template.spec" used by pear to be as close as possible
of this one.

You can have a look at
http://remi.collet.free.fr/rpms/SPEC/php-pear-1.4.11-template.spec

Or even try new it with
http://remi.collet.free.fr/rpms/fc5.i386/php-pear-1.4.11-2.fc5.remi.noarch.rpm

I will file a bug (a RFE) on php-pear core package to include it.
By this way, "pear makerpm" will produce "Extras" ready specfiles.

Next is to wait on php-pear-PEAR-Command-Packaging (bugzilla #185423).

Created attachment 135708 [details]
Forgot one

Forgot one.

Cosmetic note: camelcase %{ClassName} looks unfamiliar in specfiles, I'd use
something like %{pear_name} instead.  Objections?

After the latest patches, there are still these (unmacroized) in %files:
  %{pear_phpdir}/Foo
  %{pear_phpdir}/Foo_Bar.php

Could those be replaced by just this?
  %{pear_phpdir}/*

Oh, I missed comment 35.  Anyway, using %{pear_phpdir}/* would allow simplifying
rpmdev-newspec too - needinfo for comment 39 still holds...

The reason I added those was because in some cases you don't want the package to
own the top level directory.  Like an XML package such as XML_Parser should not
own /usr/share/pear/XML directory since this is clearly owned by php-pear.

However, I see little harm in allowing this because most pear packages behave
this  way anyway (directory ownership is mostly unclear.)  If XML_Parser does
own the XML directory it wont harm anything AFAIK.

So basically the reason I added the top level directory to the spec was for the
purpose of allowing the packager to specify which (if any) directories the
package should own.  However I don't see any harm in allowing all pear packages
to own all the directories.

Comments?

Good point.  The guidelines pretty strongly advice against dir ownership bloat,
so I think we should try to steer away from it in the template.

However, I think adding a comment and still using the "*" approach could be the
best way to go; that's basically how it's done in the other templates too
(although perl is a bit different; due to versioned dirs and a mixture of them
being a valid setup, multiply owned dirs are a necessity there).

I mean something like this:
   # Expand this as needed to avoid owning dirs owned by our dependencies
   %{pear_phpdir}/*

This would make the intention more clear (for example, I missed it from the
current implementation) and still allow the simplifications.  If you're fine
with this, I'll commit it and the latest patches (with s/ClassName/pear_name/).

Maybe the ruby and python spec templates should have a similar comment, but OTOH
I believe reusing some of the install dirs in a way similar to perl and pear is
much less common (practically nonexistent?) with them.

I have no objections to comment #42.

Ok, done.  5.1 release candidate packages are at
http://koti.welho.com/vskytta/tmp/ , please test.  Unless something still needs
changing, I'm planning to release these early next week.

5.1-1 just finished building for FC5 and devel, it'll hit the repos in the next
push.  Thanks!