[bug#52578] [PATCH] updating openldap and adding service definition

[Maxime Devos]

Hi,

>+        "--disable-static"
>+        "--enable-shared"
>+        "--with-tls=openssl"
>+        "--disable-static"

A single "--disable-static" should be suficient.

> +        ,@(if (%current-target-system)
> +          '("--with-yielding_select=yes"
"ac_cv_func_memcmp_working=yes")
> +          '()
> +        )

is this speculation on what's necessary for cross-compilation, or has
it been determined these flags are necessary?

>+      #:make-flags '("STRIP=")

Why?

>+ #:parallel-build? #t

This is the default, no need to mention it.

> +        ,@(if (%current-target-system)
> +            '(
> +              (add-before 'make-depend 'fix-cross-gcc
> +                (lambda* (#:key target #:allow-other-keys)
> +                  (setenv "CC" (string-append target "-gcc"))
> +                  #t
> +                )
> +              )
> +            )
> +            '()

You can use ,(cc-for-target) here. Also, CC can be set in #:make-flags.

> +    (synopsis "Implementation of the Lightweight Directory Access
Protocol")
> +    (description "OpenLDAP is a free implementation of the
Lightweight Directory Access Protocol.")

That's a very terse description --- is it a server, a client
application, programming APIs for communicating with a server, or all
of these? Also, no need to mention it's free, everything in Guix is
free.

> +(define-public openldap-2.5.9
> + (package
> +   (inherit openldap)

What's the reason for defining multiple versions of openldap?
Usually, it is only necessary to keep the latest version of a package
(with some rare exceptions).

>+(define-module (gnu services openldap)

A copyright + license header is missing, and this file needs to be
added to Makefile.am (or local.mk, I'm not sure about the details).

>+  #:use-module (gnu packages openldap)
>+  #:use-module (gnu services)
>+  #:use-module (gnu services shepherd)
>+  #:use-module (guix)
>+  #:use-module (guix records)
>+  #:use-module (ice-9 match)
>+  #: export (

This seems unlikely to compile, what's the space doing here?

Something I'm missing here, is some documentation. As it is, this
openldap service isn't documented anywhere, so nobody would figure out
it even exists, unless they search in the source code.

> +        (shepherd-service [...])

As-is, this service would be run as root, which is very suboptimal from
a security perspective. Consider running it as a separate user & group,
and if feasible in a container (the latter is optional but would be
great).

> +  (pid-file openldap-configuration-pid-file
> +    (default "/var/run/openldap/slapd.pid"))
> +  (log-file openldap-configuration-log-file
> +    (default "/var/log/slapd.log"))

I don't see the point in making this customisable.
Why would anyone want to change the log locations or location of the
pid file? Unless there's some compelling reason otherwise, I'd prefer
to keep complexity down by not making this configurable.

> +  (config-file openldap-configuration-config-file
> +    (default (file-append openldap "/etc/openldap/slapd.conf"))
> +  )

Allowing writing the configuration with configuration records would be
preferred (with an 'extra-content'-style escape hatch, because it would
probably be infeasible to support every single configuration option of
openldap, but some basic options like ‘which network port to bind to’
should be configurable in Scheme).

> +          (requirement '(user-processes))

This service probably requires a network interface, so loopback might
be required. Also, why is user-processes included? I know many services
include it, but it doesn't appear to be documented anywhere when user-
processes must be added to 'requirement'.

>+    openldap-configuration
>+    openldap-configuration?
>+    openldap-shepherd-service
>+    openldap-service-type
>+  )

These parentheses are lonely, consider moving the parenthese to right
after openldap-service-type, to keep the style consistent in Guix.

Greetings,
Maxime.