Bug 187430 - Review Request: elektra
Review Request: elektra
Status: CLOSED DUPLICATE of bug 209906
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-30 16:00 EST by Avi Alkalay
Modified: 2010-11-02 12:47 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-02 12:50:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Spec update based on Joost comments (22.92 KB, application/octet-stream)
2006-03-31 12:11 EST, Avi Alkalay
no flags Details
Spec file revised. (23.13 KB, text/plain)
2006-04-04 18:04 EDT, Avi Alkalay
no flags Details
Another spec file revision (23.04 KB, text/plain)
2006-04-14 18:39 EDT, Avi Alkalay
no flags Details
Another revision based on last comments (22.43 KB, application/octet-stream)
2006-04-15 11:57 EDT, Avi Alkalay
no flags Details

  None (edit)
Description Avi Alkalay 2006-03-30 16:00:31 EST
Spec Name or Url: http://sourceforge.net/project/showfiles.php?group_id=117521&package_id=127957&release_id=406035

SRPM Name or Url: http://sourceforge.net/project/showfiles.php?group_id=117521&package_id=127957&release_id=406035

Summary:
A key/value pair database to store software configurations

Description:
Elektra provides a universal and secure framework to store configuration
parameters in a hierarchical key-value pair mechanism, instead of each
program using its own text configuration files. This allows any program
to read and save its configuration with a consistent API, and allows
them to be aware of other applications' configurations, permitting
easy application integration. While architecturally similar to other OS
registries, Elektra does not have most of the problems found in those
implementations.

P.S.: This inclusion was requested in the last days in the fedora-devel list
Comment 1 Joost van der Sluis 2006-03-31 04:36:29 EST
You don't specify a spec-file. The SRPM that I found on your link doesn't seems
to be made especially for fedora-extras.

I coudn't find any mail from you on the FE-mailing list, if this is the first
time that you want to add a package to Fedora-Extras please read this:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines and then you also need
to request a sponsor.

I took a quick look on your spec-file and my first observations are that the
buildroot isn't correct and that the release-name doesn't match the packaging
naming guidelines. And I missed the changelog.
Comment 2 Avi Alkalay 2006-03-31 12:11:04 EST
Created attachment 127135 [details]
Spec update based on Joost comments

Check this updated spec file, please.
Comment 3 Joost van der Sluis 2006-03-31 14:51:04 EST
It doesn't build for me. I removed the debug-package comment, and fixed a type
on a date in the changelog.

But with the changelog, I meant a changelog for the package. So for now, only a
'Initial build' would be enough, something like this:
----------
* Fri Mar 31 2006 Avi Alkalay <avi@unix.sh> 0.6.0-0.1
- Initial build.
----------

further, what is this part for?
----------
# A buggy version of rpmbuild
%define _sysconfdir /etc
----------

In the buildrequires section you have db4-devel. Are you sure that the berkeley
sub-package doesn't need db4 to work? And GConf2 and libxml?

And the filename in the buildrequires, where is that good for?

Further, you're using $RPM_BUILD_ROOT, please change those in %{buildroot}

And you must uncomment the rm -rf ... from the %clean section, iirc.

and I'm not sure, but I thought that calling /sbin/ldconfig wasn't necessary.

Please review your spec file once more yourself, and try to build it.
Comment 4 Avi Alkalay 2006-04-04 18:04:03 EDT
Created attachment 127316 [details]
Spec file revised.

Second revision of the spec file. Please check.
Comment 5 Avi Alkalay 2006-04-14 08:10:21 EDT
Is there anything missing here to go ahead with this approval ? 
Comment 6 Patrice Dumas 2006-04-14 08:50:00 EDT
The spec file is still in very bad shape with regard with fedora extras
packaging rules and standards. I'll make a few comments but I would
advise you to read the packaging guildelines more thoroughly, and have
a look at similar packages by importing their cvs directories.
You could also have a look at the standard spec file template.

* don't use a specific server name (aleron) from sourceforge
* if libxml2 isn't picked automatically by rpm add a comment to 
  say so, and explain why
* the following seems to be a bad cut and paste
%package backend-berkeleydb
Summary:      Include files and API documentation for Elektra Project
* DTDVERSION isn't usefull
* You don't use macros like %{_bindir} and the like. This is mandatory
* The Setup for parallel builds part seems very strange. You should use
  what is advertized in the packaging guidelines.
* the %clean section is wrong
* %deffatr is not standard
* no need to add %docs for man pages
* No need of Backwards compatibility, from the Linux Registry days
* autoamke and autoconf and so on don't seem to be required
* the devel and main package aren't set up how they should be.
* Why not use %configure?

In fact the shape of the spec file seems to show that you haven't
read anything of the packaging guidelines/how to become a fedora 
contributor. It seems that you need to be sponsored, but in my 
opinion you should try to show that you are more willing to follow
the guidelines for fedora contributors and actively show your want
to be of fedora extras, and not only throw a package in the build 
system.

I'd be pleased to be wrong, however ;-)
Comment 7 Patrice Dumas 2006-04-14 08:53:52 EDT
Forgot to say that the changelog is much too long for an initial
submission, and it shouldn't be a duplicate for the upstream 
changelog but hold only what is related to packaging. You should
have a look at some examples.
Comment 8 Patrice Dumas 2006-04-14 09:06:06 EDT
(In reply to comment #3)
> But with the changelog, I meant a changelog for the package. So for now, only a
> 'Initial build' would be enough, something like this:
> ----------
> * Fri Mar 31 2006 Avi Alkalay <avi@unix.sh> 0.6.0-0.1
> - Initial build.
> ----------

Agreed.


> In the buildrequires section you have db4-devel. Are you sure that the berkeley
> sub-package doesn't need db4 to work? And GConf2 and libxml?

These should be picked up automatically by rpm.
 
> Further, you're using $RPM_BUILD_ROOT, please change those in %{buildroot}

It is not required to use one or the other form, as long as the use
of one or the other is consistent (and consistent with the optflags).

> and I'm not sure, but I thought that calling /sbin/ldconfig wasn't necessary.

As there are some dynamic libs installed, the ldconfig call is
necessary.
Comment 9 Avi Alkalay 2006-04-14 18:39:46 EDT
Created attachment 127766 [details]
Another spec file revision

> * don't use a specific server name (aleron) from sourceforge

Fixed.


> * if libxml2 isn't picked automatically by rpm add a comment to
>   say so, and explain why

Explicit dependency removed.


> * the following seems to be a bad cut and paste
> %package backend-berkeleydb
> Summary:	Include files and API documentation for Elektra Project

Fixed.


> * DTDVERSION isn't usefull

Its being used in a %post script. Kept.


> * You don't use macros like %{_bindir} and the like. This is mandatory

Thats because for correct usage of this package, it is mandatory to install
files in /bin and /lib (and not /usr/bin and /usr/lib) and according to
http://fedoraproject.org/wiki/Extras/RPMMacros there are no macros that satisfy
this. Any other places that macros are available, they are being used.


> * The Setup for parallel builds part seems very strange. You should use
>   what is advertized in the packaging guidelines.

Fixed.


> * the %clean section is wrong

Fixed.


> * %deffatr is not standard

I think it is fixed. Documentation isn't clear about the standard way of using
it.


> * no need to add %docs for man pages

Fixed.


> * No need of Backwards compatibility, from the Linux Registry days

Removed.


> * autoamke and autoconf and so on don't seem to be required

Removed.


> * the devel and main package aren't set up how they should be.

Not sure what you mean. They look good for me.


> * Why not use %configure?

Because of the same reason %{_bindir} is not being used. Red Hat's %configure
forces a /usr/bin and /usr/lib, which is wrong for this package.


> Forgot to say that the changelog is much too long for an initial
> submission, and it shouldn't be a duplicate for the upstream
> changelog but hold only what is related to packaging. You should
> have a look at some examples.

We don't have a smaller changelog because we are building RPMs since the
beginning. The packaging is tightly integrated to the build system and the
changelog is automatically being appended to the specfile. Can we just leave a
better, more complete changelog this way ?
Comment 10 Patrice Dumas 2006-04-14 20:06:22 EDT
(In reply to comment #9)

> > * DTDVERSION isn't usefull
> 
> Its being used in a %post script. Kept.

It is used only once, so it is better to replace by the
value.

> > * You don't use macros like %{_bindir} and the like. This is mandatory
> 
> Thats because for correct usage of this package, it is mandatory to install
> files in /bin and /lib (and not /usr/bin and /usr/lib) and according to
> http://fedoraproject.org/wiki/Extras/RPMMacros there are no macros that satisfy
> this. Any other places that macros are available, they are being used.

Ok. But %{_lib} should be used too, it is lib64 sometimes,
so /lib should be replaced by /%{_lib}

Also the macros should be used in %install too.

> > * the %clean section is wrong
> 
> Fixed.

No, the following line should be uncommented:
rm -rf $RPM_BUILD_ROOT
 
> > * %deffatr is not standard
> 
> I think it is fixed. Documentation isn't clear about the standard way of using
> it.

It is indeed fixed. 

> > * the devel and main package aren't set up how they should be.
> 
> Not sure what you mean. They look good for me.

the .so files should be in the devel package. It is usual to have,
in the main package something like
/lib/*elektra-filesys.so.*
and in -devel:
/lib/*elektra-filesys.so

Same for /lib/*berkeleydb.so*
Then devel should 
Requires:     elektra-backend-berkeleydb = %{version}-%{release}

> > * Why not use %configure?
> 
> Because of the same reason %{_bindir} is not being used. Red Hat's %configure
> forces a /usr/bin and /usr/lib, which is wrong for this package.

Couldn't it be possible to use
%configure
and override the default flags by adding them in the end?
Otherwise you'll have to set CFLAGS to $RPM_OPT_FLAGS.
 
> We don't have a smaller changelog because we are building RPMs since the
> beginning. The packaging is tightly integrated to the build system and the
> changelog is automatically being appended to the specfile. Can we just leave a
> better, more complete changelog this way ?

No. It is much too verbose. Leave only the packaging infos, not
everything. Sometime little is better...



* The Version is wrong, should be 0.6. The Source may be then
Source:       
http://dl.sourceforge.net/sourceforge/elektra/%{name}-%{version}.tar.gz

* libtool and gettext-devel are certainly not needed.

* ldconfig call is needed for backend-berkeleydb too

* rpmlint reports (among others)
E: elektra binary-or-shlib-defines-rpath /bin/kdb ['//lib']

* the %post action should only be done for the first install?
In that case look at how to achieve that in scriptlet snippets.

* The paragraph that appear in the elektra main package description
shouldn't appear in the other subpackages summaries.

* It is only an advice, and not a blocker, but I prefer
  listing files in bindir using the full names and not 
  globs, such that it is easier to catch mistakes.

* Also not a blocker, but I believe it would be clearer 
to have 
%{_datadir}/sgml/elektra-0.1.1/

* Some doc files are missing, like
%docs AUTHORS COPYING NEWS README 
Comment 11 Avi Alkalay 2006-04-15 11:57:29 EDT
Created attachment 127781 [details]
Another revision based on last comments

> > > * DTDVERSION isn't usefull
> It is used only once, so it is better to replace by the
> value.

We need a clear way to change it in the beginig of the spec.



> Ok. But %{_lib} should be used too, it is lib64 sometimes,
> so /lib should be replaced by /%{_lib}
> 
> Also the macros should be used in %install too.

Fixed.


> No, the following line should be uncommented:
> rm -rf $RPM_BUILD_ROOT

Fixed.


> the .so files should be in the devel package. It is usual to have,
> in the main package something like
> /lib/*elektra-filesys.so.*
> and in -devel:
> /lib/*elektra-filesys.so
> 
> Same for /lib/*berkeleydb.so*
> Then devel should
> Requires:	elektra-backend-berkeleydb = %{version}-%{release}

It is right this way. The -filesys.so and -berkeleydb.so have no development
versions.
They are specific implementations of the interfaces provided in the -devel
package. They have nothing to do in the -devel package.



> Couldn't it be possible to use
> %configure
> and override the default flags by adding them in the end?

Didn't worked since the begining. This is why we are doing a raw ./configure.
RPM and autoconf has little support for /bin and /lib-installable software. We
checked that in all Red Hat essential packages that get installed in /bin and
/lib: none of them use autoconf.

> Otherwise you'll have to set CFLAGS to $RPM_OPT_FLAGS.

Fixed.



> > We don't have a smaller changelog because we are building RPMs since the
> > beginning. The packaging is tightly integrated to the build system and the
> > changelog is automatically being appended to the specfile. Can we just
leave a
> > better, more complete changelog this way ?
> 
> No. It is much too verbose. Leave only the packaging infos, not
> everything. Sometime little is better...

In a perfect world, where all upstream software would be RPM packaged, their
changelog should be RPM's changelog, as we are already doing.

Is this a blocker ?


> * The Version is wrong, should be 0.6. The Source may be then
> Source:
> http://dl.sourceforge.net/sourceforge/elektra/%{name}-%{version}.tar.gz

The correct URL is
http://prdownloads.sourceforge.net/elektra/%{name}-%{version}.tar.gz
We re working with the development version, which is not in SF. Use this
instead:

http://avi.alkalay.net/software/elektra/



> * libtool and gettext-devel are certainly not needed.

Lets keep them please. I once spent 3 days trying to figure out what was
missing for autoconf in a newly installed system, while building a package. And
was precisely this packages. This works mostly as a documentation to who wants
to repackage it.



> * ldconfig call is needed for backend-berkeleydb too

Fixed.


> * rpmlint reports (among others)
> E: elektra binary-or-shlib-defines-rpath /bin/kdb ['//lib']

I have no idea what this means and how to fix it. After some investigation I
concluded this is an autoconf bug. As I said, autotools has little support for
/bin and /lib-installable software and this is a consequence of that.



> * the %post action should only be done for the first install?

Yes, it is correct this way.


> * The paragraph that appear in the elektra main package description
> shouldn't appear in the other subpackages summaries.

Fixed.


> * Some doc files are missing, like
> %docs AUTHORS COPYING NEWS README

Fixed.
Comment 12 Patrice Dumas 2006-04-15 19:33:30 EDT
(In reply to comment #11)
> Created an attachment (id=127781) [edit]

> > > > * DTDVERSION isn't usefull
> > It is used only once, so it is better to replace by the
> > value.
> 
> We need a clear way to change it in the beginig of the spec.

Why?

> > Ok. But %{_lib} should be used too, it is lib64 sometimes,
> > so /lib should be replaced by /%{_lib}
> > 
> > Also the macros should be used in %install too.
> 
> Fixed.

There is still one left:
mv $RPM_BUILD_ROOT/%{_lib}/libelektra.a $RPM_BUILD_ROOT/usr/lib

> > the .so files should be in the devel package. It is usual to have,
> > in the main package something like
> > /lib/*
> > Same for /lib/*berkeleydb.so*
> > Then devel should
> > Requires:	elektra-backend-berkeleydb = %{version}-%{release}
> 
> It is right this way. The -filesys.so and -berkeleydb.so have no development
> versions.
> They are specific implementations of the interfaces provided in the -devel
> package. They have nothing to do in the -devel package.

Ok. libelektra.so should be in a development package, however.

The backend are there to be dlopened, so I think that the libtool
call should be like
libelektra_berkeleydb_la_LDFLAGS = -avaoid-version -module
instead of
libelektra_berkeleydb_la_LDFLAGS = -version-info 2:0:0 -module

It would even be better to install the backend libraries in a specific
directory (say /lib/elektra-backends/, with the help of a backenddir
automake varialbe) and use lt_dlsetsearchpath.

Even if those ideas are not accepted upstream it would be nice to
have patches in the rpm that achieve it. Not a priority, though.

> > Couldn't it be possible to use
> > %configure
> > and override the default flags by adding them in the end?
> 
> Didn't worked since the begining. This is why we are doing a raw ./configure.
> RPM and autoconf has little support for /bin and /lib-installable software. We
> checked that in all Red Hat essential packages that get installed in /bin and
> /lib: none of them use autoconf.

coreutils does. In fact things are installed in /usr/bin and then moved
to /bin. RPM has support for things in /bin and /lib. for autoconf it is 
less easy. But see below, I propose something that works.

> In a perfect world, where all upstream software would be RPM packaged, their
> changelog should be RPM's changelog, as we are already doing.

That's debatable. The rpm changelog is what is visible from the
package users. They shouldn't have all the details about code.

> Is this a blocker ?

In my point of view, yes. But as I am not doing the formal review
the formal reviewer may have another point of view.

> The correct URL is
> http://prdownloads.sourceforge.net/elektra/%{name}-%{version}.tar.gz
> We re working with the development version, which is not in SF. Use this
> instead:
> 
> http://avi.alkalay.net/software/elektra/

No. We want to avoid using private source when public sources exist.
Indeed we cannot review all the sources so we rely on public sources
having been reviewed.
 
> > * libtool and gettext-devel are certainly not needed.
> 
> Lets keep them please. I once spent 3 days trying to figure out what was
> missing for autoconf in a newly installed system, while building a package. And
> was precisely this packages. This works mostly as a documentation to who wants
> to repackage it.

I am not opposed to having a comment like this one:

# to rebuild from cvs you need:
# automake autoconf libtool gettext-devel

But having them as explicit Buildrequires is blocking and
if you think more about it it is really unacceptable from the
rebuilding user point of view. Documentation shouldn't be code.
 
> > * ldconfig call is needed for backend-berkeleydb too
> 
> Fixed.

In fact it is not ;-). The backend-berkeleydb is dlopened so
no need for ldconfig call.

> > * rpmlint reports (among others)
> > E: elektra binary-or-shlib-defines-rpath /bin/kdb ['//lib']
> 
> I have no idea what this means and how to fix it. After some investigation I
> concluded this is an autoconf bug. As I said, autotools has little support for
> /bin and /lib-installable software and this is a consequence of that.

What about
%configure --libdir=/%{_lib} \
     --bindir=/bin \
     --disable-xmltest \
     --with-docbook=%{_datadir}/sgml/docbook/xsl-stylesheets

It fixes the rpath issue. 
 
> > * the %post action should only be done for the first install?
> 
> Yes, it is correct this way.

I don't really understand your answer, but my question was not a
well formulated question ;-). So here it is:

Should the following 2 commands be run only for the first 
installation?
kdb set -t dir system/sw
kdb set system/sw/kdb/schemapath
"%{_datadir}/sgml/elektra-%{DTDVERSION}/elektra.xsd"
Comment 13 Avi Alkalay 2006-04-16 06:35:20 EDT
(In reply to comment #12)
> > > > > * DTDVERSION isn't usefull
> Why?

OK. Hardcoded later.



> There is still one left:
> mv $RPM_BUILD_ROOT/%{_lib}/libelektra.a $RPM_BUILD_ROOT/usr/lib

Fixed.


> Ok. libelektra.so should be in a development package, however.

It is as /usr/lib/libelektra.a
I'm not sure I understand this. Are you saying that libelektra.so must be 
duplicated in the -devel and main package ?



> libelektra_berkeleydb_la_LDFLAGS = -avaoid-version -module

Thanks for this tip !



> It would even be better to install the backend libraries in a specific
> directory (say /lib/elektra-backends/, with the help of a backenddir
> automake varialbe) and use lt_dlsetsearchpath.

We'll think about it in next versions.


> > The correct URL is
> > http://prdownloads.sourceforge.net/elektra/%{name}-%{version}.tar.gz
> > We re working with the development version, which is not in SF. Use this
> > instead:
> > 
> > http://avi.alkalay.net/software/elektra/
> 
> No. We want to avoid using private source when public sources exist.
> Indeed we cannot review all the sources so we rely on public sources
> having been reviewed.

The correct URL is being used. I gave you my private one just for your test.
Please download the update for your new test again from the private URL.



> I am not opposed to having a comment like this one:
> 
> # to rebuild from cvs you need:
> # automake autoconf libtool gettext-devel

OK, they are just comments now.



> In fact it is not ;-). The backend-berkeleydb is dlopened so
> no need for ldconfig call.

Thats right. Removed.




> What about
> %configure --libdir=/%{_lib} \
>      --bindir=/bin \
>      --disable-xmltest \
>      --with-docbook=%{_datadir}/sgml/docbook/xsl-stylesheets

Thanks !



> > > * the %post action should only be done for the first install?
> > 
> > Yes, it is correct this way.
> 
> I don't really understand your answer, but my question was not a
> well formulated question ;-). So here it is:
> 
> Should the following 2 commands be run only for the first 
> installation?
> kdb set -t dir system/sw
> kdb set system/sw/kdb/schemapath
> "%{_datadir}/sgml/elektra-%{DTDVERSION}/elektra.xsd"

They must be executed on every install, upgrade, etc because we want to set 
the correct DTD version being installed. So there is no need to test if we are 
in a fresh installation or upgrade with a `if [ $1 -eq 0 ]; then`

Comment 14 Avi Alkalay 2006-04-18 08:37:06 EDT
Any other comment after last revision on http://avi.alkalay.net/software/elektra/ ??

Anything left to move on ?
Comment 15 Ralf Corsepius 2006-04-18 09:14:42 EDT
Blocker:
- Wrong doc dirs

Further personal remarks:

- The include file names are too generic to justfy installing them into
$(includedir)

- Way too many warnings to provide sufficient trust to allow it to be installed
into /lib

- IMO, installing DLLs to /lib is a fundamental design flaw. I refuse to approve
any package doing so. Use ordinary, properly versioned shared libs, instead of
trying to introduce DLL hell to the Linux bootsystem.
Comment 16 Patrice Dumas 2006-04-18 18:14:09 EDT
* the %configure call I proposed above seems better than what is in the
spec file, where redundant and unusefull options are set.

* call 
%setup -q
instead of
%setup

* libelektra.so must be in the devel package only. Similarly for
other libraries that aren't dlopened.

* Source: is wrong

* Still the %changelog issue
Comment 17 Patrice Dumas 2006-04-18 18:43:14 EDT
(In reply to comment #15)
> - The include file names are too generic to justfy installing them into
> $(includedir)

I do agree for kdb.h. Others may be acceptable. I believe the reviewer
might make a choice here.
 
> - Way too many warnings to provide sufficient trust to allow it to be installed
> into /lib

I don't think the issue is with trusted/untrusted. The issue is should
this package be needed before /usr is mounted. Currently as nothing 
requires elektra during the boot process the whole elektra stuff could
be in /usr. But it is a long term goal for elektra to be in such position,
so maybe it could be kept in /lib?

> - IMO, installing DLLs to /lib is a fundamental design flaw. I refuse to approve
> any package doing so. Use ordinary, properly versioned shared libs, instead of
> trying to introduce DLL hell to the Linux bootsystem.

The use of dlopened libraries for elektra backends may make sense.
There are other examples in fedora (pam, iptable, firefox plugins...).
What seems more dubious is to drop those dlopened files in /lib.
Especially since there exists a clean way to do that with libtool
(as I described above). This issue is the same if things go to /usr/lib
instead of /lib.
Comment 18 Patrice Dumas 2006-04-18 18:52:19 EDT
Is kdb really used during boot time? I think not. If not it should be
in /usr/bin.

kdb_static doesn't seems to be usefull to me.

Regarding the libs in /lib or /usr/lib, I think that it would be acceptable
to have dlopened libs in /lib/elektra/ and the main lib in /lib. However
as long as the dlopend libraries are installed uncleanly I think that 
everything should go in /usr/lib.

There are many warnings in the build logs when building with the fedora
options, they should be looked at carefully.
Comment 19 Ralf Corsepius 2006-04-18 22:59:53 EDT
(In reply to comment #17)
> (In reply to comment #15)

> > - Way too many warnings to provide sufficient trust to allow it to be installed
> > into /lib
> 
> I don't think the issue is with trusted/untrusted.
I guess, I can't avoid to more direct: The amount of warnings qualifies this
piece of SW as "not ready for public use".

I would be willing to ignore them for an arbirary standard library, but I am not
willing to ignore them for a package using DLLs, being involved into booting.

> > - IMO, installing DLLs to /lib is a fundamental design flaw. I refuse to approve
> > any package doing so. Use ordinary, properly versioned shared libs, instead of
> > trying to introduce DLL hell to the Linux bootsystem.
> 
> The use of dlopened libraries for elektra backends may make sense.
> There are other examples in fedora (pam, iptable, firefox plugins...).
Yes, these packages are also suffering from DLL hell. In particular, packaging
firefox/mozilla plugins is a PITA because of this.

But there still is a major difference between these packages and elektra:
They install their DLLs into /lib/<subdir> rsp. /usr/lib/<subdir>/plugins.
Comment 20 Patrice Dumas 2006-04-19 05:36:02 EDT
(In reply to comment #19)

> I guess, I can't avoid to more direct: The amount of warnings qualifies this
> piece of SW as "not ready for public use".
> 
> I would be willing to ignore them for an arbirary standard library, but I am not
> willing to ignore them for a package using DLLs, being involved into booting.

No software is forced to use elektra for booting. It will take years 
before stable elements of the booting process use elektra, if it ever
happens. However for new elements in boot process I think it is not 
problematic if they use an experimental piece of software - even if only
for testing. 

> Yes, these packages are also suffering from DLL hell. In particular, packaging
> firefox/mozilla plugins is a PITA because of this.

I am pretty unclear on the subject, but I can't see how this may be
avoided. For non dlopended modules the library must be present during 
linking, and that's what is avoided with dlopened modules. Do you mean
that dlopened modules should have a soname? But lt_dlopenext (and dlopen)
don't care about sonames? The only way I see to achieve it would be to 
hardcode the version in the module name.

So it seems to me that it would be misleading to have a version information
in the file name similar with what is done for classical shared libraries
when it is ignored.

The versioning still may be achieved at runtime, by searching for some 
symbols that would be present only for a given version, or use values stored 
in variables to make sure that the backend version uses a given api 
version. From a quick glance at the backend code this doesn't seems to 
be implemented in elektra. 

But I do agree with you that it won't solve the trouble for the packagers 
to make sure that, at package install time the abi/api is the right one.
What solution do you have, that allows the flexibility of dlopened modules
while allowing to have a abi/api check at package? I see one, it is to have 
the backend link against a specific libelektra library version. But it is 
rather ugly.

> But there still is a major difference between these packages and elektra:
> They install their DLLs into /lib/<subdir> rsp. /usr/lib/<subdir>/plugins.
 
That I agree completly with (as can be ssen in my other comments...). 
Comment 21 Michael A. Peters 2006-05-08 09:37:53 EDT
Suggestions:

in /etc/profile.d/elektraenv.sh :

FILE="/tmp/elektraenv${RANDOM}${RANDOM}"

should perhaps be

FILE="`mktemp -t elektraenv.XXXXXX`"

-=-

Most (all?) core packages that provide a .sh profile.d file also provide a .csh
equivalent. It might be a good idea to port the script to csh.

-=-

As mentioned above - the following are packaging mistakes.

in %files:
%doc %{_docdir}/%{name}

in %files devel:
%doc %{_docdir}/%{name}-devel

The Makefile should not install them into /usr/share (or they should be removed
after make install) and instead they should be packaged with %doc in this way:

%install
*stuff currently in %install, followed by*
rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/%{name}
rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/%{name}-devel
rm -f scripts/Makefile*
rm -rf examples/Makefile* examples/*.xml
mv doc/elektra-api/html ./api-html

in %files:
%doc scripts

in %files devel:
%doc examples api-html

That should result in those docs being properly packaged.
Comment 22 Avi Alkalay 2006-05-08 09:54:59 EDT
Nobody contributed a CSH script yet for profile.d and we don't know how to write
one. This is expected to appear soon.
We'll use mktemp instead of ${RANDOM}.

Doesn't this documentation packaging proccess makes the build system too much
dependent on RPM ?
We'd like to install documentation with that layout even on non-RPM platforms.

Comment 23 Ralf Corsepius 2006-05-08 10:17:06 EDT
(In reply to comment #22)
> Nobody contributed a CSH script yet for profile.d and we don't know how to
> write one.

Then I strongly suggest to learn about it:
yum install tcsh
 
> Doesn't this documentation packaging proccess makes the build system too much
> dependent on RPM ?
Not at all.
Comment 24 Avi Alkalay 2006-05-08 14:11:43 EDT
The new version is temporarily here:
http://avi.alkalay.net/software/elektra/elektra-0.6.1-30.src.rpm

All warnings were cleaned.
/bin/kdb and /lib/libelektra* were not moved to /usr because the nature of this
software is to be usable also by early boot stage programs.
All %doc-related suggestions were implemented.
Many other cleanups and suggestions were implemented.
Comment 25 Patrice Dumas 2006-05-17 14:28:26 EDT
blockers: 
* changelog 
* source not found
* .so for libelekra should be in a devel package

rpmlint output: 
W: elektra incoherent-version-in-changelog 0.6.1-3 0.6.1-30
E: elektra invalid-soname /lib/libelektra-filesys.so libelektra-filesys.so
E: elektra invalid-soname /lib/libelektra-fstab.so libelektra-fstab.so
W: elektra wrong-file-end-of-line-encoding
/usr/share/doc/elektra-0.6.1/standards/signature.xml
W: elektra devel-file-in-non-devel-package /usr/lib/libelektratools.so
W: elektra devel-file-in-non-devel-package /lib/libelektra.so
W: elektra devel-file-in-non-devel-package /lib/libelektra-default.so
E: elektra script-without-shellbang
/usr/share/doc/elektra-0.6.1/scripts/convert-hwconfKudzu


other comments:
* --prefix=%{_prefix} and --exec-prefix=/ were unneeded in my tests
* The redundant %doc line is not needed in devel subpackage:
%doc AUTHORS COPYING ChangeLog README INSTALL
Comment 26 Rudolf Kastl 2006-05-26 05:45:35 EDT
manpage conflict with allegro-devel

 LANG=C rpm -i /home/che/rpmbuild/RPMS/x86_64/elektra-devel-0.6.1-30.x86_64.rpm
        file /usr/share/man/man3/key.3.gz from install of elektra-devel-0.6.1-30
conflicts with file from package allegro-devel-4.2.0-12.fc5
Comment 27 Avi Alkalay 2006-05-27 08:37:54 EDT
Please try the new src.rpm from 
http://prdownloads.sourceforge.net/elektra/elektra-0.6.1-31.src.rpm

(In reply to comment #25)
> blockers: 
> * changelog 
> * source not found
> * .so for libelekra should be in a devel package

All three points fixed.


> rpmlint output: 
> W: elektra incoherent-version-in-changelog 0.6.1-3 0.6.1-30
> E: elektra invalid-soname /lib/libelektra-filesys.so libelektra-filesys.so
> E: elektra invalid-soname /lib/libelektra-fstab.so libelektra-fstab.so
> W: elektra wrong-file-end-of-line-encoding
> /usr/share/doc/elektra-0.6.1/standards/signature.xml
> W: elektra devel-file-in-non-devel-package /usr/lib/libelektratools.so
> W: elektra devel-file-in-non-devel-package /lib/libelektra.so
> W: elektra devel-file-in-non-devel-package /lib/libelektra-default.so
> E: elektra script-without-shellbang
> /usr/share/doc/elektra-0.6.1/scripts/convert-hwconfKudzu

All coherent messages fixed.


> other comments:
> * --prefix=%{_prefix} and --exec-prefix=/ were unneeded in my tests

This is correct. If removed, software won't be installed in /bin and /lib

> * The redundant %doc line is not needed in devel subpackage:
> %doc AUTHORS COPYING ChangeLog README INSTALL

Removed.


The key.3.gz conflict was also removed.
Comment 28 Patrice Dumas 2006-05-29 18:37:21 EDT
rpmlint output:

E: elektra configure-without-libdir-spec

I don't understand this one.

W: elektra devel-file-in-non-devel-package /lib/libelektra-default.so

Safe to ignore - as long as having dlopened libs in /lib is accepted,
as it is a link to a dlopened lib.

E: elektra script-without-shellbang
/usr/share/doc/elektra-0.6.1/scripts/convert-hwconfKudzu

The shellbang should be 
#!/usr/bin/perl
and if you want to avoid the dependency on perl, you should
chmod -x  

E: elektra-devel no-ldconfig-symlink /usr/lib/libelektratools.so
E: elektra-devel no-ldconfig-symlink /usr/lib/libelektra.so

This should be acted upon (those should be symlinks and not 
files).

E: elektra invalid-soname /lib/libelektra-filesys.so libelektra-filesys.so
E: elektra invalid-soname /lib/libelektra-fstab.so libelektra-fstab.so
E: elektra-backend-berkeleydb invalid-soname /lib/libelektra-berkeleydb.so
libelektra-berkeleydb.so

That's strange. They seems to me to be acceptable sonames for dlopened
libs. Maybe it is an error that happens when dlopened libs are put in 
standard library directories. As said in a comment above, putting 
those dlopened libs in /lib/elektra/ should be much better.

W: elektra-backend-berkeleydb no-documentation

Safe to ignore.

> > other comments:
> > * --prefix=%{_prefix} and --exec-prefix=/ were unneeded in my tests
> 
> This is correct. If removed, software won't be installed in /bin and /lib

I guess you wanted to say incorrect. But it is not incorrect, the
software is installed in /bin and /lib thanks to 
     --bindir=/bin \
     --libdir=/%{_lib} \

Have a look at the resulting variables in config.log.

> The key.3.gz conflict was also removed.

There is still /bin/kdb, kdb.{1,3}, kdb.h which are somehow generic 
but could be acceptable.
Comment 29 Patrice Dumas 2006-05-29 18:42:03 EDT
There aren't many different compile warnings, it would be nice to have
them fixed (upstream if possible).
Comment 30 Avi Alkalay 2006-05-30 00:00:03 EDT
Check update on 
http://prdownloads.sourceforge.net/elektra/elektra-0.6.1-32.src.rpm

The rpmlint output is almost empty now.

> E: elektra script-without-shellbang
> /usr/share/doc/elektra-0.6.1/scripts/convert-hwconfKudzu
> 
> The shellbang should be 
> #!/usr/bin/perl
> and if you want to avoid the dependency on perl, you should
> chmod -x  

Fixed. Thanks.



> E: elektra-devel no-ldconfig-symlink /usr/lib/libelektratools.so
> E: elektra-devel no-ldconfig-symlink /usr/lib/libelektra.so
> 
> This should be acted upon (those should be symlinks and not 
> files).

Fixed.
I did this on spec:
rm $RPM_BUILD_ROOT/%{_lib}/libelektra.so
ln -sf ../../%{_lib}/libelektra.so.2 $RPM_BUILD_ROOT/%{_libdir}/libelektra.so

Any suggestion for a cleaner way without hardcoding the lib version ?



> > > other comments:
> > > * --prefix=%{_prefix} and --exec-prefix=/ were unneeded in my tests
> > 
> > This is correct. If removed, software won't be installed in /bin and /lib
> 
> I guess you wanted to say incorrect. But it is not incorrect, the
> software is installed in /bin and /lib thanks to 
>      --bindir=/bin \
>      --libdir=/%{_lib} \

Fixed. Thanks.

Most warnings are due to some bug in the build system claiming "warning: 
implicit declaration of function 'usleep'". They are sort of fake, and will be 
fixed.
Comment 31 Patrice Dumas 2006-05-30 06:18:46 EDT
> I did this on spec:
> rm $RPM_BUILD_ROOT/%{_lib}/libelektra.so
> ln -sf ../../%{_lib}/libelektra.so.2 $RPM_BUILD_ROOT/%{_libdir}/libelektra.so
> 
> Any suggestion for a cleaner way without hardcoding the lib version ?

ln -sf ../../%{_lib}/libelektra.so.? $RPM_BUILD_ROOT/%{_libdir}/libelektra.so
  
> Most warnings are due to some bug in the build system claiming "warning: 
> implicit declaration of function 'usleep'". They are sort of fake, and will be 
> fixed.

Wha do you mean by "They are sort of fake"? I had a look at the code, and
indeed in keyset.c usleep is used although there is no
#include <unistd.h>
(maybe conditionalized on HAVE_UNISTD_H, with AC_CHECK_HEADERS(unistd.h))

As a side note, in case you weren't aware, in the usleep man page, 
there is:
   This function is obsolete. Use nanosleep(2) or setitimer(2) instead.



Anyway I don't have any other comments. I believe the package is in shape
now, so now you should look for a sponsor who accepts the dlopened libs 
in /lib and the header files directly in /usr/include... Or be prepared to 
fix those issues.
To look for a sponsor, the best is to show that you have enough knowledge
of the packaging guidelines to have CVS access granted to you, and 
the best for that is to participate in other packages reviews, by
comenting and sending patches for specfiles.
Comment 32 Patrice Dumas 2006-05-31 14:29:15 EDT
As it ships a .pc file, the elektra-devel package should 
Requires:   pkgconfig
Comment 33 Avi Alkalay 2006-06-02 08:31:34 EDT
Version 0.6.2 is available at:
http://sourceforge.net/project/showfiles.php?group_id=117521&package_id=127957

(In reply to comment #31)
> ln -sf ../../%{_lib}/libelektra.so.? 
$RPM_BUILD_ROOT/%{_libdir}/libelektra.so

Doesn't work. Globs don't work inside specs.


> Wha do you mean by "They are sort of fake"? I had a look at the code, and
> indeed in keyset.c usleep is used although there is no
> #include <unistd.h>
> (maybe conditionalized on HAVE_UNISTD_H, with AC_CHECK_HEADERS(unistd.h))

This is happening already. This bug is being fixed by our build system 
specialists.


> As a side note, in case you weren't aware, in the usleep man page, 
> there is:
>    This function is obsolete. Use nanosleep(2) or setitimer(2) instead.

Will be changed post-0.6.2, just released.


> who accepts the dlopened libs in /lib

Patrice Dumas accepted patch moved dlopened backends to /lib/elektra/


(In reply to comment #32)
> As it ships a .pc file, the elektra-devel package should 
> Requires:   pkgconfig

Dependency added.


How and where to ask for a sponsor ?
Comment 34 Patrice Dumas 2006-06-03 07:15:14 EDT
(In reply to comment #33)

There is still an issue, with kdb dlopening libelektratools. Indeed,
libelektratools.so is in elektra-devel, so kdb cannot dlopen
libelektratool. For example at install time

[root@patoune ~]# rpm -Uvh /home/dumas/RPM-fc/RPMS/i386/elektra-0.6.2-1.i386.rpm
Préparation...              ########################################### [100%]
   1:elektra                ########################################### [100%]
kdbLibLoad : libelektratools.so: cannot open shared object file: No such file or
directorykdb: XML importing and editing disabled
kdbLibLoad : libelektratools.so: cannot open shared object file: No such file or
directorykdb: XML importing and editing disabled

> How and where to ask for a sponsor ?

First of all make that bug block FE-NEEDSPONSOR
Ask on the fedora-extras-list for a sponsor pointing out your
comments on other people package reviews, participate in the
discussions on the fedora extras mailing lists, subscribe to the
cvs commit extras mailing list, watch the changes for the packages
you are interested in and comment on them when you have something to 
say, submit bug reports when you have found limitations in other
people packages.
Comment 35 Jason Tibbitts 2006-09-23 10:49:11 EDT
This has been in NEEDINFO for nearly two months now.  I will close this bug in
one week if there is no further response.
Comment 36 Patrice Dumas 2006-09-23 12:32:47 EDT
Most of the issues raised during the review have been 
fixed upstream in the new release (I believe so) except 
for the potential name clashes. So I think it shouldn't be 
closed for now.
Comment 37 Jason Tibbitts 2006-09-23 13:14:56 EDT
The lack of response by the package submitter is the problem.  Nothing upstream
does makes any difference as long as the person who submitted the package
doesn't respond to comments here.  The last message from the submitter was in
early June.
Comment 38 Jason Tibbitts 2006-10-02 12:50:12 EDT
And another week with no response.  Closing.  If upstream indeed has fixed the
issues and there is another maintainer willing to resubmit this package, they
should open a new tiket and mark this one as a duplicate of the new one.
Comment 39 Jason Tibbitts 2006-10-07 21:14:39 EDT

*** This bug has been marked as a duplicate of 209906 ***

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