Bug 447847 - Review Request: unbound - Validating, recursive, and caching DNS(SEC) resolver
Summary: Review Request: unbound - Validating, recursive, and caching DNS(SEC) resolver
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Adam Tkac
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-05-22 03:49 UTC by Paul Wouters
Modified: 2013-04-30 23:39 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-11-01 17:11:16 UTC
Type: ---
Embargoed:
atkac: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Paul Wouters 2008-05-22 03:49:30 UTC
Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: tp://ftp.xelerance.com/unbound/unbound-1.0.0-1.src.rpm
Description: Unbound is a validating, recursive, and caching DNS(SEC) resolver.

The C implementation of Unbound is developed and maintained by NLnet
Labs. It is based on ideas and algorithms taken from a java prototype
developed by Verisign labs, Nominet, Kirei and ep.net.

Unbound is designed as a set of modular components, so that also
DNSSEC (secure DNS) validation and stub-resolvers (that do not run
as a server, but are linked into an application) are easily possible.


output from rpmlint:

unbound.x86_64: E: non-standard-uid /var/unbound unbound
unbound.x86_64: E: non-standard-gid /var/unbound unbound
unbound.x86_64: E: non-standard-dir-perm /var/unbound 0700
unbound.x86_64: E: non-standard-uid /var/unbound/unbound.conf unbound
unbound.x86_64: E: non-standard-gid /var/unbound/unbound.conf unbound
unbound.x86_64: W: non-standard-dir-in-var unbound
unbound.x86_64: W: dangerous-command-in-%preun rm
unbound.x86_64: W: incoherent-subsys /etc/rc.d/init.d/unbound $prog

Currently talking to upstream about the problem of including a static ldns shipped with unbound, versus using the real standaline ldns.

The dangerous rm in %preun is the chroot /var/unbound getting cleaned up. The chroot() is currently somewhat variable, eg needing the real /etc/resolv.conf, which is manipulated in the init script, since hard links might not work across /etc/ and /var

I am not sure about the incoherent-subsys warning. It seems similar to other packages used. Using prog=$(basename $exec) did not help.

Comment 1 Paul Wouters 2008-05-22 16:36:49 UTC
ldns 1.3.x is required, but not yet released by upstream. So once that is
released and updated in fedora (I am the ldns maintainer), the static lib issue
will go away.

Comment 2 Paul Wouters 2008-05-28 20:47:19 UTC
ldns 1.3.0 (prerelease) addressed these problems, and the build now uses the
installed ldns properly. I've uploaded new rpm and spec file:

Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: ftp://ftp.xelerance.com/unbound/unbound-1.0.0-2.fc9.src.rpm 

ldns-1.3.0 should make it in the repo in the next day or so when upstream
releases it.

currently rpmlint says:
unbound.x86_64: E: non-standard-uid /var/unbound unbound
unbound.x86_64: E: non-standard-gid /var/unbound unbound
unbound.x86_64: E: non-standard-uid /var/unbound/unbound.conf unbound
unbound.x86_64: E: non-standard-gid /var/unbound/unbound.conf unbound
unbound.x86_64: W: non-standard-dir-in-var unbound
unbound.x86_64: W: dangerous-command-in-%preun rm
unbound.x86_64: W: incoherent-subsys /etc/rc.d/init.d/unbound $prog

I am not sure about the incoherent-subsys error.....


Comment 3 Paul Wouters 2008-07-16 18:36:01 UTC
updated to 1.0.1

Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: ftp://ftp.xelerance.com/unbound/unbound-1.0.1-1.fc9.src.rpm 



Comment 4 Adam Tkac 2008-08-11 13:41:30 UTC
It would be nice to have this software in Fedora. I'm going to take care about review.

Comment 5 Adam Tkac 2008-08-11 14:21:26 UTC
rpmlint output:
unbound.x86_64: W: non-standard-uid /var/unbound unbound
unbound.x86_64: W: non-standard-gid /var/unbound unbound
unbound.x86_64: W: non-standard-uid /var/unbound/unbound.conf unbound
unbound.x86_64: W: non-standard-gid /var/unbound/unbound.conf unbound
unbound.x86_64: W: non-standard-dir-in-var unbound
unbound.x86_64: W: incoherent-subsys /etc/rc.d/init.d/unbound $prog
^^ OK

unbound.x86_64: W: dangerous-command-in-%preun rm
Why do you have to call rm? I think rpm could handle remove of such directories automatically.

build process:
you could see something like

Depend daemon/daemon.c
Depend daemon/acl_list.c
Build services/listen_dnsport.c
Build services/localzone.c

Is it intentional that gcc parameters are not written to output? It doesn't look fine for me.

spec file:
is it really needed ship static libunbound.a library?

Comment 6 Paul Wouters 2008-09-16 21:10:59 UTC
Hi, I missed the emails telling me someone was reviewing this. Sorry.

I've updated unbound to version 1.0.2

Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: ftp://ftp.xelerance.com/unbound/unbound-1.0.2-1.fc9.src.rpm 

As for your comments:

- hiding the gcc command is what upstream does. It is not something we do.
- the rm command is used because unbound creates these files on startup.
  I will look into pre-creating these as part of the package, so they are
  removed when the package is uninstalled without using the rm command manually.
- the static library does not need shipping in the -devel package, i remove it in the next release

Comment 7 Paul Wouters 2008-09-16 21:11:58 UTC
I'll also update the user addition scheme to comply with the new packaging guidelines.

Comment 8 Ralf Corsepius 2008-09-18 06:38:03 UTC
(In reply to comment #6)
> - the static library does not need shipping in the -devel package, i remove it
> in the next release
MUSTFIX: There is shared libunbound, so you must not ship it as part of the -devel package. If you really feel you must ship static libs (feel strongly encouraged not to do so), move the lib*.a to a -static package.

MUSTFIX:
/var/unbound violates the FHS. Check the FHS for details, but this likely should be /var/lib/unbound.

Comment 9 Paul Wouters 2008-10-10 04:07:51 UTC
I've fixed up all issues and created a new release.

rmplint now only shows:

unbound.x86_64: W: non-standard-uid /var/run/unbound unbound
unbound.x86_64: W: non-standard-gid /var/run/unbound unbound

Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: ftp://ftp.xelerance.com/unbound/unbound-1.0.2-2.fc9.src.rpm
initscript:  ftp://ftp.xelerance.com/unbound/unbound.init

Comment 10 Adam Tkac 2008-10-17 11:22:31 UTC
source files match upstream: YES
package meets naming and versioning guidelines: YES
specfile is properly named, is cleanly written and uses macros consistently: YES
dist tag is present: YES
build root is correct: YES
license field matches the actual license: YES
license is open source-compatible: YES (BSD)
License text included in package: YES
latest version is being packaged: YES
BuildRequires are proper: YES

compiler flags are appropriate: NO
- look on http://people.redhat.com/atkac/unbound-build.patch. I think that patch should be included in main source. It shows build parameters

%clean is present: YES
package builds in mock (Rawhide/x86_64): YES
debuginfo package looks complete: YES
rpmlint is silent: OK (messages written below are fine)
unbound.x86_64: W: non-standard-uid /var/run/unbound unbound
unbound.x86_64: W: non-standard-gid /var/run/unbound unbound

final provides and requires look sane: OK
%check is present and all tests pass: OK (check is not present)
no shared libraries are added to the regular linker search paths: NO
- it seems that it is possible write "clients" for unbound. Due this reason I think the best will be create unbound-libs subpackage which will contain 
libunbound.so.0 and libunbound.so.0.14.0 libraries

owns the directories it creates: YES
doesn't own any directories it shouldn't: YES
no duplicates in %files: YES
file permissions are appropriate: VERIFY
- isn't better to make configuration files and chroot non-readable by "others"?

scriptlets: NO
- I think you should add unbound group as well. Look on 
https://fedoraproject.org/wiki/Packaging/UsersAndGroups

Please correct/explain problematic points written above.

Comment 11 Paul Wouters 2008-10-19 19:52:41 UTC
- I've changed the make command to run with QUIET=no, so it does not require a patch.
- the chroot contains no secret information, and is just there for security in case the unbound process would get compromised with a buffer overflow. Since there is nothing to hide, using world-readable seems appropriate.
- I've created a separate libs package as you recommended
- I've added the unbound group

Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: ftp://ftp.xelerance.com/unbound/unbound-1.0.2-3.fc9.src.rpm
initscript:  ftp://ftp.xelerance.com/unbound/unbound.init

Comment 12 Adam Tkac 2008-10-21 15:03:17 UTC
All problems were fixed except one - compiler flags:

http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags

"Standard" approach

%build
export CFLAGS=$RPM_OPT_FLAGS
./configure

doesn't pass CFLAGS to gcc, I don't know why. But something like

sed -i 's/CFLAGS=@CFLAGS@/CFLAGS=@CFLAGS@ '"$RPM_OPT_FLAGS"'/' Makefile.in

works.

I recommend put %configure command to build section (but it isn't needed). Otherwise all looks fine for me.

Comment 13 Paul Wouters 2008-10-21 15:40:40 UTC
Ahh, i see. I used:

%{__make} CFLAGS="$RPM_OPT_FLAGS" QUIET=no %{?_smp_mflags}

Which seems to work, but causes a previous warning to be an error now. So I'll have to make a patch :)

Comment 14 Mamoru TASAKA 2008-10-21 15:42:15 UTC
Just for CFLAGS issue

(In reply to comment #12)
> %build
> export CFLAGS=$RPM_OPT_FLAGS
> ./configure
> 
> doesn't pass CFLAGS to gcc, I don't know why. 

Because configure.ac contains:
---------------------------------------------------------------
    35  
    36  CFLAGS=
    37  AC_AIX
---------------------------------------------------------------
The line 36 is the culprit.
---------------------------------------------------------------
sed -i.cflags -e '/^CFLAGS=$/d' configure
---------------------------------------------------------------
before calling %configure will save CFLAGS.

Comment 15 Paul Wouters 2008-10-21 16:08:55 UTC
Ahh, we lost -D_GNU_SOURCE. I added this to the make line, since it seems it is the only part of CFLAGS that we lose.

I also disabled interface-automatic, as this caused unbound to listen on 0.0.0.0 and ::0 instead of just localhost.

Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: ftp://ftp.xelerance.com/unbound/unbound-1.0.2-4.fc9.src.rpm
initscript:  ftp://ftp.xelerance.com/unbound/unbound.init

Comment 16 Paul Wouters 2008-10-22 16:46:01 UTC
Spec URL: ftp://ftp.xelerance.com/unbound/unbound.spec
SRPM URL: ftp://ftp.xelerance.com/unbound/unbound-1.0.2-5.fc9.src.rpm
initscript:  ftp://ftp.xelerance.com/unbound/unbound.init

* Wed Oct 22 2008 Paul Wouters <paul> - 1.0.2-5
- Only call ldconfig in -libs package
- Move configure into build section
- devel subpackage should only depend on libs subpackage

Comment 17 Adam Tkac 2008-10-29 11:02:40 UTC
All problems written in previous comments are fixed (especially from comment #10 which contains detail review steps). Reviewed

Comment 18 Paul Wouters 2008-10-31 13:44:25 UTC
New Package CVS Request
=======================
Package Name: unbound
Short Description: Validating, recursive, and caching DNS(SEC) resolver
Owners: pwouters
Branches: F-10 F-9 EL-5
InitialCC:

Comment 19 Kevin Fenzi 2008-10-31 16:58:59 UTC
cvs done.


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