Package: guix-patches;
Reported by: Jean-Francois GUILLAUME <Jean-Francois.Guillaume <at> univ-nantes.fr>
Date: Fri, 17 Dec 2021 15:07:01 UTC
Severity: normal
Tags: patch
Message #14 received at 52578 <at> debbugs.gnu.org (full text, mbox):
From: zimoun <zimon.toutoune <at> gmail.com> To: Jean-Francois GUILLAUME <Jean-Francois.Guillaume <at> univ-nantes.fr>, 52578 <at> debbugs.gnu.org Subject: Re: [bug#52578] [PATCH] updating openldap and adding service definition Date: Sat, 18 Dec 2021 11:22:05 +0100
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 <at> 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 <at> 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 <at> 2.4.57 minimal, as it currently is, and use
’inherit’ to build BIG ’openldap <at> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.