[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[david larsson]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

Hi!
This patch is needed for the Galera add-on to MariaDB, which runs some 
scripts like for example wsrep_sst_rsync that needs access to additional 
binaries in PATH.

I tested the patch with (and without) below snippets to the 
mysql-service in my config.scm and successfully connected to a 
MariaDB/Galera cluster.

I ran these commands to test:
guix pull --url=/home/user1/src/guix --profile=/tmp/guix.master 
--disable-authentication --allow-downgrades ; 
GUIX_PROFILE="/tmp/guix.master" ; . "$GUIX_PROFILE/etc/profile" ; guix 
system reconfigure config.scm --fallback --allow-downgrades

------------------------------------------------------------------

(extra-environment #~(list (string-append "PATH=/usr/bin:/bin:" #$rsync 
"/bin:" #$coreutils "/bin:" #$gawk "/bin:" #$grep "/bin:" #$mariadb 
"/bin:" #$iproute "/sbin:" 
"/run/setuid-programs:/run/current-system/profile/bin:/run/current-system/profile/sbin" 
) (string-append "SHELL=" #$bash) "USER=mysql" 
"SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt" 
"SSL_CERT_DIR=/run/current-system/profile/etc/ssl/certs"))


(extra-content #~(string-append "log_error=/var/lib/mysql/log_error.log
# 
https://www.percona.com/blog/2017/07/26/what-is-innodb_autoinc_lock_mode-and-why-should-i-care/
binlog_format=ROW
default-storage-engine=innodb
innodb_autoinc_lock_mode=2

# Galera Provider Configuration
wsrep_on=ON
wsrep_provider=" #$galera "/lib/libgalera_smm.so

# Galera Cluster Configuration
wsrep_cluster_name=\"test_cluster\"
wsrep_cluster_address=\"gcomm://redacted,redacted\"
# according to 
https://galeracluster.com/library/documentation/mysql-wsrep-options.html
# leaving it empty starts a new cluster, so you should immediately 
reconfigure again after doing this.
#wsrep_cluster_address=\"gcomm://\"

# Galera Synchronization Configuration
wsrep_sst_method=rsync

# Galera Node Configuration
wsrep_node_address=\"redacted\"
wsrep_node_name=\"librem13v3guixsd\""))
                                ))

------------------------------------------------------------------

Please someone also review [bug#47517] [PATCH] gnu: nginx: Enable stream 
module

which adds support for tcp loadbalancing that can be used to scale a 
MariaDB/Galera cluster.

Best regards,
David


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-services-mysql-Add-extra-environment-as-configuratio.patch --]
[-- Type: text/x-diff; name=0001-services-mysql-Add-extra-environment-as-configuratio.patch, Size: 2954 bytes --]

From 927444652f4b774928de78c1558b4e942abff203 Mon Sep 17 00:00:00 2001
From: methuselah-0 <david.larsson@selfhosted.xyz>
Date: Sun, 11 Apr 2021 10:30:01 +0200
Subject: [PATCH] services: mysql: Add extra-environment as configuration
 option.

* gnu/services/databases.scm (mysql-configuration): Add extra-environment
(mysql-service): Use #:log-file and #:environment-variables

* doc/guix.texi: Document it.
---
 doc/guix.texi              | 3 +++
 gnu/services/databases.scm | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index c23d044ff5..298585a695 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -19809,6 +19809,9 @@ Socket file to use for local (non-network) connections.
 @item @code{extra-content} (default: @code{""})
 Additional settings for the @file{my.cnf} configuration file.
 
+@item @code{extra-environment} (default: @code{#~'()})
+List of environment variables passed to the @command{mysqld} process.
+
 @item @code{auto-upgrade?} (default: @code{#t})
 Whether to automatically run @command{mysql_upgrade} after starting the
 service.  This is necessary to upgrade the @dfn{system schema} after
diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index a841e7a50e..8d266c1cba 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -7,6 +7,7 @@
 ;;; Copyright © 2018 Julien Lepiller <julien@lepiller.eu>
 ;;; Copyright © 2019 Robert Vollmert <rob@vllmrt.net>
 ;;; Copyright © 2020 Marius Bakke <marius@gnu.org>
+;;; Copyright © 2021 David Larsson <david.larsson@selfhosted.xyz>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -527,6 +528,7 @@ created after the PostgreSQL database is started.")))
   (port mysql-configuration-port (default 3306))
   (socket mysql-configuration-socket (default "/run/mysqld/mysqld.sock"))
   (extra-content mysql-configuration-extra-content (default ""))
+  (extra-environment mysql-configuration-extra-environment (default #~'()))
   (auto-upgrade? mysql-configuration-auto-upgrade? (default #t)))
 
 (define %mysql-accounts
@@ -611,11 +613,14 @@ FLUSH PRIVILEGES;
          (provision '(mysql))
          (documentation "Run the MySQL server.")
          (start (let ((mysql  (mysql-configuration-mysql config))
+                      (extra-env (mysql-configuration-extra-environment config))
                       (my.cnf (mysql-configuration-file config)))
                   #~(make-forkexec-constructor
                      (list (string-append #$mysql "/bin/mysqld")
                            (string-append "--defaults-file=" #$my.cnf))
-                     #:user "mysql" #:group "mysql")))
+                           #:user "mysql" #:group "mysql"
+                           #:log-file "/var/log/mysqld.log"
+                           #:environment-variables #$extra-env)))
          (stop #~(make-kill-destructor)))))
 
 (define (mysql-upgrade-wrapper mysql socket-file)
-- 
2.30.2

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

On Sun, 2021-04-11 at 10:44 +0200, david larsson wrote:
> Hi!
> This patch is needed for the Galera add-on to MariaDB, which runs some 
> scripts like for example wsrep_sst_rsync that needs access to additional 
> binaries in PATH.
> [... reordered ...]
> (extra-environment #~(list (string-append "PATH=/usr/bin:/bin:" #$rsync 
> "/bin:" #$coreutils "/bin:" #$gawk "/bin:" #$grep "/bin:" #$mariadb 
> "/bin:" #$iproute "/sbin:" 
> "/

Please corect the galera package to refer to the coreutils (ls, stat, ...)
by absolute file name instead, using something like

(add-after 'install
  (substitute* "INSTALL-LOCATION/wsrep_sst_rsync"
    (("\\bls\\b") (string-append (assoc-ref inputs "coreutils") "/bin/ls"))
    ...))

(Likewise for rsync, gawk, iproute ...)

Don't use (which "ls") instead of string-append + assoc-ref! (which "ls") is
incorrect when cross-compiling;

That way, people don't have to fiddle with PATH in their configuration file.

> I tested the patch with (and without) below snippets to the 
> mysql-service in my config.scm and successfully connected to a 
> MariaDB/Galera cluster.

If possible, consider writing a "system test" automatically testing
some very basic functionality of mariadb + galera (gnu/tests/databases.scm).

> I ran these commands to test:
> guix pull --url=/home/user1/src/guix --profile=/tmp/guix.master 
> --disable-authentication --allow-downgrades ; 
> GUIX_PROFILE="/tmp/guix.master" ; . "$GUIX_PROFILE/etc/profile" ; guix 
> system reconfigure config.scm --fallback --allow-downgrades
> 
> ------------------------------------------------------------------
> 
> (extra-environment #~(list ...
>
> "USER=mysql" 
> "SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt" 
> "SSL_CERT_DIR=/run/current-system/profile/etc/ssl/certs"))

It seems extra-environment is still useful.  "USER=mysql" should probably be
added automatically, though (see my proposal below).

> 
> (extra-content #~(string-append "log_error=/var/lib/mysql/log_error.log
> # 
> https://www.percona.com/blog/2017/07/26/what-is-innodb_autoinc_lock_mode-and-why-should-i-care/
> binlog_format=ROW
> default-storage-engine=innodb
> innodb_autoinc_lock_mode=2
> 
> # Galera Provider Configuration
> wsrep_on=ON
> wsrep_provider=" #$galera "/lib/libgalera_smm.so
> 
> # Galera Cluster Configuration
> wsrep_cluster_name=\"test_cluster\"
> wsrep_cluster_address=\"gcomm://redacted,redacted\"
> # according to 
> https://galeracluster.com/library/documentation/mysql-wsrep-options.html
> # leaving it empty starts a new cluster, so you should immediately 
> reconfigure again after doing this.
> #wsrep_cluster_address=\"gcomm://\"
> 
> # Galera Synchronization Configuration
> wsrep_sst_method=rsync
> 
> # Galera Node Configuration
> wsrep_node_address=\"redacted\"
> wsrep_node_name=\"librem13v3guixsd\""))
>                                 ))

Perhaps you could extend "mysql-configuration" with a "galera" field
(with #f as default)?  Theoretical example:

(mysql-configuration
  (port A-DIFFERENT-PORT)
  ;; [...] other fields
  (galera
    (package my-version-of-galera) ; optional
    (cluster-name "test_cluster")
    (cluster-address "gcom://...")
    (synchronization-method 'rsync)
    (node-adress "redacted")
    (node-name "librem13v3guixsd")))

.. and modify mysql-service-type to insert appropriate configuration entries
and perhaps add things to the PATH of the shepherd service as appropriate.

Escape hatches like "extra-content" are useful, but this seems a bit
neater.

> ------------------------------------------------------------------
> 
> Please someone also review [bug#47517] [PATCH] gnu: nginx: Enable stream 
> module

I'll take a look at it.

Greetings,
Maxime. 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[david larsson]

Hi Maxime!

On 2021-04-11 17:33, Maxime Devos wrote:
> Please corect the galera package to refer to the coreutils (ls, stat, 
> ...)
> by absolute file name instead, using something like
> 
> (add-after 'install
>   (substitute* "INSTALL-LOCATION/wsrep_sst_rsync"
>     (("\\bls\\b") (string-append (assoc-ref inputs "coreutils") 
> "/bin/ls"))
>     ...))
> 
> (Likewise for rsync, gawk, iproute ...)
> 
> Don't use (which "ls") instead of string-append + assoc-ref! (which 
> "ls") is
> incorrect when cross-compiling;
> 
> That way, people don't have to fiddle with PATH in their configuration 
> file.

I think you misundestood here - these rsync, gawk, iproute etc are 
executed as part of scripts in the mysqld/bin package output folder (see 
`ls -la $(dirname $(readlink -f $(which mysqld)))/` ) because the Galera 
add-on was (at least partially) merged into the regular mariadb sources, 
so the scripts where the binaries are needed are not from the galera 
package outputs but the mysql one. The galera package in guix master 
today only provides the #$galera "/lib/libgalera_smm.so file. I agree it 
would be nice to patch all the shell scripts in the $#mysql/bin folder 
but this would 1. require maintaining all the additional patching of 
those scripts as part of the mysql package and 2. since custom 
wsrep_sst_<X> shell scripts are possible and are usually added to 
/usr/bin/wsrep_sst_<X> those binaries will also require setting 
additional environment variables or the same type of patching (which 
would therefore "require" this patch to make it easier).

> If possible, consider writing a "system test" automatically testing
> some very basic functionality of mariadb + galera 
> (gnu/tests/databases.scm).

I would have liked to, but Im sorry - I do not have the time/skill for 
this for the time being. I hope the current patch is simple enough that 
an additional test is not currently needed.

>> I ran these commands to test:
>> guix pull --url=/home/user1/src/guix --profile=/tmp/guix.master
>> --disable-authentication --allow-downgrades ;
>> GUIX_PROFILE="/tmp/guix.master" ; . "$GUIX_PROFILE/etc/profile" ; guix
>> system reconfigure config.scm --fallback --allow-downgrades
>> 
>> ------------------------------------------------------------------
>> 
>> (extra-environment #~(list ...
>> 
>> "USER=mysql"
>> "SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt"
>> "SSL_CERT_DIR=/run/current-system/profile/etc/ssl/certs"))
> 
> It seems extra-environment is still useful.  "USER=mysql" should 
> probably be
> added automatically, though (see my proposal below).

Yes, in particular for custom wsrep scripts. The "USER=mysql" may not be 
needed per se for the example snippet config I sent earlier as I did not 
make sure that the list of environment variables there were the minimal 
amount needed; I mainly hoped to provide a repeatable example of 
something to test with.

> Perhaps you could extend "mysql-configuration" with a "galera" field
> (with #f as default)?  Theoretical example:
> 
> (mysql-configuration
>   (port A-DIFFERENT-PORT)
>   ;; [...] other fields
>   (galera
>     (package my-version-of-galera) ; optional
>     (cluster-name "test_cluster")
>     (cluster-address "gcom://...")
>     (synchronization-method 'rsync)
>     (node-adress "redacted")
>     (node-name "librem13v3guixsd")))
> 
> .. and modify mysql-service-type to insert appropriate configuration 
> entries
> and perhaps add things to the PATH of the shepherd service as 
> appropriate.
> 
> Escape hatches like "extra-content" are useful, but this seems a bit
> neater.

This looks nice! I agree that something like what you show here is the 
better option, at the same time I don't have the time/skill to provide a 
patch with a well-featured interface to the galera options for the time 
being. The "escape hatch" would therefore be very useful for now, and 
less hassle to maintain - compare with for example the vpn service and 
the amount of emails in the lists regarding lack of options that could 
have easily been added via some g-expression strings. In general I don't 
see the harm in providing both "the escape hatch" way to add options to 
a configuration file and the guile interface which is otherwise nicer 
(IMO).

>> ------------------------------------------------------------------
>> 
>> Please someone also review [bug#47517] [PATCH] gnu: nginx: Enable 
>> stream
>> module
> 
> I'll take a look at it.

Thanks!

Best regards,
David

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 7167 bytes --]

On Sun, 2021-04-11 at 20:07 +0200, david larsson wrote:
> Hi Maxime!
> 
> On 2021-04-11 17:33, Maxime Devos wrote:
> > Please corect the galera package to refer to the coreutils (ls, stat, 
> > ...)
> > by absolute file name instead, using something like
> > 
> > (add-after 'install
> >   (substitute* "INSTALL-LOCATION/wsrep_sst_rsync"
> >     (("\\bls\\b") (string-append (assoc-ref inputs "coreutils") 
> > "/bin/ls"))
> >     ...))
> > 
> > (Likewise for rsync, gawk, iproute ...)
> > 
> > Don't use (which "ls") instead of string-append + assoc-ref! (which 
> > "ls") is
> > incorrect when cross-compiling;
> > 
> > That way, people don't have to fiddle with PATH in their configuration 
> > file.
> 
> I think you misundestood here - these rsync, gawk, iproute etc are 
> executed as part of scripts in the mysqld/bin package output folder

Ok, I should have asked to modify the mariadb package instead.

>  (see 
> `ls -la $(dirname $(readlink -f $(which mysqld)))/` ) because the Galera 
> add-on was (at least partially) merged into the regular mariadb sources, 
> so the scripts where the binaries are needed are not from the galera 
> package outputs but the mysql one. The galera package in guix master 
> today only provides the #$galera "/lib/libgalera_smm.so file.

So the mariadb package has the scripts, and the galera package has a library.

>  I agree it 
> would be nice to patch all the shell scripts in the $#mysql/bin folder 
> but this would 1. require maintaining all the additional patching of 
> those scripts as part of the mysql package

This shouldn't be complicated, though possibly somewhat tedious.
I took a look at /gnu/store/bjgz8jnfsbb4qvaa9csfy8i3x1i3ivp7-mariadb-10.5.8/bin/wsrep_*
(your hash may vary).  The following should be ‘absolutised’:

* In wsrep_sst_mariabackup:

  OS=$(uname)
  sfmt="tar"
  if pv --help 2>/dev/null | grep -q FORMAT;then
  mariabackup
  wsrep_log_error
  mbstream
  xbcrypt (*)
  [more things]
* In wsrep_sst_*: other things (eg. rm)

The following shouldn't be required, and could be commented out:
# Setting the path for lsof on CentOS
export PATH="/usr/sbin:/sbin:$PATH"

It's a little tedious, but it should be worth it.  This could be done
in the post-install phase of mariadb.  For an (almost) good example on
how to ‘absolutise’, see 'xvfb-run'.  Actually, it uses "which" which
is incorrect when cross-compiling, but that can be worked-around by
adding (setenv "PATH" (string-append BINDIR-OF-INPUTS-COREUTILS ":"
                                     BINDIR-OF-INPUTS-AWK ...))

About xbcrypt (*): I have no idea from which package this comes.
Is it an optional dependency?  If it is optional, _not_ patching it
could make sense, as to avoid increasing the closure.  This would
extra-environment.
>  and 2. since custom 
> wsrep_sst_<X> shell scripts are possible and are usually added to 
> /usr/bin/wsrep_sst_<X> those binaries will also require setting 
> additional environment variables or the same type of patching
> (which would therefore "require" this patch to make it easier).

I'd recommend that these custom shell scripts are patched as well,
but idk how they would be used with mariadb, perhaps there are
some complications here.

That said, if that's too much work or too error-prone, I've an alternative
proposal below, which is a little more ‘high level’ than asking the user to
spell out the PATH:

Take a look at 'nscd-shepherd-service' in gnu/serices/base.scm:

  [snip]
  #:environment-variables
  (list (string-append "LD_LIBRARY_PATH="
          (string-join
            (map (lambda (dir)
              (string-append dir "/lib"))
            (list #$@name-services))
            ":")))))

Replace LD_LIBRARY_PATH with PATH.  Add a "extensions" field to
<mysql-configuration>.  Replace 'name-services' with 'extensions'.

Procedures you need to modify:
  * mysql-cofiguration-file: add the field to $ <mysql-configuration>
  * mysql-shepherd-service: add #:environment-variables as appropriate
  * mysql-upgrade-shepherd-service: also, maybe, I don't know?

Possible problems: maybe some scripts need some additional environment
variables.  Mabe provide both an "extensions" field, and an
"extra-environment" field, and combine the results?

> > If possible, consider writing a "system test" automatically testing
> > some very basic functionality of mariadb + galera 
> > (gnu/tests/databases.scm).
> 
> I would have liked to, but Im sorry - I do not have the time/skill for 
> this for the time being. I hope the current patch is simple enough that 
> an additional test is not currently needed.

For the patch as you've submitted it, the lack of a system test shouldn't
be a problem, as Guix System doesn't support mariadb + galera.  The system
test is more for if ‘we’ add a 'galera' field to 'mysql-configuration' as
I suggested.

> > It seems extra-environment is still useful.  "USER=mysql" should 
> > probably be
> > added automatically, though (see my proposal below).
> 
> Yes, in particular for custom wsrep scripts.
>  The "USER=mysql" may not be 
> needed per se for the example snippet config I sent earlier as I did not 
> make sure that the list of environment variables there were the minimal 
> amount needed; I mainly hoped to provide a repeatable example of 
> something to test with.

Warning: I didn't actually test your patch.  I don't have a mysql service.

> > Perhaps you could extend "mysql-configuration" with a "galera" field
> > (with #f as default)?  Theoretical example:
> > 
> > (mysql-configuration [...] (galera [...]))
>
> > .. and modify mysql-service-type to insert appropriate configuration 
> > entries
> > and perhaps add things to the PATH of the shepherd service as 
> > appropriate.
> > 
> > Escape hatches like "extra-content" are useful, but this seems a bit
> > neater.
> 
> This looks nice! I agree that something like what you show here is the 
> better option, at the same time I don't have the time/skill to provide a 
> patch with a well-featured interface to the galera options for the time 
> being. The "escape hatch" would therefore be very useful for now, and 
> less hassle to maintain - compare with for example the vpn service and 
> the amount of emails in the lists regarding lack of options that could 
> have easily been added via some g-expression strings.

Somewhat off-topic, which e-mails would these be?

> In general I don't 
> see the harm in providing both "the escape hatch" way to add options to 
> a configuration file and the guile interface which is otherwise nicer 
> (IMO).

Agreed.  I believe the current plan is to:

* add the 'extra-environment' escape hatch.
* (possibly [dubious-discuss]) Add the extensions field (a list of
  package objects).  This adds the listed packages to PATH.
  Slightly neater than 'extra-environment', but is not as general
  as 'extra-environment'.
* The contents of 'extra-environment' and and 'extensions' are merged.

> Best regards,
> David
^W^W^W^W^W Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[david larsson]

>> On 2021-04-11 17:33, Maxime Devos wrote:
>> > Please corect the galera package to refer to the coreutils (ls, stat,
>> > ...)
>> > by absolute file name instead, using something like
>> >
>> > (add-after 'install
>> >   (substitute* "INSTALL-LOCATION/wsrep_sst_rsync"
>> >     (("\\bls\\b") (string-append (assoc-ref inputs "coreutils")
>> > "/bin/ls"))
>> >     ...))
>> >
>> > (Likewise for rsync, gawk, iproute ...)

I don't think this is a nice solution because for every update to the 
mysql package would mean to verify that we are actually hitting a bunch 
of spread out invocations of external programs inside those shell 
scripts with the regex in the (substitute* .. procedure. I would suggest 
instead to define a variable like say: (define 
%default-mysqld-environment #~(list (string-append #$gawk "/bin/awk") 
..) which contains all the external commands invoked from the shell 
scripts in the mysql/bin folder, and append that to the 
extra-environment list.

>>  I agree it
>> would be nice to patch all the shell scripts in the $#mysql/bin folder
>> but this would 1. require maintaining all the additional patching of
>> those scripts as part of the mysql package
> 
> This shouldn't be complicated, though possibly somewhat tedious.
> I took a look at
> /gnu/store/bjgz8jnfsbb4qvaa9csfy8i3x1i3ivp7-mariadb-10.5.8/bin/wsrep_*
> (your hash may vary).  The following should be ‘absolutised’:
> 
> * In wsrep_sst_mariabackup:
> 
>   OS=$(uname)
>   sfmt="tar"
>   if pv --help 2>/dev/null | grep -q FORMAT;then
>   mariabackup
>   wsrep_log_error
>   mbstream
>   xbcrypt (*)
>   [more things]
> * In wsrep_sst_*: other things (eg. rm)
> 
> The following shouldn't be required, and could be commented out:
> # Setting the path for lsof on CentOS
> export PATH="/usr/sbin:/sbin:$PATH"
> 
> It's a little tedious, but it should be worth it.  This could be done
> in the post-install phase of mariadb.

I find this to be too tedious, and since it introduces maintenance 
hassle I would prefer my suggestion above.

> For an (almost) good example on
> how to ‘absolutise’, see 'xvfb-run'.  Actually, it uses "which" which
> is incorrect when cross-compiling, but that can be worked-around by
> adding (setenv "PATH" (string-append BINDIR-OF-INPUTS-COREUTILS ":"
>                                      BINDIR-OF-INPUTS-AWK ...))
> 
> About xbcrypt (*): I have no idea from which package this comes.
> Is it an optional dependency?  If it is optional, _not_ patching it
> could make sense, as to avoid increasing the closure.  This would
> extra-environment.

[..]

> That said, if that's too much work or too error-prone, I've an 
> alternative
> proposal below, which is a little more ‘high level’ than asking the 
> user to
> spell out the PATH:
> 
> Take a look at 'nscd-shepherd-service' in gnu/serices/base.scm:
> 
>   [snip]
>   #:environment-variables
>   (list (string-append "LD_LIBRARY_PATH="
>           (string-join
>             (map (lambda (dir)
>               (string-append dir "/lib"))
>             (list #$@name-services))
>             ":")))))
> 
> Replace LD_LIBRARY_PATH with PATH.  Add a "extensions" field to
> <mysql-configuration>.  Replace 'name-services' with 'extensions'.

Not sure if you mean to keep /lib in the above procedure and use this 
for Galera's .so file, or to help set the PATH for the external programs 
invoked from the shell scripts?

AFAIK the galera service requires you to specify the full path to the 
.so either way, and if you meant to use /bin instead of /lib above; the 
binaries of the external programs that are needed are sometimes in 
out/sbin and sometimes in out/bin so this would unfortunately miss 
programs occasionally.

> Procedures you need to modify:
>   * mysql-cofiguration-file: add the field to $ <mysql-configuration>
>   * mysql-shepherd-service: add #:environment-variables as appropriate
>   * mysql-upgrade-shepherd-service: also, maybe, I don't know?
> 
> Possible problems: maybe some scripts need some additional environment
> variables.  Mabe provide both an "extensions" field, and an
> "extra-environment" field, and combine the results?

Possibly. What would you say about opening a separate bug report and 
potentially fix those things in separate PATCH-set? The mysql-upgrade 
service in particular is something I don't know whether it could benefit 
from adding some things to the PATH.

> For the patch as you've submitted it, the lack of a system test 
> shouldn't
> be a problem, as Guix System doesn't support mariadb + galera.  The 
> system
> test is more for if ‘we’ add a 'galera' field to 'mysql-configuration' 
> as
> I suggested.

Great!

>> The "escape hatch" would therefore be very useful for now, and
>> less hassle to maintain - compare with for example the vpn service and
>> the amount of emails in the lists regarding lack of options that could
>> have easily been added via some g-expression strings.
> 
> Somewhat off-topic, which e-mails would these be?

This one for example: 
https://lists.gnu.org/archive/html/bug-guix/2020-02/msg00321.html
But I would also assume that some of the interest in using 
network-manager's vpn plugin is due to it being hard to cover all the 
options in openvpn-client-configuration. (There was also an issue with 
cert and key options being mandatory in openvpn-client-configuration)


> Agreed.  I believe the current plan is to:
> 
> * add the 'extra-environment' escape hatch.
> * (possibly [dubious-discuss]) Add the extensions field (a list of
>   package objects).  This adds the listed packages to PATH.
>   Slightly neater than 'extra-environment', but is not as general
>   as 'extra-environment'.
> * The contents of 'extra-environment' and and 'extensions' are merged.

Can we do or discuss these things in a separately filed BUG report or 
PATCH?

Best regards,
David

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 5273 bytes --]

On Mon, 2021-04-12 at 20:06 +0200, david larsson wrote:
> > > On 2021-04-11 17:33, Maxime Devos wrote:
> > > > Please corect the galera package to refer to the coreutils (ls, stat,
> > > > ...)
> > > > by absolute file name instead, using something like
> > > > 
> > > > (add-after 'install
> > > >   (substitute* "INSTALL-LOCATION/wsrep_sst_rsync"
> > > >     (("\\bls\\b") (string-append (assoc-ref inputs "coreutils")
> > > > "/bin/ls"))
> > > >     ...))
> > > > 
> > > > (Likewise for rsync, gawk, iproute ...)
> 
> I don't think this is a nice solution because for every update to the 
> mysql package would mean to verify that we are actually hitting a bunch 
> of spread out invocations of external programs inside those shell 
> scripts with the regex in the (substitute* .. procedure. I would suggest 
> instead to define a variable like say: (define 
> %default-mysqld-environment #~(list (string-append #$gawk "/bin/awk") 
> ..) which contains all the external commands invoked from the shell 
> scripts in the mysql/bin folder, and append that to the 
> extra-environment list.

What about inserting a (string-append "export PATH=" #$(file-apend gawk "/bin/awk") ...)
line in the scripts?  Some of the scripts even already have a PATH=... line
(PATH=/bin:/sbin:/usr/bin or something like that).

> I find this to be too tedious, and since it introduces maintenance 
> hassle I would prefer my suggestion above.

Fair enough (-:.

> > [...]
> > Take a look at 'nscd-shepherd-service' in gnu/serices/base.scm:
> > 
> >   [snip]
> >   #:environment-variables
> >   (list (string-append "LD_LIBRARY_PATH="
> >           (string-join
> >             (map (lambda (dir)
> >               (string-append dir "/lib"))
> >             (list #$@name-services))
> >             ":")))))
> > 
> > Replace LD_LIBRARY_PATH with PATH.  Add a "extensions" field to
> > <mysql-configuration>.  Replace 'name-services' with 'extensions'.
> 
> Not sure if you mean to keep /lib in the above procedure and use this 
> for Galera's .so file, or to help set the PATH for the external programs 
> invoked from the shell scripts?

I meant for help setting the PATH for the external programs invoked from
the shell scripts. 

> AFAIK the galera service requires you to specify the full path to the 
> .so either way, and if you meant to use /bin instead of /lib above; the 
> binaries of the external programs that are needed are sometimes in 
> out/sbin and sometimes in out/bin so this would unfortunately miss 
> programs occasionally.

I guess both out/bin and out/sbin could be added to the PATH.

> > Procedures you need to modify:
> >   * mysql-cofiguration-file: add the field to $ <mysql-configuration>
> >   * mysql-shepherd-service: add #:environment-variables as appropriate
> >   * mysql-upgrade-shepherd-service: also, maybe, I don't know?
> > 
> > Possible problems: maybe some scripts need some additional environment
> > variables.  Mabe provide both an "extensions" field, and an
> > "extra-environment" field, and combine the results?
> 
> Possibly. What would you say about opening a separate bug report and 
> potentially fix those things in separate PATCH-set? The mysql-upgrade 
> service in particular is something I don't know whether it could benefit 
> from adding some things to the PATH.

mysql-configuration-file deconstruct the <mysql-configuration> struct.  I'm not
100% sure, but I think when you have a (match obj (($ <record> field ...) ...))
expression, and you add a field to <record>, you need to add the new field to
$ <record> field ..., in the same order.

mysql-shepherd-service: no need to explain, you have modified it in your patch
to pass #:enviroment-variables #$extra-env.

mysql-upgrade-service: I don't know either.

I'm not going to write a patch.  I don't have a mysql service on my system;
it was only a suggestion.

> [...]
> This one for example: 
> https://lists.gnu.org/archive/html/bug-guix/2020-02/msg00321.html
> But I would also assume that some of the interest in using 
> network-manager's vpn plugin is due to it being hard to cover all the 
> options in openvpn-client-configuration. (There was also an issue with 
> cert and key options being mandatory in openvpn-client-configuration)
That's an informtive example, thanks!

> 
> > Agreed.  I believe the current plan is to:
> > 
> > * add the 'extra-environment' escape hatch.
> > * (possibly [dubious-discuss]) Add the extensions field (a list of
> >   package objects).  This adds the listed packages to PATH.
> >   Slightly neater than 'extra-environment', but is not as general
> >   as 'extra-environment'.
> > * The contents of 'extra-environment' and and 'extensions' are merged.
> 
> Can we do or discuss these things in a separately filed BUG report or 
> PATCH?

These things = the 'extensions' field, and merging 'extra-environment' and
'extensions'?  As I said, it was only a suggestion and I won't be working on it.
I think your original patch is good to go into the git repo.  I'll open a
separate bug report about ‘absolutising’ the binaries referred to from the scripts.

Note: I do not have commit access.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

bug#47704: [PATCH] services: mysql: Add extra-environment as configuration option.

[Leo Prikler]

Am Montag, den 12.04.2021, 22:09 +0200 schrieb Maxime Devos:
> I think your original patch is good to go into the git repo.  I'll
> open a
> separate bug report about ‘absolutising’ the binaries referred to
> from the scripts.
I've pushed this patch now, but let us still look for a smaller
solution if applicable.  (That said, I'm not a mysql user and I'm happy
to leave security stuff to lle_bout.)

@david: Note, that I did not change the author, meaning it is committed
as "methuselah-0 <david.larsson@selfhosted.xyz>" rather than 
"david larsson <david.larsson@selfhosted.xyz>".  Since this patch is
hopefully small enough to not require attribution, that is fine, but if
you plan on making bigger changes, please consider setting your git up
appropriately.

Regards,
Leo 

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Julien Lepiller]

Le Tue, 13 Apr 2021 18:58:57 +0200,
Leo Prikler <leo.prikler@student.tugraz.at> a écrit :

> Am Montag, den 12.04.2021, 22:09 +0200 schrieb Maxime Devos:
> > I think your original patch is good to go into the git repo.  I'll
> > open a
> > separate bug report about ‘absolutising’ the binaries referred to
> > from the scripts.  
> I've pushed this patch now, but let us still look for a smaller
> solution if applicable.  (That said, I'm not a mysql user and I'm
> happy to leave security stuff to lle_bout.)
> 
> @david: Note, that I did not change the author, meaning it is
> committed as "methuselah-0 <david.larsson@selfhosted.xyz>" rather
> than "david larsson <david.larsson@selfhosted.xyz>".  Since this
> patch is hopefully small enough to not require attribution, that is
> fine, but if you plan on making bigger changes, please consider
> setting your git up appropriately.
> 
> Regards,
> Leo 
> 
> 
> 
> 

Hi!

I saw this patch went through, but it updates the Guix Manual while we
are in string freeze. The change is very small and close to the start
of string freeze, so I'm willing to let it go. However, please refrain
from pushing such changes to the manual. Ideally, this patch should
have been pushed yesterday or postponed until after the release.

As a reminder, the string freeze applies to guix and to the manual. It
does not apply to the website, package synopsis and descriptions, nor
to the cookbook.

Thank you!

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Leo Prikler]

Am Mittwoch, den 14.04.2021, 00:29 +0200 schrieb Julien Lepiller:
> Le Tue, 13 Apr 2021 18:58:57 +0200,
> Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
> 
> > Am Montag, den 12.04.2021, 22:09 +0200 schrieb Maxime Devos:
> > > I think your original patch is good to go into the git
> > > repo.  I'll
> > > open a
> > > separate bug report about ‘absolutising’ the binaries referred to
> > > from the scripts.  
> > I've pushed this patch now, but let us still look for a smaller
> > solution if applicable.  (That said, I'm not a mysql user and I'm
> > happy to leave security stuff to lle_bout.)
> > 
> > @david: Note, that I did not change the author, meaning it is
> > committed as "methuselah-0 <david.larsson@selfhosted.xyz>" rather
> > than "david larsson <david.larsson@selfhosted.xyz>".  Since this
> > patch is hopefully small enough to not require attribution, that is
> > fine, but if you plan on making bigger changes, please consider
> > setting your git up appropriately.
> > 
> > Regards,
> > Leo 
> > 
> > 
> > 
> > 
> 
> Hi!
> 
> I saw this patch went through, but it updates the Guix Manual while
> we
> are in string freeze. The change is very small and close to the start
> of string freeze, so I'm willing to let it go. However, please
> refrain
> from pushing such changes to the manual. Ideally, this patch should
> have been pushed yesterday or postponed until after the release.
> 
> As a reminder, the string freeze applies to guix and to the manual.
> It
> does not apply to the website, package synopsis and descriptions, nor
> to the cookbook.
> 
> Thank you!
Ahh, my bad, I totally missed that.  Would it be better to revert this
change until April 18th and then apply it again?

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Julien Lepiller]

Le Wed, 14 Apr 2021 00:38:50 +0200,
Leo Prikler <leo.prikler@student.tugraz.at> a écrit :

> Am Mittwoch, den 14.04.2021, 00:29 +0200 schrieb Julien Lepiller:
> > Le Tue, 13 Apr 2021 18:58:57 +0200,
> > Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
> >   
> > > Am Montag, den 12.04.2021, 22:09 +0200 schrieb Maxime Devos:  
> > > > I think your original patch is good to go into the git
> > > > repo.  I'll
> > > > open a
> > > > separate bug report about ‘absolutising’ the binaries referred
> > > > to from the scripts.    
> > > I've pushed this patch now, but let us still look for a smaller
> > > solution if applicable.  (That said, I'm not a mysql user and I'm
> > > happy to leave security stuff to lle_bout.)
> > > 
> > > @david: Note, that I did not change the author, meaning it is
> > > committed as "methuselah-0 <david.larsson@selfhosted.xyz>" rather
> > > than "david larsson <david.larsson@selfhosted.xyz>".  Since this
> > > patch is hopefully small enough to not require attribution, that
> > > is fine, but if you plan on making bigger changes, please consider
> > > setting your git up appropriately.
> > > 
> > > Regards,
> > > Leo 
> > > 
> > > 
> > > 
> > >   
> > 
> > Hi!
> > 
> > I saw this patch went through, but it updates the Guix Manual while
> > we
> > are in string freeze. The change is very small and close to the
> > start of string freeze, so I'm willing to let it go. However, please
> > refrain
> > from pushing such changes to the manual. Ideally, this patch should
> > have been pushed yesterday or postponed until after the release.
> > 
> > As a reminder, the string freeze applies to guix and to the manual.
> > It
> > does not apply to the website, package synopsis and descriptions,
> > nor to the cookbook.
> > 
> > Thank you!  
> Ahh, my bad, I totally missed that.  Would it be better to revert this
> change until April 18th and then apply it again?
> 

Yeah, it would be nicer for our translators if you could revert the
change for now. Can you do it?

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Leo Prikler]

Am Mittwoch, den 14.04.2021, 00:56 +0200 schrieb Julien Lepiller:
> Le Wed, 14 Apr 2021 00:38:50 +0200,
> Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
> 
> > Am Mittwoch, den 14.04.2021, 00:29 +0200 schrieb Julien Lepiller:
> > > Le Tue, 13 Apr 2021 18:58:57 +0200,
> > > Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
> > > 
> > > [...]
> > > Hi!
> > > 
> > > I saw this patch went through, but it updates the Guix Manual
> > > while
> > > we
> > > are in string freeze. The change is very small and close to the
> > > start of string freeze, so I'm willing to let it go. However,
> > > please
> > > refrain
> > > from pushing such changes to the manual. Ideally, this patch
> > > should
> > > have been pushed yesterday or postponed until after the release.
> > > 
> > > As a reminder, the string freeze applies to guix and to the
> > > manual.
> > > It
> > > does not apply to the website, package synopsis and descriptions,
> > > nor to the cookbook.
> > > 
> > > Thank you!  
> > Ahh, my bad, I totally missed that.  Would it be better to revert
> > this
> > change until April 18th and then apply it again?
> > 
> 
> Yeah, it would be nicer for our translators if you could revert the
> change for now. Can you do it?
What's the current status w.r.t. string freeze and translations?  Is
this good to go?

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[david larsson]

On 2021-04-19 11:59, Leo Prikler wrote:
> Am Mittwoch, den 14.04.2021, 00:56 +0200 schrieb Julien Lepiller:
>> Le Wed, 14 Apr 2021 00:38:50 +0200,
>> Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
>> 
>> > Am Mittwoch, den 14.04.2021, 00:29 +0200 schrieb Julien Lepiller:
>> > > Le Tue, 13 Apr 2021 18:58:57 +0200,
>> > > Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
>> > >
>> > > [...]
>> > > Hi!
>> > >
>> > > I saw this patch went through, but it updates the Guix Manual
>> > > while
>> > > we
>> > > are in string freeze. The change is very small and close to the
>> > > start of string freeze, so I'm willing to let it go. However,
>> > > please
>> > > refrain
>> > > from pushing such changes to the manual. Ideally, this patch
>> > > should
>> > > have been pushed yesterday or postponed until after the release.
>> > >
>> > > As a reminder, the string freeze applies to guix and to the
>> > > manual.
>> > > It
>> > > does not apply to the website, package synopsis and descriptions,
>> > > nor to the cookbook.
>> > >
>> > > Thank you!
>> > Ahh, my bad, I totally missed that.  Would it be better to revert
>> > this
>> > change until April 18th and then apply it again?
>> >
>> 
>> Yeah, it would be nicer for our translators if you could revert the
>> change for now. Can you do it?
> What's the current status w.r.t. string freeze and translations?  Is
> this good to go?

Ping! :-)

[bug#47704] [PATCH] services: mysql: Add extra-environment as configuration option.

[Leo Prikler]

Am Dienstag, den 27.04.2021, 20:15 +0200 schrieb david larsson:
> On 2021-04-19 11:59, Leo Prikler wrote:
> > Am Mittwoch, den 14.04.2021, 00:56 +0200 schrieb Julien Lepiller:
> > > Le Wed, 14 Apr 2021 00:38:50 +0200,
> > > Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
> > > 
> > > > Am Mittwoch, den 14.04.2021, 00:29 +0200 schrieb Julien
> > > > Lepiller:
> > > > > Le Tue, 13 Apr 2021 18:58:57 +0200,
> > > > > Leo Prikler <leo.prikler@student.tugraz.at> a écrit :
> > > > > 
> > > > > [...]
> > > > > Hi!
> > > > > 
> > > > > I saw this patch went through, but it updates the Guix Manual
> > > > > while
> > > > > we
> > > > > are in string freeze. The change is very small and close to
> > > > > the
> > > > > start of string freeze, so I'm willing to let it go. However,
> > > > > please
> > > > > refrain
> > > > > from pushing such changes to the manual. Ideally, this patch
> > > > > should
> > > > > have been pushed yesterday or postponed until after the
> > > > > release.
> > > > > 
> > > > > As a reminder, the string freeze applies to guix and to the
> > > > > manual.
> > > > > It
> > > > > does not apply to the website, package synopsis and
> > > > > descriptions,
> > > > > nor to the cookbook.
> > > > > 
> > > > > Thank you!
> > > > Ahh, my bad, I totally missed that.  Would it be better to
> > > > revert
> > > > this
> > > > change until April 18th and then apply it again?
> > > > 
> > > 
> > > Yeah, it would be nicer for our translators if you could revert
> > > the
> > > change for now. Can you do it?
> > What's the current status w.r.t. string freeze and
> > translations?  Is
> > this good to go?
> 
> Ping! :-)
Pushed once again, thanks for the reminder!