Bug 228555
Summary: | Review Request: fedora-ds-base - Fedora Directory Server | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Rich Megginson <rmeggins> |
Component: | Package Review | Assignee: | 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: | rawhide | CC: | 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: |
Description
Rich Megginson
2007-02-13 19:23:53 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. (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? I stand corrected, there IS a comment in the spec to this effect. I missed it in the ifarch. Ignore me. (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. * 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} (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. (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. (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? (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. > 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/ OK, based upon the new SRPM and some feedback from the extras list about the license, I'm approving the package. 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. 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 Make Summary parser happy. |