Bug 187430
Summary: | Review Request: elektra | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Avi Alkalay <avibrazil> | ||||||||||
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> | ||||||||||
Status: | CLOSED DUPLICATE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||||||||
Severity: | medium | Docs Contact: | |||||||||||
Priority: | medium | ||||||||||||
Version: | rawhide | CC: | mpeters, pertusus, tomspur | ||||||||||
Target Milestone: | --- | ||||||||||||
Target Release: | --- | ||||||||||||
Hardware: | All | ||||||||||||
OS: | Linux | ||||||||||||
Whiteboard: | |||||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||||
Doc Text: | Story Points: | --- | |||||||||||
Clone Of: | Environment: | ||||||||||||
Last Closed: | 2006-10-02 16:50:12 UTC | Type: | --- | ||||||||||
Regression: | --- | Mount Type: | --- | ||||||||||
Documentation: | --- | CRM: | |||||||||||
Verified Versions: | Category: | --- | |||||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||||
Embargoed: | |||||||||||||
Attachments: |
|
Description
Avi Alkalay
2006-03-30 21:00:31 UTC
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. Created attachment 127135 [details]
Spec update based on Joost comments
Check this updated spec file, please.
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> 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. Created attachment 127316 [details]
Spec file revised.
Second revision of the spec file. Please check.
Is there anything missing here to go ahead with this approval ? 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 ;-) 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. (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> 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. 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 ? (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 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. (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" (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` Any other comment after last revision on http://avi.alkalay.net/software/elektra/ ?? Anything left to move on ? 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. * 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 (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. 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. (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. (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...). 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. 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. (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. 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. 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 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 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. 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. There aren't many different compile warnings, it would be nice to have them fixed (upstream if possible). 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. > 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. As it ships a .pc file, the elektra-devel package should Requires: pkgconfig 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 ? (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. This has been in NEEDINFO for nearly two months now. I will close this bug in one week if there is no further response. 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. 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. 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. *** This bug has been marked as a duplicate of 209906 *** |