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

[zimoun]

Hi Jean-François,

Nice to see you here. :-)

Various comments for improving the submission.

On Fri, 17 Dec 2021 at 14:52, Jean-Francois GUILLAUME 
<Jean-Francois.Guillaume@univ-nantes.fr> wrote:
> * gnu/packages/openldap.scm (openldap): Update to 2.6.0, adding 2.5.7, 
> 2.5.8, 2.5.9
> * gnu/services/openldap.scm (openldap): Adding slapd service

I would split: one commit for adding a big openldap and another for
adding the service.  WDYT?

(I have not looked yet to the service.)


>   (define-public openldap
> +  (package
> +    (name "openldap")
> +    (version "2.6.0")
> +    (source (origin
> +      (method url-fetch)
> +      (uri (list
> +        (string-append 
> "https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-";
>  
> version ".tgz")

Why the mirror list had been removed?


> +        (string-append 
> "http://repository.linagora.org/OpenLDAP/openldap-release/openldap-"; 
> version ".tgz")

This is new, right?


> +        (string-append 
> "ftp://ftp.dti.ad.jp/pub/net/OpenLDAP/openldap-release/openldap-"; 
> version ".tgz")

As it is currently and already done in gnu/packages/openldap.scm, to
ease the reading, this long string could be slip as,

--8<---------------cut here---------------start------------->8---
                        (string-append
                         "ftp://ftp.dti.ad.jp/pub/net/OpenLDAP/";
                         "openldap-release/openldap-" version ".tgz")))
--8<---------------cut here---------------end--------------->8---

(See below for details if many variants are required.)


> +    (inputs `(
> +      ("argon2", argon2)
> +      ("cyrus-sasl", cyrus-sasl)
> +      ("libevent", libevent)
> +      ("libgcrypt", libgcrypt)
> +      ("libltdl", libltdl)
> +      ("lz4", lz4)
> +      ("openssl", openssl)
> +      ("perl", perl)
> +      ("snappy", snappy)
> +      ("unixodbc", unixodbc)
> +      ("wiredtiger", wiredtiger)
> +      ("zlib", zlib)
> +    ))
> +    (native-inputs `(
> +      ("bdb", bdb)
> +      ("groff", groff)
> +      ("libtool", libtool)
> +      ("pkg-config", pkg-config)
> +    ))

Currently, openldap@2.4.57 is built using (reformatted by me to ease the
comparison):

--8<---------------cut here---------------start------------->8---
   (inputs (list bdb-5.3 
                 cyrus-sasl 
                 gnutls 
                 libgcrypt 
                 zlib))
   (native-inputs (list libtool 
                        groff 
                        bdb-5.3))
--8<---------------cut here---------------end--------------->8---

Aside the new style vs the old style which is a detail, are these lists
expanded because the version bump or because more OpenLDAP is built
using more features?


> +    (arguments `(
> +      ; this is needed because the make check does not work inside guix
> +      #:tests? #f

It was already off, but I do not understand the new comment.  Well,
maybe this commentary is not necessary.


> +      #:configure-flags '(
> +        "--enable-debug"
> +        "--enable-dynamic"
> +        "--enable-syslog"
> +        "--enable-ipv6"
> +        "--enable-local"
> +        "--enable-slapd"
> +        "--enable-dynacl"
> +        "--enable-aci"
> +        "--enable-cleartext"
> +        "--enable-crypt"
> +        "--enable-spasswd"
> +        "--enable-modules"
> +        "--enable-rlookups"
> +        "--enable-slapi"
> +        "--enable-backends=mod"
> +        "--enable-overlays=mod"
> +        "--enable-argon2"
> +        "--enable-balancer"
> +        "--disable-static"
> +        "--enable-shared"
> +        "--with-tls=openssl"
> +        "--disable-static"

This is a lot more. :-)  Therefore, the question is: is it better 

 - to have only one BIG openldap package?
 - or to have one minimal openldap and a bigger variant?

Well, “guix refresh -l openldap” answers for us. ;-)

I propose to keep openldap@2.4.57 minimal, as it currently is, and use
’inherit’ to build BIG ’openldap@2.6.0.’ and variants.


> +        ,@(if (%current-target-system)
> +          '("--with-yielding_select=yes" 
> "ac_cv_func_memcmp_working=yes")
> +          '()
> +        )
> +      )
> +      #:make-flags '("STRIP=")
> +      #:parallel-build? #t

This is not necessary because it is the default.


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

A minor comment, usually, we do:

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

instead of all these closing parens, each on one line.

Using ’inherit’, this is even probably not required. :-)


> +(define-public openldap-2.5.9
> +  (package
> +    (inherit openldap)
> +    (name "openldap")
> +    (version "2.5.9")
> +    (source (origin
> +      (method url-fetch)
> +      (uri (list
> +        (string-append 
> "https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-";
>  
> version ".tgz")
> +        (string-append 
> "http://repository.linagora.org/OpenLDAP/openldap-release/openldap-"; 
> version ".tgz")
> +        (string-append 
> "ftp://ftp.dti.ad.jp/pub/net/OpenLDAP/openldap-release/openldap-"; 
> version ".tgz")
> +      ))
> +      (sha256 ( base32 
> "17pvwrj27jybbmjqpv0p7kd2qa4i6jnp134lz7cxa0sqrbs153n0" ))
> +      )

Do you need all these variants?  If yes, it could be nice to have,
instead of copy/paste all, something like:

--8<---------------cut here---------------start------------->8---
(define (openldap-uris version)
  (let ((openldap-release "OpenLDAP/openldap-release/")
        (openldap-version.tgz
         (string-append "openldap-" version ".tgz")))
    (map (lambda (url)
           (string-append url openldap-release openldap-version.tgz))
         (list "https://www.openldap.org/software/download/";
               "http://repository.linagora.org/";
               "ftp://ftp.dti.ad.jp/pub/net/";))))

(define-public openldap-2.5.8
  (package
    (inherit openldap)
    (name "openldap")
    (version "2.5.8")
    (source (origin
      (method url-fetch)
      (uri (openldap-uris version))
      (sha256
       (base32 "1p3jck2kh7rsz6mkrqaailaf9ky050hn72wph52dw0j2nb1s2vin")))))

[…]
--8<---------------cut here---------------end--------------->8---

(Untested though. :-)))



Cheers,
simon