[bug#52578] updating openldap and adding service definition

* 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
---
  gnu/packages/openldap.scm | 148 ++++++++++++++++++++++++++++++++++++++
  gnu/services/openldap.scm |  87 ++++++++++++++++++++++
  2 files changed, 235 insertions(+)
  create mode 100644 gnu/services/openldap.scm

+    (description "Run @uref{https://www.openldap.org, Openldap} 
community developped LDAP software.")
+  )
+)
--
2.30.2

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.

Hi,

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

What do you mean with ‘does not work inside guix’?
A failing test, a missing test dependency, ...?
If it doesn't work, then it should be fixed if feasible
-- test suites exist for a reason!

And if it is a failing test, that would mean the test suite caught a
bug, so in that case, the test suite is succeeding in its purpose, not
failing.

Greetings,
Maxime

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

Hi,

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

Indeed, copy-paste from our local repository went wrong.


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

These were necessary with the old autoconf in <= 2.5 realeases. It's 
mostly a leftover from the older definition already in guix.

> Why?

Stripping was sometime leading to crash of the build on my side.

> This is the default, no need to mention it.

True, leftover from when i needed the build to be monothread to see 
where it failed.


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

Ok, i will look into it.


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

I'll be honest, it's a copy-paste from the already defined package. I'll 
update it to be more meaningfull.

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

This is mostly another case of copy-paste from our local repository went 
wrong.


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

Ok, i will look into it.


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

Well, we use this in our local guix infrastructure and it doesn't 
complain, nor does our building of ldap server vms with guix system 
build.

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

True, forgot about this, my bad. Could you please point me to an example 
?

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

True, i'll try to get it work with it's own user and group.

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

This allow us to run multiple instance of this service on the same 
machine (granted you also change the storage directory 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).

Well this is beyond my current abilities.

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

True. From my understanding, when you reach user-processes you're in the 
late stage of booting your system and everything network-wise should be 
available.

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

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.

> What do you mean with ‘does not work inside guix’?

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 test work properly. And i think some 
tests need some kinds of network connection but that could be on another 
package.

Sorry for the messy patch.

Best,

Hi Maxime,

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

Indeed, copy-paste from our local repository went wrong.


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

These were necessary with the old autoconf in <= 2.5 realeases. It's 
mostly a leftover from the older definition already in guix.

> Why?

Stripping was sometime leading to crash of the build on my side.

> This is the default, no need to mention it.

True, leftover from when i needed the build to be monothread to see 
where it failed.


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

Ok, i will look into it.


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

I'll be honest, it's a copy-paste from the already defined package. I'll 
update it to be more meaningfull.

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

This is mostly another case of copy-paste from our local repository went 
wrong.


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

Ok, i will look into it.


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

Well, we use this in our local guix infrastructure and it doesn't 
complain, nor does our building of ldap server vms with guix system 
build.

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

True, forgot about this, my bad. Could you please point me to an example 
?

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

True, i'll try to get it work with it's own user and group.

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

This allow us to run multiple instance of this service on the same 
machine (granted you also change the storage directory 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).

Well this is beyond my current abilities.

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

True. From my understanding, when you reach user-processes you're in the 
late stage of booting your system and everything network-wise should be 
available.

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

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.

> What do you mean with ‘does not work inside guix’?

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 test work properly. And i think some 
tests need some kinds of network connection but that could be on another 
package.

Sorry for the messy patch.

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

Hi Maxime,

The package ’openldap’ already exists and some of your comments ask
about the existing recipe. :-)

I think Jean-François just copy/pasted the current recipe and expand it
for their own requirements: having the service they need, IIUC.


On Fri, 17 Dec 2021 at 22:39, Maxime Devos <maximedevos@telenet.be> wrote:

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

This bits had been added by Mathieu 1c8b1870a60de12f6c19d809522f5d8362215131.


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

This one too.

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

Again this one.


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

This description is from 2013, 2a75d4e664e802d3a3e2ed6455c63f32541559c8. ;-)


Your comments about the package itself make sense but I am not convinced
that they are related to the first Jean-François submission. :-)


Cheers,
simon

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, 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?
> 

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 openldap@2.4.57 minimal, as it currently is, and use
> ’inherit’ to build BIG ’openldap@2.6.0.’ 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