Bug 177083 - Review Request: xmldiff
Review Request: xmldiff
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
David Lawrence
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-01-05 21:34 EST by Paul W. Frields
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version: 0.6.7-9
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-02-04 14:43:40 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Doc installation fix, cleanups (928 bytes, patch)
2006-01-08 14:42 EST, Ville Skyttä
no flags Details | Diff

  None (edit)
Description Paul W. Frields 2006-01-05 21:34:05 EST
Spec Name or Url: http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff.spec
SRPM Name or Url: http://marilyn.frields.org:8080/rpm/extras-testing/xmldiff/xmldiff-0.6.7-1.src.rpm

Description:
The xmldiff utility extracts differences between two XML files. It
returns a set of primitives to apply on source tree to obtain the
destination tree.  The implementation is based on _Change detection in
hierarchically structured information_, by S. Chawathe, A. Rajaraman,
H. Garcia-Molina and J. Widom (Stanford University, 1996).
Comment 1 Ville Skyttä 2006-01-06 04:12:05 EST
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".
Comment 2 Paul W. Frields 2006-01-06 10:23:05 EST
- 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
Comment 3 Ville Skyttä 2006-01-08 06:19:57 EST
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
Comment 4 Paul W. Frields 2006-01-08 10:59:35 EST
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

Comment 5 Ville Skyttä 2006-01-08 14:42:13 EST
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.
Comment 6 Paul W. Frields 2006-01-10 21:44:42 EST
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
Comment 7 Ville Skyttä 2006-01-11 16:54:58 EST
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
Comment 8 Ville Skyttä 2006-01-12 02:07:52 EST
"Requires: file" still missing in 0.6.7-5 in CVS (see comment 7).
Comment 9 Paul W. Frields 2006-01-12 10:58:01 EST
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.
Comment 10 Ville Skyttä 2006-01-28 05:11:16 EST
Package still missing from the repos, reopening.
Comment 11 Paul W. Frields 2006-01-28 13:33:23 EST
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?
Comment 12 Ville Skyttä 2006-01-28 14:03:45 EST
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).
Comment 13 Paul W. Frields 2006-01-28 16:27:13 EST
Build good, you should see 0.6.7-7 shortly in the mirrors.
Comment 14 Ville Skyttä 2006-01-28 16:43:04 EST
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.
Comment 15 Paul W. Frields 2006-01-28 17:52:21 EST
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.
Comment 16 Thorsten Leemhuis (ignored mailbox) 2006-01-29 06:42:09 EST
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. 
Comment 17 Ville Skyttä 2006-01-29 06:55:08 EST
(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.
Comment 18 Thorsten Leemhuis (ignored mailbox) 2006-01-29 07:04:09 EST
(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?
Comment 19 Paul W. Frields 2006-01-29 16:49:42 EST
(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
Comment 20 Ville Skyttä 2006-02-04 14:19:26 EST
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
Comment 21 Ville Skyttä 2006-02-04 14:21:00 EST
(In reply to comment #20)
> fixes the test suite on x86_64

s/test suite/test suite failure/
Comment 22 Paul W. Frields 2006-02-04 14:43:40 EST
Great news.  I will patch, remove the ExcludeArch, and push a new release. 
Thanks for finding an x86_64 to test this, Ville.

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