Bug 235293 - (adminutil) Review Request: adminutil - Utility library for Fedora Directory Server administration
Review Request: adminutil - Utility library for Fedora Directory Server admin...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dennis Gilmore
Fedora Package Reviews List
:
Depends On: 240516
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-04 16:49 EDT by Rich Megginson
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-20 18:48:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dennis: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
diffs (2.22 KB, patch)
2007-05-22 11:11 EDT, Rich Megginson
no flags Details | Diff
spec file diffs (1.81 KB, patch)
2007-05-23 11:00 EDT, Rich Megginson
no flags Details | Diff
removed requires: icu (1.78 KB, patch)
2007-05-23 11:18 EDT, Rich Megginson
no flags Details | Diff
rpmlint for (installed) adminutil (4.20 KB, text/plain)
2007-05-23 11:43 EDT, Mamoru TASAKA
no flags Details
more diffs (8.06 KB, patch)
2007-05-23 16:51 EDT, Rich Megginson
no flags Details | Diff
diffs (7.57 KB, patch)
2007-05-24 16:43 EDT, Rich Megginson
no flags Details | Diff
mock build log for -3 (167.37 KB, text/plain)
2007-05-25 13:19 EDT, Mamoru TASAKA
no flags Details
cvs commit log (1.85 KB, text/plain)
2007-05-25 19:03 EDT, Rich Megginson
no flags Details

  None (edit)
Description Rich Megginson 2007-04-04 16:49:01 EDT
Spec URL: http://directory.fedoraproject.org/sources/adminutil.spec
SRPM URL: http://directory.fedoraproject.org/sources/adminutil-1.1.0-1.src.rpm
Description: adminutil is used to administer Fedora Directory Server, usually in conjunction with the admin server.  It is broken into two libraries - libadminutil contains the basic functionality, and libadmsslutil contains SSL versions and wrappers around the basic functions.  The PSET functions allow applications to store their preferences and configuration parameters in LDAP, without having to know anything about LDAP.  The configuration is cached in a
local file, allowing applications to function even if the LDAP server
is down.  The other code is typically used by CGI programs used for
directory server management, containing GET/POST processing code as
well as resource handling (ICU ures API).
Source tarball: http://directory.fedoraproject.org/sources/adminutil-1.1.0.tar.bz2
Comment 1 Dennis Gilmore 2007-04-07 13:28:31 EDT
missing BuildRequires: cyrus-sasl-devel 
Shared Objects are not versioned.

with correct BuildRequires builds in mock.  rpmlint is quiet 
Comment 2 Rich Megginson 2007-05-08 16:05:13 EDT
(In reply to comment #1)
> missing BuildRequires: cyrus-sasl-devel 

Ok.

> Shared Objects are not versioned.

I don't know what this means.  I'm using standard autotools with libtool.  Is
there some libtool flag I need to pass in?

> 
> with correct BuildRequires builds in mock.  rpmlint is quiet 

Comment 3 Rich Megginson 2007-05-08 16:19:08 EDT
(In reply to comment #2)

> > Shared Objects are not versioned.
> 
> I don't know what this means.  I'm using standard autotools with libtool.  Is
> there some libtool flag I need to pass in?

/me reads info libtool - the -version-info flag - is this what you mean?
Comment 4 Dennis Gilmore 2007-05-17 00:51:21 EDT
i think that what im refering to is  

`-version-number MAJOR[:MINOR[:REVISION]]'
     If OUTPUT-FILE is a libtool library, compute interface version
     information so that the resulting library uses the specified
     major, minor and revision numbers.  This is designed to permit
     libtool to be used with existing projects where identical version
     numbers are already used across operating systems.  New projects
     should use the `-version-info' flag instead.

the .so should be .so.1.1.0  not .so.0.0.0
Comment 5 Rich Megginson 2007-05-17 10:05:14 EDT
(In reply to comment #4)
> i think that what im refering to is  
> 
> `-version-number MAJOR[:MINOR[:REVISION]]'
...
>      numbers are already used across operating systems.  New projects
>      should use the `-version-info' flag instead.
> 
> the .so should be .so.1.1.0  not .so.0.0.0

The description says "New projects should use the -version-info flag instead." 
I see a real mix of usage in /usr/lib - some libs use -version-number, some use
-version-info, some are just .so with no version (or version in the name e.g.
libnspr4.so).  Is there a page on the fedoraproject wiki that describes library
naming and versioning conventions?
Comment 6 Adam Jackson 2007-05-17 14:27:13 EDT
The libtool doc is wrong.  The versioning scheme implemented by -version-info
not only doesn't match how ELF works, it doesn't match how _any_ extant
executable format works.  It's a fiction.

Use -version-number.
Comment 7 Rich Megginson 2007-05-17 16:28:45 EDT
New version, with several bug fixes and review issues addressed.

Spec URL: http://directory.fedoraproject.org/sources/adminutil.spec
SRPM URL: http://directory.fedoraproject.org/sources/adminutil-1.1.1-1.src.rpm
Source tarball: http://directory.fedoraproject.org/sources/adminutil-1.1.1.tar.bz2

Comment 8 Dennis Gilmore 2007-05-17 23:10:50 EDT
 source files match upstream:
3f3aedc553e9f4eb9c0a1e9e6e047bf4  adminutil-1.1.1.tar.bz2
3f3aedc553e9f4eb9c0a1e9e6e047bf4  ../SOURCES/adminutil-1.1.1.tar.bz2
 package meets naming and versioning guidelines.
 specfile is properly named, is cleanly written and uses macros consistently.
 dist tag is present.
 build root is correct.
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 license field matches the actual license.
 license is open source-compatible.  LGPL License text included in package.
 latest version is being packaged.
 compiler flags are appropriate.
 %clean is present.
 package builds in mock ( devel x86_64).
 package installs properly
 debuginfo package looks complete.
 rpmlint says
W: adminutil-devel no-documentation

 owns the directories it creates.
 doesn't own any directories it shouldn't.
 no duplicates in %files.
 file permissions are appropriate.
 no scriptlets present.
 code, not content.
 documentation is small, so no -docs subpackage is necessary.
 %docs are not necessary for the proper functioning of the package.
 no libtool .la droppings.
 not a GUI app.

Needs work:
 final provides and requires are sane:
all devel packages with  .pc packages need to Requires: pkgconfig

pkgconfig should not be BuildRequires it is provided by the devel packages 
that are BuildRequires and provide .pc files

i filed bug# 240516  
# cyrus-sasl-devel is required because mozldap uses it
BuildRequires:    cyrus-sasl-devel

mozldap-devel should require it if this is the case.

Fix these small issues and i will approve adminutil
Comment 10 Dennis Gilmore 2007-05-21 22:29:08 EDT
This needs to go now that you fixed mozldap-devel

# cyrus-sasl-devel is required because mozldap uses it
BuildRequires:    cyrus-sasl-devel

pkgconfig should not be a Requires of the main package but of the -devel 
package  no need to BR pkgconfig  
Comment 11 Rich Megginson 2007-05-22 11:11:29 EDT
Created attachment 155166 [details]
diffs

Does this look ok?
Comment 12 Dennis Gilmore 2007-05-22 23:47:17 EDT
Yes thats it.
Comment 14 Mamoru TASAKA 2007-05-23 10:29:45 EDT
* What are the lines of "BuildRequires" on -devel package?
* Is this package parellel make unsafe?
Comment 15 Rich Megginson 2007-05-23 10:35:07 EDT
(In reply to comment #14)
> * What are the lines of "BuildRequires" on -devel package?

They are the packages required to build the adminutil-devel package?

> * Is this package parellel make unsafe?

Not that I know of.  Why?
Comment 16 Mamoru TASAKA 2007-05-23 10:47:22 EDT
(In reply to comment #15)
> (In reply to comment #14)
> > * What are the lines of "BuildRequires" on -devel package?
> They are the packages required to build the adminutil-devel package?

There seems to be some confusion.
"BuildRequires" is the packages which are required to
rebuild this (adminutil) package and they should be
written at the part of the summary of main package (as you
wrote them). There is no need to write duplicate BuildRequires.

What is actually needed is to write "Requires" for -devel
package. For example, adminutil.pc.in contains the line:
-----------------------------------------------
Requires: nspr, nss, svrcore, mozldap, icu
-----------------------------------------------
This means -devel package should have "Requires" (not BuildRequires)
nspr-devel, nss-devel, ........
Check the section "Requires" of
http://fedoraproject.org/wiki/Packaging/Guidelines

> 
> > * Is this package parellel make unsafe?
> 
> Not that I know of.  Why?
Please refer to the section "Parallel make" of
http://fedoraproject.org/wiki/Packaging/Guidelines
Comment 17 Rich Megginson 2007-05-23 11:00:40 EDT
Created attachment 155254 [details]
spec file diffs

Do these diffs address the issues in comment #16?
Comment 18 Mamoru TASAKA 2007-05-23 11:15:55 EDT
I cannot check if your comment 17 is sufficient until
I can rebuild this package with mock successfully.

However, I have one question about if -devel package
should have "Requires: icu". Would you explain why
this is needed?
("Requires: icu" in pkgconfig .pc file is satisfied
  with libicu-devel)
Comment 19 Rich Megginson 2007-05-23 11:18:06 EDT
Created attachment 155258 [details]
removed requires: icu

Removed Requires: icu

Does this look ok?
Comment 20 Mamoru TASAKA 2007-05-23 11:43:03 EDT
Created attachment 155265 [details]
rpmlint for (installed) adminutil

Still not enough
* undefined non-weak symbols
  - libadmsslutil.so.1.1.1 contains undefined non-weak
    symbols. This is not allowed for packages to provide
    -devel subpackages because linkage against this
    library fails due to these symbols.

* Timestamps
  - -devel package contains many header files (i.e. text
    files which are not created or modified during rebuild)
    and keeping timestamps on these files are recommend.
    Please use:
---------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"
---------------------------------------------------

* macros consistency
  - Please determine to use macros or not to use macros
    for binaries. Please don't use both "rm" and "%{__rm}"
    If you want to use %{__rm}, please also use %{__make},
    for example.
Comment 21 Mamoru TASAKA 2007-05-23 11:51:48 EDT
One question:
Is the license of lib/libadminutil/acclanglist.c
compatible with LGPL (not GPL) ? What does
the following mean? (sorry, I am not legal specialist)
-------------------------------------------------
Non-GPL Code
permitted under this exception must only link to the code of this Program
through those well defined interfaces identified in the file named EXCEPTION
found in the source code files (the "Approved Interfaces").
--------------------------------------------------
Comment 22 Rich Megginson 2007-05-23 16:51:17 EDT
Created attachment 155295 [details]
more diffs
Comment 23 Rich Megginson 2007-05-23 16:53:15 EDT
(In reply to comment #20)
> Still not enough
> * undefined non-weak symbols
>   - libadmsslutil.so.1.1.1 contains undefined non-weak
>     symbols. This is not allowed for packages to provide
>     -devel subpackages because linkage against this
>     library fails due to these symbols.

Fixed - added linkage against libadminutil.la

> * Timestamps
>   - -devel package contains many header files (i.e. text
>     files which are not created or modified during rebuild)
>     and keeping timestamps on these files are recommend.
>     Please use:
> ---------------------------------------------------
> make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"
> ---------------------------------------------------

Done.

> * macros consistency
>   - Please determine to use macros or not to use macros
>     for binaries. Please don't use both "rm" and "%{__rm}"
>     If you want to use %{__rm}, please also use %{__make},
>     for example.

Done.
Comment 24 Rich Megginson 2007-05-23 16:54:33 EDT
(In reply to comment #21)
> One question:
> Is the license of lib/libadminutil/acclanglist.c
> compatible with LGPL (not GPL) ? What does
> the following mean? (sorry, I am not legal specialist)

Sorry.  That file was copied from Fedora Directory Server to adminutil.  I have
modified the license to use LGPL instead of the FDS license (GPL + exception). 
This is ok since we (Red Hat) control the copyright.
Comment 25 Mamoru TASAKA 2007-05-24 03:51:02 EDT
Well,

* Patching Makefile.am requires 
  - to call "autoreconf" in %build stage
  - to add "Requires: libtool"
  - And please write "Patch0: ..." and "%patch0 ....."

* For Requires problem of mozldap-devel against cyrus-sasl-devel,
  please see my comment (bug 240516 comment 2)
Comment 26 Rich Megginson 2007-05-24 10:23:34 EDT
(In reply to comment #25)
> Well,
> 
> * Patching Makefile.am requires 
>   - to call "autoreconf" in %build stage
>   - to add "Requires: libtool"
>   - And please write "Patch0: ..." and "%patch0 ....."

I will commit the changes upstream (I am the upstream maintainer), so no patch
required.  There is a shell script called autogen.sh which makes sure the system
has the correct version of the autotools before running autoreconf.

> 
> * For Requires problem of mozldap-devel against cyrus-sasl-devel,
>   please see my comment (bug 240516 comment 2)

Comment 27 Mamoru TASAKA 2007-05-24 11:52:38 EDT
(In reply to comment #26)
> (In reply to comment #25)
> > Well,
> > 
> > * Patching Makefile.am requires 
> >   - to call "autoreconf" in %build stage
> >   - to add "Requires: libtool"
> >   - And please write "Patch0: ..." and "%patch0 ....."
> 
> I will commit the changes upstream (I am the upstream maintainer), 
> so no patch
> required.  There is a shell script called autogen.sh which 
> makes sure the system
> has the correct version of the autotools before running autoreconf.

Okay. Then if you release new tarball (1.1.2), it is not a problem.

> > * For Requires problem of mozldap-devel against cyrus-sasl-devel,
> >   please see my comment (bug 240516 comment 2)
So please add "BuildRequires: cyrus-sasl-devel" to this package
(adminutil).
Comment 28 Rich Megginson 2007-05-24 12:04:47 EDT
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Well,
> > > 
> > > * Patching Makefile.am requires 
> > >   - to call "autoreconf" in %build stage
> > >   - to add "Requires: libtool"
> > >   - And please write "Patch0: ..." and "%patch0 ....."
> > 
> > I will commit the changes upstream (I am the upstream maintainer), 
> > so no patch
> > required.  There is a shell script called autogen.sh which 
> > makes sure the system
> > has the correct version of the autotools before running autoreconf.
> 
> Okay. Then if you release new tarball (1.1.2), it is not a problem.

Is it necessary to re-version from 1.1.1 to 1.1.2?  1.1.1 has not yet been
officially released.  I'd like to get all of these changes for fedora extras
into 1.1.1.

> > > * For Requires problem of mozldap-devel against cyrus-sasl-devel,
> > >   please see my comment (bug 240516 comment 2)
> So please add "BuildRequires: cyrus-sasl-devel" to this package
> (adminutil).

But I thought since adminutil does not use any cyrus-sasl include files nor link
directly to any cyrus-sasl libs, it doesn't need BuildRequires: cyrus-sasl-devel?
Comment 29 Mamoru TASAKA 2007-05-24 12:22:54 EDT
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (In reply to comment #25)

> > Okay. Then if you release new tarball (1.1.2), it is not a problem.
> 
> Is it necessary to re-version from 1.1.1 to 1.1.2?  1.1.1 has not yet been
> officially released.  I'd like to get all of these changes for fedora extras
> into 1.1.1.

I didn't know it...

> 
> > > > * For Requires problem of mozldap-devel against cyrus-sasl-devel,
> > > >   please see my comment (bug 240516 comment 2)
> > So please add "BuildRequires: cyrus-sasl-devel" to this package
> > (adminutil).
> 
> But I thought since adminutil does not use any 
> cyrus-sasl include files nor link
> directly to any cyrus-sasl libs, it doesn't need 
> BuildRequires: cyrus-sasl-devel?

Then please fix configure or the option of configre. 
Actually the lastest tarball I have locally checks the existence of
sasl.h and mock build fails without cyrus-sasl-devel.
-----------------------------------------------------
configure: checking for sasl...
checking for --with-sasl... no
checking for --with-sasl-inc... no
checking for --with-sasl-lib... no
checking for sasl.h... no
configure: error: sasl not found, specify with --with-sasl.
error: Bad exit status from /var/tmp/rpm-tmp.16648 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.16648 (%build)
---------------------------------------------------------

Comment 30 Rich Megginson 2007-05-24 14:53:13 EDT
(In reply to comment #29)
> > But I thought since adminutil does not use any 
> > cyrus-sasl include files nor link
> > directly to any cyrus-sasl libs, it doesn't need 
> > BuildRequires: cyrus-sasl-devel?
> 
> Then please fix configure or the option of configre. 
> Actually the lastest tarball I have locally checks the existence of
> sasl.h and mock build fails without cyrus-sasl-devel.
> -----------------------------------------------------
> configure: checking for sasl...
> checking for --with-sasl... no
> checking for --with-sasl-inc... no
> checking for --with-sasl-lib... no
> checking for sasl.h... no
> configure: error: sasl not found, specify with --with-sasl.
> error: Bad exit status from /var/tmp/rpm-tmp.16648 (%build)

Ok.  This is the problem.  The actual package with just the shared libs does not
need sasl to build, because it doesn't build an executable.  However, a
developer may build tests, and since the tests are executables, they must be
linked against sasl:

gcc -g -O2 -o .libs/psetread tests/psetread-psetread.o  ./.libs/libadmsslutil.so
/share/adminutil/built/.libs/libadminutil.so -L/usr/lib64/dirsec -L/usr/lib64
./.libs/libadminutil.so -lplc4 -lplds4 -lnspr4 -lssl3 -lnss3 -lsoftokn3
-lssldap60 -lldap60 -lprldap60 -lldif60 -licui18n -licuuc -licudata  -Wl,--rpath
-Wl,/home/rich/11srv/lib
/usr/lib64/libldap60.so: undefined reference to `sasl_client_step'
...
/usr/lib64/libldap60.so: undefined reference to `sasl_encode'
collect2: ld returned 1 exit status
make: *** [psetread] Error 1

So I suppose the real solution here will be to add another configure option e.g.
--enable-tests.  Then I'll have to see if there is some way I can conditionally
include the m4/sasl.m4 file in configure.ac.
Comment 31 Rich Megginson 2007-05-24 16:43:09 EDT
Created attachment 155392 [details]
diffs

Add --enable-tests configure option.  By default this is on.  If enable-tests
is on, then sasl will be used, otherwise, sasl will not be used.  However, for
rpm packaging, we don't care about the tests, so we use --disable-tests, which
skips the sasl stuff.
Comment 32 Mamoru TASAKA 2007-05-25 11:36:33 EDT
Well, I want to check if your patch in comment 31 works
on mockbuild, however it seems that all my domestic mirrors
(i.e. mirror servers in Japan) don't work well now...
Comment 33 Mamoru TASAKA 2007-05-25 13:19:30 EDT
Created attachment 155467 [details]
mock build log for -3

Okay, patch on comment 31 seems okay for me.
Comment 34 Rich Megginson 2007-05-25 19:03:40 EDT
Created attachment 155491 [details]
cvs commit log

New version, should be the final one

Spec URL: http://directory.fedoraproject.org/sources/adminutil.spec
SRPM URL: http://directory.fedoraproject.org/sources/adminutil-1.1.1-3.src.rpm
Source tarball:
http://directory.fedoraproject.org/sources/adminutil-1.1.1.tar.bz2

I'm going to be out next week.	Margaret, if this is approved, can you do the
cvs-import and create the branches?  Or someone else?  Otherwise, it will have
to wait until the 5th at the earliest.
Comment 35 Mamoru TASAKA 2007-06-08 01:57:33 EDT
Dennis, ping?
Comment 36 Mamoru TASAKA 2007-06-17 11:25:04 EDT
I will unset the reviewer of this bug if no response is received from
the current submitter within ONE WEEK.
Comment 37 Rich Megginson 2007-06-18 09:31:33 EDT
By submitter do you mean me?  Is there any other information you need from me?
Comment 38 Mamoru TASAKA 2007-06-18 09:42:25 EDT
(In reply to comment #37)
> By submitter do you mean me?  Is there any other information you need from me?

s|submitter|reviewer|, sorry...
Again:

I will unset the reviewer of this bug if no response is received from
the current reviewer within 6 days.
Comment 39 Dennis Gilmore 2007-06-20 09:09:05 EDT
Builds in mock.

Looks good to me Approved

Mamoru thanks for your input
Comment 40 Rich Megginson 2007-06-20 16:24:14 EDT
New Package CVS Request
=======================
Package Name: adminutil
Short Description: Utility library for Fedora Directory Server administration
Owners: rmeggins@redhat.com,mlum@redhat.com
Branches: FC-5 FC-6 F-7 devel
InitialCC:

Note that if no new branches are being created for FC-5, that's fine.
Comment 41 Kevin Fenzi 2007-06-20 17:03:06 EDT
cvs done. Yes, no new FC-5 branches, I did the rest. 
Comment 42 Rich Megginson 2007-06-20 18:00:06 EDT
Thanks.  I'm following these instructions:
http://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess

I did the cvs add, commit, and make tag in FC-6, F-7, and devel.  I did a make
build in FC-6 and it succeeded.  However, F-7 and devel fail to build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=44132
BuildError: package adminutil not in list for tag dist-f8
http://koji.fedoraproject.org/koji/taskinfo?taskID=44119
BuildError: package adminutil not in list for tag dist-fc7-updates-candidate
Comment 43 Rich Megginson 2007-06-20 18:48:51 EDT
Woohoo!  Built successfully on all branches!  Thanks to everyone for the help.

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