Bug 228555

Summary: Review Request: fedora-ds-base - Fedora Directory Server
Product: [Fedora] Fedora Reporter: Rich Megginson <rmeggins>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: dennis, dyoung, fitzsim, jarod, jeff, jmccann, k.georgiou, overholt, rvokal
Target Milestone: ---Flags: jeff: fedora-review+
dennis: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-03-22 12:20:59 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:

Comment 1 Rob Crittenden 2007-02-13 21:35:36 UTC
At first glance it is strange that lm_sensors-devel is required. This is needed
for the SNMP agent per the output of net-snmp-config --agent-libs.

Perhaps a comment should be added to this effect to avoid further head scratching.

Comment 2 Rich Megginson 2007-02-13 21:43:53 UTC
(In reply to comment #1)
> At first glance it is strange that lm_sensors-devel is required. This is needed
> for the SNMP agent per the output of net-snmp-config --agent-libs.

There is some header file pulled in by compiling the snmp code/agent in fedora
ds that requires a header file provided by lm_sensors-devel.

> Perhaps a comment should be added to this effect to avoid further head scratching.

Where?



Comment 3 Rob Crittenden 2007-02-13 21:52:15 UTC
I stand corrected, there IS a comment in the spec to this effect. I missed it in
the ifarch. Ignore me.

Comment 4 Ville Skyttä 2007-02-13 21:56:22 UTC
(In reply to comment #1)
> At first glance it is strange that lm_sensors-devel is required. This is needed
> for the SNMP agent per the output of net-snmp-config --agent-libs.

Sounds like something that should be fixed in net-snmp-devel instead of
propagating kludges to dependent packages, by adding "Requires:
lm_sensors-devel" to it on appropriate architectures.

Comment 5 Jeffrey C. Ollie 2007-02-14 15:48:11 UTC
* source files match upstream - no differences found when comparing included
tarball with tarball generated by included script
! package meets naming and packaging guidelines.

  See below.

* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (fc7 i386, fc6 i386).
* package installs properly. 
! rpmlint says:

W: fedora-ds invalid-license GPL plus extensions
W: fedora-ds-devel invalid-license GPL plus extensions
W: fedora-ds-debuginfo invalid-license GPL plus extensions
W: fedora-ds invalid-license GPL plus extensions

Perhaps should use just GPL as the license tag?

E: fedora-ds dir-or-file-in-tmp /var/tmp/fedora-ds

Is this directory really needed?

W: fedora-ds log-files-without-logrotate /var/log/fedora-ds

Is there something built into the directory server to rotate log files?

E: fedora-ds-devel only-non-binary-in-usr-lib
W: fedora-ds-devel dangling-relative-symlink /usr/lib/fedora-ds/libns-dshttpd.so
libns-dshttpd.so.0.0.0
W: fedora-ds-devel dangling-relative-symlink /usr/lib/fedora-ds/libslapd.so
libslapd.so.0.0.0
W: fedora-ds-devel dangling-relative-symlink /usr/lib/fedora-ds/libds_admin.so
libds_admin.so.0.0.0

I think that these can be ignored, rpmlint doesn't seem to handle this case
appropriately

* %check is not present; There is no test code in the districution.
* shared libraries are present, ldconfig called in %post & %postun
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* header files are in -devel package
* unversioned .so files in -devel package
* no pkconfig file
* no libtool .la droppings.

Other notes:

* "-p" should be used to preserve timestamps when installing
  slapi-plugin.h

* Include a comment near the "Source0" line that indicates that the
  "fedora-ds-cvs.sh" script should be used to generate the tarball.

* Release should be 0.1.%{cvsdate}%{?dist}


Comment 6 Dennis Gilmore 2007-02-14 15:58:26 UTC
(In reply to comment #5)
> * source files match upstream - no differences found when comparing included
> tarball with tarball generated by included script
> ! package meets naming and packaging guidelines.

> Perhaps should use just GPL as the license tag?
I was told that its fine as thats what it is 

> E: fedora-ds dir-or-file-in-tmp /var/tmp/fedora-ds
> 
> Is this directory really needed?
per instance run time files go inside that dir 

> W: fedora-ds log-files-without-logrotate /var/log/fedora-ds
> 
> Is there something built into the directory server to rotate log files?
FDS handles its own log rotation 


> I think that these can be ignored, rpmlint doesn't seem to handle this case
> appropriately
I agree
 
 
> Other notes:
> 
> * "-p" should be used to preserve timestamps when installing
>   slapi-plugin.h
I agree

> * Include a comment near the "Source0" line that indicates that the
>   "fedora-ds-cvs.sh" script should be used to generate the tarball.
> 
> * Release should be 0.1.%{cvsdate}%{?dist}
> 
Guidelines dont mention where the disttag should go in this case.  I think its 
fine as is.

Comment 7 Paul Howarth 2007-02-14 16:39:55 UTC
(In reply to comment #6)
> > * Release should be 0.1.%{cvsdate}%{?dist}
> > 
> Guidelines dont mention where the disttag should go in this case.  I think its 
> fine as is.

The current release tag of 0.1.%{?dist}%{?cvsdate} will expand to
0.1..fc720070213 if build in the Extras buildsystem, since the expansion of
%{?dist} includes a leading dot.

The DistTag page (http://fedoraproject.org/wiki/Packaging/DistTag), which is
referenced from the package naming guidelines page
(http://fedoraproject.org/wiki/Packaging/NamingGuidelines) says:

  "Basically, follow the Packaging/NamingGuidelines for how to set the
   value for Release, then append %{?dist} to the end."

So I think that the guidelines do say where to put the dist tag and Comment #5
is right.

Comment 8 Rich Megginson 2007-02-14 18:31:08 UTC
(In reply to comment #5)
> W: fedora-ds invalid-license GPL plus extensions
> W: fedora-ds-devel invalid-license GPL plus extensions
> W: fedora-ds-debuginfo invalid-license GPL plus extensions
> W: fedora-ds invalid-license GPL plus extensions
> 
> Perhaps should use just GPL as the license tag?

I don't want to give people the impression that the license is just plain old
GPL.  The extension is important for those people who want to write and
distribute plugins with the directory server.  So if there is some other way to
indicate that, I'm open to suggestions.

> * %check is not present; There is no test code in the districution.

We're still working on that, being able to open source our test suites.  Is it
necessary to have something for %check?

Comment 9 Jeffrey C. Ollie 2007-02-14 18:51:27 UTC
(In reply to comment #8)
> (In reply to comment #5)
> > * %check is not present; There is no test code in the districution.
> 
> We're still working on that, being able to open source our test suites.  Is it
> necessary to have something for %check?

If there's no test code available, then there's no need for a check.
It's not the job of a packager to write test code.  However, if there
was test code, then yes, I'd block the package if there wasn't a
%check section.


Comment 10 Rich Megginson 2007-02-14 19:30:41 UTC
> W: fedora-ds log-files-without-logrotate /var/log/fedora-ds
> 
> Is there something built into the directory server to rotate log files?

Fedora Directory Server does not use syslog, in case you were wondering.  That
is also on our to-do list.

I've addressed all of the concerns (except for GPL vs. GPL + exception) and
posted new files, same links as above:
SRPM URL:
http://directory.fedora.redhat.com/sources/fds110a1/fedora-ds-1.1.0-0.1.20070213.src.rpm
Source URL:
http://directory.fedora.redhat.com/sources/fds110a1/fedora-ds-1.1.0-20070213.tar.bz2
Spec URL: http://directory.fedora.redhat.com/sources/fds110a1/fedora-ds.spec

Other sources: http://directory.fedora.redhat.com/sources/fds110a1/


Comment 11 Jeffrey C. Ollie 2007-02-14 20:05:26 UTC
OK, based upon the new SRPM and some feedback from the extras list about the
license, I'm approving the package.


Comment 12 Rich Megginson 2007-02-14 20:17:39 UTC
Dennis found a typo in the spec file - should have been install -p -m 644, not
install -m -p 644.  This has been fixed and uploaded to the same links as above.

Comment 13 Dennis Gilmore 2007-02-22 00:19:26 UTC
as this caused a problem using the old fedora-ds package provided by Directory 
Server Team and it only provides the base ldap server functionality.  none of 
the admin tools It has been renamed in fedora cvs to fedora-ds-base  with the 
intention when there is a full replacement in fedora we can have a fedora-ds 
metapackage that provides the full user experience

Comment 14 Christian Iseli 2007-06-05 16:29:34 UTC
Make Summary parser happy.