Bug 195645 - Review Request: rasqal - RDF query library
Review Request: rasqal - RDF query library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 195647
  Show dependency treegraph
 
Reported: 2006-06-16 07:09 EDT by Thomas Vander Stichele
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-27 06:03:57 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Thomas Vander Stichele 2006-06-16 07:09:33 EDT
Spec URL: https://apestaart.org/thomas/trac/browser/pkg/fedora.extras/rasqal/rasqal.spec
SRPM URL: http://thomas.apestaart.org/download/pkg/fedora-5-i386-extras/rasqal-0.9.12-1.fc5/rasqal-0.9.12-1.fc5.src.rpm
Description: 

Rasqal is a library providing full support for querying Resource
Description Framework (RDF) including parsing query syntaxes, constructing
the queries, executing them and returning result formats.  It currently
handles the RDF Data Query Language (RDQL) and SPARQL Query language.
Comment 1 Jason Tibbitts 2006-06-17 02:15:32 EDT
Builds fine on x86_64 development.  rpmlint has this to say:

W: rasqal invalid-license LGPL or Apache 2.0
W: rasqal-debuginfo invalid-license LGPL or Apache 2.0
W: rasqal-devel invalid-license LGPL or Apache 2.0

rpmlint likes to see "Apache License" but still doesn't know what to do with the
version; I would suggest using "LGPL or Apache License 2.0".

W: rasqal no-version-in-last-changelog
W: rasqal-debuginfo no-version-in-last-changelog
W: rasqal-devel no-version-in-last-changelog

The accepted changelog format includes "version-release" after the email address.

E: rasqal binary-or-shlib-defines-rpath /usr/bin/roqet ['/usr/lib64']

You should eliminate the use of rpath if at all possible.  You can do this by
adding BuildRequires: libtool, then adding "LIBTOOL=/usr/bin/libtool" on the
make line, and deleting any *.a files when you delete the *.la files.  I've
verified that this works but you should retest the package to make sure nothing
has broken.

Additional comments about the specfile:

%makeinstall should not be used. See
http://fedoraproject.org/wiki/Packaging/Guidelines#MakeInstall

Consider adding %{?_smp_mflags} to the make line.  In addition, your passing of
OPTIMIZE doesn't seem to have any effect; gcc is still called with the
appropriate options even after deleting it.

You use %{__make}, but don't use %{__rm} or %{__install}.  Macro use should be
consistent; it is up to you to use the style you prefer, but you should not mix
styles.

These are all pretty minor, and fixing them up doesn't seem to harm anything,
but you should still test things out.

Also, there seems to be an extensive test suite included.  I added a quick
%check section and it seems there are some off failures and such all over.  I'm
not sure if that's expected behavior or not.
Comment 2 Thomas Vander Stichele 2006-06-17 06:27:51 EDT
Thanks for reviewing, much appreciated !

- changed license, but it still complains.  Filed
http://rpmlint.zarb.org/cgi-bin/trac.cgi/ticket/22

- re: changelog version; my name + email is rather long, and frequently extends
the width to more than 80 chars if I add version.  I filed 
http://rpmlint.zarb.org/cgi-bin/trac.cgi/ticket/23
and
http://rpmlint.zarb.org/cgi-bin/trac.cgi/ticket/24 (with patch)
If this is not critical I would like to keep this as-is for aesthetic reasons.

- got rid of rpath, ugh @ hacks.  This will teach me to actually lint on a 64
bit machine - apparently this doesn't cause trouble on a 32 bit one.

- %makeinstall: that wiki entry is factually wrong and lacking any actual
reasons except dogma - I will ask spot about it.  In the case of this package,
there is nothing wrong when doing makeinstall.  Unless you insist I prefer to
keep it.

- smp_mflags added
- OPTIMIZE removed
- make instead of %{__make} done

- make check added.  pinging upstream about test failures - since upstream did
not set it up to actually error out on those errors, and there are few, I'm
assuming for now these are expected or non-critical failures.

New version:
http://thomas.apestaart.org/download/pkg/fedora-5-i386-extras/rasqal-0.9.12-2.fc5/
Thanks again !
Comment 3 Jason Tibbitts 2006-06-18 15:20:13 EDT
The changes look good to me.

The license tag is fine now; I don't think anyone's decided on any standard way
to specify the license version, but it will have to happen eventually.

I see your point about the changelog; as long as the version number gets in
there it should be OK.

Which leaves %makeinstall.  I really don't want to get in the middle of an
argument about it, expecially since this was just covered on one of the mailing
lists a week or so ago, but from my standpoint the guidelines are clear.  spot
just added that bit to the guidelines ten days ago so this isn't an outdated
rule.  I haven't taken this ticket for review so if someone feels differently
they can take it.
Comment 4 Thomas Vander Stichele 2006-06-18 16:14:42 EDT
It's not that I do not want to use %makeinstall - it's that the packaging
guidelines that mention you shouldn't use it do not say *anything* useful about
it, why it's bad, and they're not even correct.   If I need to make this change,
I need to know because I have a bunch of other packages using %makeinstall.

Let's take those rules step by step:

      %makeinstall overrides a set of environment variables during "make
install". I.e. it performs make prefix="..." includedir="..." ...

This is wrong - it overrides make variables, not environment variables.  There's
a difference between

prefix= make install

and

make prefix= install

In addition, this is all the rule says - it does not say that it is wrong or why
it is wrong.  So it's a) factually wrong and b) irrelevant.


      It is error prone, and can have unexpected effects when run against less
than perfect Makefiles.

How is it error prone ? How does make DESTDIR=... not fail when run against a
less than perfect Makefile - for example, one that doesn't even *have* DESTDIR ?

      It can trigger unnecessary rebuilds when executing "make install"

Don't know about this one, it may be true or may not be true, but in all the
packages I've built I've never known this to be a problem that actually bothered me.

      If a package contains libtool archives, it can cause broken *.la files to
be installed.

I haven't seen broken .la files, and it's not relevant - we do not ship .la
files, we delete them from every package.

If you want, I'll discuss it more directly with spot.  There may be actual good
reasons against makeinstall, but whatever they are, the ones in the wiki aren't
those.

I've searched through my local archives for any mails that could be related to
this, and didn't find anything straight away - though the lists are big, so I
may have missed them.  Do you have a link to the thread where this was discussed ?

Thanks
Thomas
Comment 5 Jason Tibbitts 2006-06-18 22:50:18 EDT
As I wrote previously, I don't want to get into the middle of an argument about
it.  I did, however, exercise google a bit; you can find the most recent
discussion under the thread starting at:
http://www.redhat.com/archives/fedora-extras-list/2006-June/msg00420.html
This led directly to the current section being added to the guidelines.  Some
additional searching will turn up at least
http://www.redhat.com/archives/fedora-extras-list/2005-June/msg00800.html
and also some discussion from 2003 that seems to be archived at fedora.us which
is no longer answering.

This was decided a long time ago and unfortunately was not added to the
guidelines until ten days ago.
Comment 6 Paul Howarth 2006-06-19 03:17:26 EDT
Unfortunately the wording on the wiki is poor, and it shouldn't be a MUST NOT.
Whilst using "make DESTDIR=..." is clearly superior where both approaches work,
not all makefiles support DESTDIR and in those cases the use of %makeinstall is
appropriate.
Comment 7 Jason Tibbitts 2006-07-06 21:26:25 EDT
Is there any chance we could make some progress here?  Perhaps the guidelines do
need to be clarified a bit, but I think the only way they're going to change is
to allow %makeinstall if and only if "make DESTDIR=..." doesn't work, which it
clearly does here.
Comment 8 Thomas Vander Stichele 2006-09-02 06:27:08 EDT
Jason,

obviously there is disagreement over this.  So as I've stated before, unless you
insist or do not want to review/approve this package over this issue, I want to
keep it as it is.

I am however far too jaded when it comes to Fedora to not consider the pragmatic
approach given how bad chances are in general to get a package through :) But I
do need your official insistance if there is such.

Thanks
Thomas
Comment 9 Jason Tibbitts 2006-09-02 11:55:50 EDT
Actually there's not much in the way of disagreement.  The guidelines currently
state:

Fedora's RPM includes a %makeinstall macro but it must NOT be used when make
install DESTDIR=%{buildroot} will work. 

http://fedoraproject.org/wiki/Packaging/Guidelines#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002

Note "must".  This was voted on by the packaging committee and ratified by FESCo
and the Fedora Board.

Now, I'm not currently reviewing this, so anybody else is free to take it and
perhaps you will find a reviewer who is willing to ignore the guidelines here. 
I don't, however, think anyone is really going to want to deal with the fallout
which would almost certainly occur from that.  I'm not going to insist that you
make any changes to your package, but personally I think there's not much chance
it being accepted into Extras otherwise.
Comment 10 Paul F. Johnson 2006-09-02 12:15:15 EDT
If you ignore the guidelines then what is the point of having them. Hell, let's
have a free for all where we can do what we please, shove what we want where we
want and to hell if we end up with a distro as screwed up as XP.

If a package doesn't follow the guidelines, it doesn't get in. Simple as that.

There is a *very* rare case where a package can break the guidelines, but it's
so rare that I've only come across it once and even then the breakage delayed
getting the package in by weeks as FESCo discussed it.

Not even the RedHat engineers can break the packaging guidelines, so it's not as
if it's one rule for one and another for another.
Comment 11 Kevin Fenzi 2006-10-01 12:34:46 EDT
Thomas: 

I was going to review redland and redland-bindings, but this package is needed 
first. 

I can't speak for other reviewers, but I won't approve your packages if they 
call %makeinstall when 'make DESTDIR=' works. Thats in the guidelines. 

I see several ways forward: 

- You can change this (and your other submissions) to just call 'make 
DESTDIR=...' and we can get them reviewed and approved. 

and/or

- You can petition the packaging folks to change the rules, or better explain 
why the rules are the way they are. 

May I suggest you do both? Change things for now, and also ask the packaging 
folks, and if you get the rule changed you can go and change your packages in 
CVS later. 

Does that sound reasonable to move these forward? 
Comment 12 Jason Tibbitts 2006-10-01 13:17:00 EDT
The packaging committee did work on that guideline rather recently, to clarify
the situations where %makeinstall is permitted.  I think there is very little
chance of the guideline changing again unless someone can bring up some
justification for %makeinstall that was missed earlier.
Comment 13 Kevin Fenzi 2006-10-14 12:14:40 EDT
Thomas: Any reply to my plea in comment #11?

Do you still wish to submit this (and redland/redland-bindings)? 
Comment 14 Thomas Vander Stichele 2006-10-27 12:32:06 EDT
yes, I actually updated it yesterday, but I need to sort out my new work desktop
to upload packages to my website.  It will be soon :)
Comment 15 Kevin Fenzi 2006-11-11 20:39:25 EST
Hey Thomas. Any further news on this package? 
Comment 17 Kevin Fenzi 2006-11-19 15:38:31 EST
Thanks for changing the makeinstall... here's a review:

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (LGPL or apache 2.0)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
403b95de5c23124f6a6491bdde3eba86  rasqal-0.9.12.tar.gz
403b95de5c23124f6a6491bdde3eba86  rasqal-0.9.12.tar.gz.1

See below - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Headers/static libs in -devel subpackage.
OK - Spec has needed ldconfig in post and postun
See below - .pc files in -devel subpackage/requires pkgconfig
OK - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - .la files are removed.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK (386/x86_64) - Should build on all supported archs
OK - Should have subpackages require base package with fully versioned depend.
OK - Should have dist tag
See below - Should package latest version

Issues:

1. 0.9.13 seems to be out, might update to that?

2. Are there some missing BuildRequires?
checking for pcre-config... no
checking for pcre-config... no
checking for pcre... not present
checking for xml2-config... no
checking for libxml2 library... no

3. The devel subpackage has a .pc, so should Requires: pkgconfig?

4. rpmlint says:

W: rasqal invalid-license LGPL or Apache Software License 2.0
W: rasqal invalid-license LGPL or Apache Software License 2.0
W: rasqal-debuginfo invalid-license LGPL or Apache Software License 2.0
W: rasqal-devel invalid-license LGPL or Apache Software License 2.0

This can be ignored as discussed above.
Comment 18 Kevin Fenzi 2006-12-03 18:03:49 EST
Hey Thomas. 

Have you had any chance to look at the issues from comment #17?
Comment 19 Thomas Vander Stichele 2006-12-13 05:37:26 EST
I can't update to a newer raptor:

gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m64 -mtune=generic -o .libs/roqet roqet.o 
../src/.libs/librasqal.so -L/usr/lib64 -lraptor 
roqet.o: In function `main':
/usr/src/rpm/BUILD/rasqal-0.9.13/utils/roqet.c:649: undefined reference to
`raptor_get_feature_count'
collect2: ld returned 1 exit status
make[1]: *** [roqet] Error 1


The extras version of rasqal is four releases behind, I assume updating that one
would allow me to build this one.

Pushing a new build with the fixed requires
Comment 21 Kevin Fenzi 2006-12-15 17:32:03 EST
Hey Thomas. 

Not sure I understand comment #19. You can't update this rasqal submission
because of raptor (already in extras) needs updating? Perhaps you could file a
bug to have the maintainer update that? It's not a showstopper though... 

Items 1, 3, and 4 look to be addressed in the package in comment #20. 

Do you have any comment on item #2: 

2. Are there some missing BuildRequires?
checking for pcre-config... no
checking for pcre-config... no
checking for pcre... not present
checking for xml2-config... no
checking for libxml2 library... no

Thats the last item I see that needs addressing... thanks for your patience. 
Comment 22 Thomas Vander Stichele 2006-12-19 06:17:47 EST
Updated package for buildrequires:

http://thomas.apestaart.org/download/pkg/fedora-6-x86_64-extras/rasqal-0.9.12-5.fc6/

wrt. 19, I get a build error when I build rasqal 0.9.13 as I pasted above. 
Looks to me like 0.9.13 of rasqal depends on a newer version of raptor than is
in extras.  The one in extras is four releases behind, so I am building one
release of rasqal behind since that would actually work today.

I have no direct need of pushing an update to raptor, but if the raptor
maintainer pushes a new build I will follow.
Comment 23 Kevin Fenzi 2006-12-19 14:05:58 EST
ok, the pcre and libxml2 buildrequires look good. 

I don't see any further blockers here, so this package is APPROVED. 

Don't forget to close this package NEXTRELEASE once it's been imported and built. 

Comment 24 Kevin Fenzi 2007-01-23 21:45:57 EST
Hey Thomas. 

Anything holding up importing and building this package?
Comment 25 Thomas Vander Stichele 2007-01-26 12:34:07 EST
nothing but xmas and lca :)

imported, requested branches, will push build
Comment 26 Thomas Vander Stichele 2007-01-27 06:03:57 EST
pushed, closing

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