Bug 445980 - Review Request: odpdom - Oversized Document Parser
Summary: Review Request: odpdom - Oversized Document Parser
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-05-10 23:33 UTC by Dominik 'Rathann' Mierzejewski
Modified: 2008-05-31 16:17 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-05-31 16:17:13 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Dominik 'Rathann' Mierzejewski 2008-05-10 23:33:50 UTC
Spec URL: http://rathann.fedorapeople.org/review/odpdom.spec
SRPM URL: http://rathann.fedorapeople.org/review/odpdom-0.2.1-1.src.rpm
Description:
ODPdom is a simple non-validating DOM (Document Object Model) parser
written in C++, that can handle relatively large XML files with the size in
order of several 10 MB (pro file). Currently it is used to process large
output files (~100MB) from scientific simulations
(http://cms.mpi.univie.ac.at/vasp/Welcome.html).

Comment 1 Jason Tibbitts 2008-05-12 05:12:53 UTC
Builds fine; rpmlint says only:
  odpdom-devel.x86_64: W: no-documentation
  python-odpdom.x86_64: W: no-documentation
which are OK.

If you're going to use the macro forms of commands line %{__make}, you should
use %{__rm} as well.  Also, if you prefer %{buildroot} over $RPM_BUILD_ROOT, you
should use %{optflags} instead of $RPM_OPT_FLAGS.  Just try to be consistent in
your usage of macro forms.

I note you're not using the dist tag.  I guess you maintain enough packages that
you can handle the version juggling required when you don't use the dist tag.

* source files match upstream:
   10aba36b25a1b0e6dcfba10b5211b28b2bbd339365fb9ddc48496d357ecf7036  
   odpdom-0.2.1.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named and is cleanly written.
X specfile does not use macros consistently.
* summary is OK.
* description is OK.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
  odpdom-0.2.1-1.x86_64.rpm
   libODP.so.0()(64bit)
   odpdom = 0.2.1-1
  =
   /sbin/ldconfig
   libODP.so.0()(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)

  odpdom-devel-0.2.1-1.x86_64.rpm
   odpdom-devel = 0.2.1-1
  =
   libODP.so.0()(64bit)
   odpdom = 0.2.1-1

  python-odpdom-0.2.1-1.x86_64.rpm
   _cODP.so()(64bit)
   python-odpdom = 0.2.1-1
  =
   libODP.so.0()(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libpython2.5.so.1.0()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)
   python(abi) = 2.5

* %check is not present; no test suite upstream.
* shared libraries installed; ldconfig is called properly.  Unversioned .so link 
   is in the -devel package.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets OK (ldconfig)
* code, not content.
* headers are in the -devel package.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

Comment 2 Mamoru TASAKA 2008-05-12 18:45:12 UTC
Well, as far as I checked this package
- the original tarball does not provide any shared libraries, only
  static archives are provided
- odpdom-p4v.patch is applied to provide a shared library for Fedora

Agreeing with Patrice, I think this situation must be 
avoided (or treated with much care)
because we cannot expect with what soversion 
the upstream developer will provide a shared library
in the future:
http://fedoraproject.org/wiki/PatriceDumas
see also
https://bugzilla.redhat.com/show_bug.cgi?id=438043

Comment 3 Jason Tibbitts 2008-05-12 18:48:04 UTC
I actually happen to disagree with that; I think it should be our goal to
provide a shared library whenever possible, and see nothing wrong with what's
being done in this package.

Comment 4 Mamoru TASAKA 2008-05-12 19:00:12 UTC
What is the problem is as written in Patrice's wiki page:
- This package provides libODP.so.0 now
- Upstream comes to provide libODP.so.0 (the same name) in the future 
  with no ABI compatibility with what this (Fedora specific) package
  provides "now".

So IMO we must discuss with upstream developer first if we want
to provide shared library for Fedora.

Comment 5 Jason Tibbitts 2008-05-12 21:53:02 UTC
Well, it certainly doesn't hurt to discuss things with the upstream developers.
 Rathann, do you know their opinions on the topic?

However, please keep in mind that we never know what upstream may do.  Any
upstream developer of any library in the distro could do what Patrice describes
in his wiki page at any time anyway, so we have to be prepared to deal with that
with rebuilds if necessary.  We can cover ourselves as well as possible by
picking the lowest possible version.

Besides, Patrice's argument has been used to prevent introduction of shared
libraries for packages which really could use them (and give us security
exposure because of it; see libnet, for example).  I stand firmly against it. 
We have to be able to make a reasonable distro even in the face of upstreams
which do dumb things.

Comment 6 Dominik 'Rathann' Mierzejewski 2008-05-19 18:26:19 UTC
(In reply to comment #1)
> If you're going to use the macro forms of commands line %{__make}, you should
> use %{__rm} as well.  Also, if you prefer %{buildroot} over $RPM_BUILD_ROOT,
> you should use %{optflags} instead of $RPM_OPT_FLAGS.  Just try to be
> consistent in your usage of macro forms.
>
> I note you're not using the dist tag.  I guess you maintain enough packages
> that you can handle the version juggling required when you don't use the dist
> tag.

I will add the dist tag (omitted by accident) and make macro usage consistent.

(In reply to comment #2)
> Well, as far as I checked this package
> - the original tarball does not provide any shared libraries, only
>   static archives are provided
> - odpdom-p4v.patch is applied to provide a shared library for Fedora

Actually that's odpdom-rpm.patch that does that. The -p4v patch comes from
p4vasp, which includes a modified odpdom. So in order to build p4vasp against
shared odpdom, I decided to patch odpdom in the same way, because p4vasp seems
to call that method.

(In reply to comment #5)
> Well, it certainly doesn't hurt to discuss things with the upstream
> developers.
> Rathann, do you know their opinions on the topic?

Not yet, but I will contact them.


Comment 7 Dominik 'Rathann' Mierzejewski 2008-05-28 20:07:51 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Well, it certainly doesn't hurt to discuss things with the upstream
> > developers.
> > Rathann, do you know their opinions on the topic?
> 
> Not yet, but I will contact them.

Both e-mail addresses I found have bounced. Looks like a dead project.

Comment 8 Jason Tibbitts 2008-05-29 16:36:58 UTC
Well, for me the bottom line is that this is the maintainer's decision, and I am
prepared to approve this package if you're satisfied and prepared to handle the
tiny possibility that development picks back up, they decide to begin providing
a shared library _and_ they use a so version lower than what you are using while
ignoring the trouble this would cause.  I would of course recommend that you use
a version that's as low as you can reasonably get, and I don't have this built
at the moment so I haven't checked the version you're using.

Comment 9 Dominik 'Rathann' Mierzejewski 2008-05-31 16:17:13 UTC
Well, I decided to scrap the whole idea. This library would be used by only one
application which doesn't even work properly with anything newer than EL-4 (some
Python incompatibility). Thanks for all your comments and sorry for wasting your
time.


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