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

From: Jean-Francois GUILLAUME <[email-masked]>
Date: Sat, 18 Dec 2021 12:09:49 +0100

Hi Simon,

> Nice to see you here. :-)

Thanks :)

> Various comments for improving the submission.

Angain, thank you. I'll glady take on these as i've other packages to 
contribute.

> 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.)

As you wish, i must admit i was kind of lazy and wanted to provide 
everything in one go.

> 
> Why the mirror list had been removed?
> 
> [...]
> 
> This is new, right?
> 

It's still using a mirror list, i've tried to select a few on each 
region of th e world on openldap's download page.

> 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.)
> 

Well, i found it more easy to read on one line but it's true that i use 
a wide terminal. I can change it, no problems.

> 
>> +    (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, [email-masked] 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?
> 

With his definition you can now run a fully featured openldap server. We 
were missing quite a few features when using the 2.4.57 version (which 
is nearly only the client tools).

> 
>> +    (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.
> 

My bad, leftovers from our local repo. For some strange reasons, when 
the tests are run by guix build they do not properly clean after each 
steps and ends up failing. If you do the same inside a guix environment 
they work properly. And i think some tests need some kinds of network 
connection but that could be on another package.

> 
>> +      #: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. :-) [...]

Indeed, need quite a lot to get a fully featured server.

> [...] 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 [email-masked] minimal, as it currently is, and use
> ’inherit’ to build BIG ’[email-masked].’ and variants.
> 

As you wish either work for me. I can also do a "-minimal" version with 
only what is needed to get a client version and a "-full" version to get 
a fully featured server.

> 
>> +        ,@(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.
> 

OK.

> 
>> +      #: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. :-)
> 

Leftovers from our local repo, we rely a bit to much on indentation to 
help us have a better view of where blocks start and stop.

> 
>> +(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. :-)))
> 

This is mostly another case of copy-paste from our local repository went 
wrong.
Initially i intended to provide only the latests versions for 2.6.x and 
2.5.x and keeping 2.4.57 from compatibility reasons.
While doing the definitions, i was wondering how i could provide only 
the hash and the version, guess i'll try your solution :)

Best,
---
Cordialement,
Jean-François GUILLAUME
Plateforme Bioinformatique BiRD

Tél. : +33 (0)2 28 08 00 57
www.pf-bird.univ-nantes.fr

Inserm UMR 1087/CNRS UMR 6291
IRS-UN - 8 quai Moncousu - BP 70721
44007 Nantes Cedex 1