Bug 177083

Some differences found in my local package, just skimming the specfiles for now:
http://cachalot.mine.nu/4/SRPMS/xmldiff-0.6.7-0.1.src.rpm

- manpages should be converted to UTF-8
- stuff in %{python_sitearch}/xmldiff/test is probably not needed
- is PyXML really required?
- xmlrev needs xsltproc and sgmlnorm and possibly docbook-xsl

It'd be nice to run the test suite but the last time I checked, it needed some
extra logilab python libs.  See "python test/runtests.py".

- Manpages converted
- Dropped test suite
- Sorry, I relied on mfr's page, saw xml.sax in parser.py, and didn't realize
that was in python pkg; removed
- Requires: docbook-style-xsl (yes, it's needed) takes care of openjade
(sgmlnorm); also added Requires: libxslt (xsltproc)

Thanks for checking this further.  Since I do a lot of docs work and have the
"A&P" buildgroup installed, my testing was flawed. New versions available:

SPEC: http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff.spec
SRPM:
http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff-0.6.7-2.src.rpm

Doing the manpage charset conversion in %install will break --short-circuit
builds as the pages will be converted more than one time.  Moving that to %prep
fixes it.

Relying on docbook-style-xsl to pull in sgmlnorm is fragile.  sgmlnorm has been
moved to the new opensp package in Rawhide and docbook-style-xsl doesn't really
require either openjade or opensp; it's more like an implementation glitch in
the current docbook-dtds package.  Requires: /usr/bin/sgmlnorm would make this
more robust.

No need to define python_sitelib at top of specfile, it's not used.

To enable the test suite, add BuildRequires: python-logilab-common and after
%install:
    %check
    cd test
    PYTHONPATH=$(ls -1d $PWD/../build/lib.*) %{__python} runtests.py

Done.  Builds OK in mock, tests complete without error.  I'm thinking there's
probably a lot less fragility in allowing the BR on libxslt to drag in libxml2
(/usr/bin/xmllint), correct?

SPEC: http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff.spec
SRPM:
http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff-0.6.7-3.src.rpm

Created attachment 122931 [details]
Doc installation fix, cleanups

Well, libxslt is inherently a library package, and those are always subject to
being split so that the utils live in their own subpackages (multilib reasons),
so I'd say being explicit about xsltproc wouldn't hurt either.

We're getting close to approval here, the attached patch has a few trivial
cleanups and fixes doc installation (*.html and *.txt were not actually
included).

And one more blocker: installation path of docbook_rev.xsl and xmlrev.xslt is
both IMO a bit strange and contains unowned directories
(%{_datadir}/sgml/stylesheet and %{_datadir}/sgml/stylesheet/xmldiff).	I'd
suggest moving the XSL's to %{_datadir}/xml/xmldiff and owning that dir too.

Done.  I also found some additional files in need of iconv help, and the package
now properly owns %(python_sitearch}/xmldiff as well.

SPEC: http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff.spec
SRPM:
http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff-0.6.7-4.src.rpm

Approved with the following final fixes applied:

- Requires: file (see /usr/bin/xmlrev)

- /usr/bin/xmlrev doesn't find xmlrev.xslt, ML_DIR in it needs to be changed to
  /usr/share/xml/xmldiff

"Requires: file" still missing in 0.6.7-5 in CVS (see comment 7).

Sorry Ville, I somehow managed to miss that line in your comment entirely.  I
hadn't tagged or built anything yet, so I simply added that fix to the existing
release 0.6.7-5.  Thanks for your patient help.

Package still missing from the repos, reopening.

There is a build problem with x86_64, specifically some of the tests fail in
%check: 

http://buildsys.fedoraproject.org/logs/fedora-development-extras/3281-xmldiff-0.6.7-6.fc5/x86_64/build.log

If someone with an x86_64 available could help troubleshoot, that would be
great.  Otherwise should I use BuildArch?  ExclusiveArch?  Another tag?

I don't have an x86_64 available yet, but if you end up needing to exclude
x86_64, do it with "ExcludeArch: x86_64" and place a comment to the specfile
next to that that points to a bugzilla bug number/link with more information (in
this case, this bug would be fine).

Build good, you should see 0.6.7-7 shortly in the mirrors.

Note that the comment syntax used in 0.6.7-7's ExcludeArch does not do what I
think you expected:

    $ rpm -qp --qf '[%{EXCLUDEARCH}\n]' xmldiff-0.6.7-7.fc5.src.rpm
    x86_64
    #
    see
    bug
    #177083

The comment should be moved away from the ExcludeArch line.

You're right, it doesn't do what I expected.  Chalk it up to the uncertainty of
the words "next to". ;-)  Fixed in 0.6.7-8.

Sorry, I have to jump in here ;-)

(In reply to comment #12)
> [...] but if you end up needing to exclude
> x86_64, do it with "ExcludeArch: x86_64" and place a comment to the specfile
> next to that that points to a bugzilla bug number/link with more information (in
> this case, this bug would be fine).

The last sentence is not 100% correct. Please open a separate bug for it just as
the Review Guidelines demand:

Each architecture listed in ExcludeArch needs to have a bug filed in bugzilla,
describing the reason that the package does not compile/build/work on that
architecture. The bug number should then be placed in a comment, next to the
corresponding ExcludeArch line. New packages will not have bugzilla entries
during the review process, so they should put this description in the comment
until the package is approved, then file the bugzilla entry, and replace the
long explanation with the bug number. The bug should be marked as blocking one
(or more) of the following bugs to simplify tracking such issues:
FE-ExcludeArch-x86, FE-ExcludeArch-x64, FE-ExcludeArch-ppc

(Okay, the last sentence is just half an hour old, but the other part is older.)

Paul, and please attach the build-log with the error to that bug because it will
be deleted from the server in some days.

And btw: A simple google search shows that other distributions were able to get
it running on x86_64 so it's probably not that hard to fix (even if the packager
has no access to that x86_64).

/me also wonders if we need a defined wait period (4 days?) before a ExludeArch
like this is allowed (especially in new packages) -- people interested in x86_64
practically had no chance to look at this problem and help fixing before the
package was pushed. That not how it should work IMHO. 

(In reply to comment #16)
> And btw: A simple google search shows that other distributions were able to 
> get it running on x86_64 so it's probably not that hard to fix

Well, many people and distributions often don't run test suites during builds
and sometimes ship plain broken packages :).  I haven't checked if that's the
case here.

> people interested in x86_64
> practically had no chance to look at this problem and help fixing before the
> package was pushed. That not how it should work IMHO. 

Agreed.  I was unpleasantly surprised to see this pushed with ExcludeArch and
"build good" comment about two hours after my "...if you end up needing to
exclude x86_64..." comment.

(In reply to comment #17)
> (In reply to comment #16)
> > And btw: A simple google search shows that other distributions were able to 
> > get it running on x86_64 so it's probably not that hard to fix
> 
> Well, many people and distributions often don't run test suites during builds
> and sometimes ship plain broken packages :).

Yeah, that's true ;-)

>  I haven't checked if that's the
> case here.

Paul?

(In reply to comment #17)
> (In reply to comment #16)
> > And btw: A simple google search shows that other distributions were able to 
> > get it running on x86_64 so it's probably not that hard to fix
> 
> Well, many people and distributions often don't run test suites during builds
> and sometimes ship plain broken packages :).  I haven't checked if that's the
> case here.

The only 0.6.7 packages I found did not run the test suites, so they wouldn't
have seen this problem.  If anyone would like to give substantive direction on
fixing the problem, that would be appreciated.  I found only the following
regarding x86_64 related issues:
http://lists.logilab.org/pipermail/xml-projects/2005-June/000372.html

> > people interested in x86_64
> > practically had no chance to look at this problem and help fixing before the
> > package was pushed. That not how it should work IMHO. 
> 
> Agreed.  I was unpleasantly surprised to see this pushed with ExcludeArch and
> "build good" comment about two hours after my "...if you end up needing to
> exclude x86_64..." comment.

Sorry about the early push; I was in the middle of many other tasks that day and
forgot that this one should have percolated for a while.  For some reason, I
thought failed builds went to the list, but I see that I was mistaken.  (That
was why the package languished originally -- to allow time for comment.)  Since
the package does work on i386 and ppc, once I have help with the x86_64 problem,
I can add the patch and make a new release.  So in summary, does this plan suffice?:

1. File bug as noted above, attaching build.log and blocking this bug and
"FE-ExcludeArch-x64"
2. Note number in an additional comment in this bug, since this one is
referenced in %changelog in xmldiff package currently, per Ville's request earlier
3. Send email to f-extras-l requesting x86_64 assistance (e.g. Hans?),
referencing new bug
4. Fix problem, test and package new release

The patch in the message pointed out in comment 19 fixes the test suite on
x86_64.  FWIW, it still has these compilation warnings:

extensions/maplookup.c: In function 'fmes_node_equal':
extensions/maplookup.c:160: warning: cast from pointer to integer of different size
extensions/maplookup.c:164: warning: cast from pointer to integer of different size

(In reply to comment #20)
> fixes the test suite on x86_64

s/test suite/test suite failure/

Great news.  I will patch, remove the ExcludeArch, and push a new release. 
Thanks for finding an x86_64 to test this, Ville.