Package: guix-patches;
Reported by: Christopher Baines <mail <at> cbaines.net>
Date: Sun, 4 Mar 2018 19:12:02 UTC
Severity: normal
Tags: patch
Done: Christopher Baines <mail <at> cbaines.net>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 30701 in the body.
You can then email your comments to 30701 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Sun, 04 Mar 2018 19:12:02 GMT) Full text and rfc822 format available.Christopher Baines <mail <at> cbaines.net>
:guix-patches <at> gnu.org
.
(Sun, 04 Mar 2018 19:12:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: guix-patches <at> gnu.org Subject: [PATCH 0/4] PostgreSQL service changes (add record type, and system test) Date: Sun, 04 Mar 2018 19:10:43 +0000
[Message part 1 (text/plain, inline)]
I was mostly adding the system test, but also ended up reworking the service so that Shepherd knows about the PID file. Christopher Baines (4): services: Rework the PostgreSQL config file to use a record type. services: Use a external pid file for PostgreSQL. tests: databases: Add a system test for PostgreSQL. services: databases: Add postgresql-configuration record exports. gnu/services/databases.scm | 125 ++++++++++++++++++++++++++++++++++++--------- gnu/tests/databases.scm | 59 +++++++++++++++++++++ 2 files changed, 161 insertions(+), 23 deletions(-)
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Sun, 04 Mar 2018 19:17:02 GMT) Full text and rfc822 format available.Message #8 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 3/4] tests: databases: Add a system test for PostgreSQL. Date: Sun, 4 Mar 2018 19:16:32 +0000
* gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables. (run-postgresql-test): New procedure. --- gnu/tests/databases.scm | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm index e7097690a..0597105da 100644 --- a/gnu/tests/databases.scm +++ b/gnu/tests/databases.scm @@ -30,6 +30,7 @@ #:use-module (guix store) #:export (%test-memcached %test-mongodb + %test-postgresql %test-mysql)) (define %memcached-os @@ -208,6 +209,64 @@ (value (run-mongodb-test)))) +;;; +;;; The PostgreSQL service. +;;; + +(define %postgresql-os + (simple-operating-system + (service postgresql-service-type))) + +(define* (run-postgresql-test) + "Run tests in %POSTGRESQL-OS." + (define os + (marionette-operating-system + %postgresql-os + #:imported-modules '((gnu services herd) + (guix combinators)))) + + (define vm + (virtual-machine + (operating-system os) + (memory-size 512))) + + (define test + (with-imported-modules '((gnu build marionette)) + #~(begin + (use-modules (srfi srfi-11) (srfi srfi-64) + (gnu build marionette)) + + (define marionette + (make-marionette (list #$vm))) + + (mkdir #$output) + (chdir #$output) + + (test-begin "postgresql") + + (test-assert "service running" + (marionette-eval + '(begin + (use-modules (gnu services herd)) + (match (start-service 'postgres) + (#f #f) + (('service response-parts ...) + (match (assq-ref response-parts 'running) + ((pid) (number? pid)))))) + marionette)) + + (test-end) + (exit (= (test-runner-fail-count (test-runner-current)) 0))))) + + (gexp->derivation "postgresql-test" test)) + +(define %test-postgresql + (system-test + (name "postgresql") + (description "Start the PostgreSQL service.") + (value (run-postgresql-test)))) + + ;;; ;;; The MySQL service. ;;; -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Sun, 04 Mar 2018 19:17:02 GMT) Full text and rfc822 format available.Message #11 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 4/4] services: databases: Add postgresql-configuration record exports. Date: Sun, 4 Mar 2018 19:16:33 +0000
* gnu/services/databases.scm: Export the record type, and all the field accessors. --- gnu/services/databases.scm | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm index 4090277a7..1e410cd39 100644 --- a/gnu/services/databases.scm +++ b/gnu/services/databases.scm @@ -43,7 +43,16 @@ postgresql-config-file-external-pid-file postgresql-config-file-extra-config + <postgresql-configuration> + postgresql-configuration postgresql-configuration? + postgresql-configuration-postgresql + postgresql-configuration-port + postgresql-configuration-locale + postgresql-configuration-pid-file + postgresql-configuration-file + postgresql-configuration-data-directory + postgresql-service postgresql-service-type -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Sun, 04 Mar 2018 19:17:03 GMT) Full text and rfc822 format available.Message #14 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type. Date: Sun, 4 Mar 2018 19:16:30 +0000
For the default config file representation. This makes it possible to more easily change the configuration file, and have dynamic content. In particular, I'm looking at adding a pid file location to the config file. * gnu/services/databases.scm (<postgresql-config-file>): New record type. (%default-postgres-config): Remove this, it's been replaced by the configuration file. (<postgresql-configuration>): Alter the default for the config file field. (postgresql-service): Alter the default value for the config-file parameter. --- gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm index 3ca8f471f..9ffb6a5e9 100644 --- a/gnu/services/databases.scm +++ b/gnu/services/databases.scm @@ -26,11 +26,20 @@ #:use-module (gnu system shadow) #:use-module (gnu packages admin) #:use-module (gnu packages databases) + #:use-module (guix store) #:use-module (guix modules) #:use-module (guix records) #:use-module (guix gexp) + #:use-module (srfi srfi-1) #:use-module (ice-9 match) - #:export (postgresql-configuration + #:export (<postgresql-config-file> + postgresql-config-file + postgresql-config-file? + postgresql-config-file-log-destination + postgresql-config-file-hba-file + postgresql-config-file-ident-file + postgresql-config-file-extra-config + postgresql-configuration? postgresql-service postgresql-service-type @@ -68,6 +77,60 @@ ;;; ;;; Code: +(define %default-postgres-hba + (plain-file "pg_hba.conf" + " +local all all trust +host all all 127.0.0.1/32 trust +host all all ::1/128 trust")) + +(define %default-postgres-ident + (plain-file "pg_ident.conf" + "# MAPNAME SYSTEM-USERNAME PG-USERNAME")) + +(define-record-type* <postgresql-config-file> + postgresql-config-file make-postgresql-config-file + postgresql-config-file? + (log-destination postgresql-config-file-log-destination + (default "syslog")) + (hba-file postgresql-config-file-hba-file + (default %default-postgres-hba)) + (ident-file postgresql-config-file-ident-file + (default %default-postgres-ident)) + (extra-config postgresql-config-file-extra-config + (default '()))) + +(define-gexp-compiler (postgresql-config-file-compiler + (file <postgresql-config-file>) system target) + (match file + (($ <postgresql-config-file> log-destination hba-file + ident-file extra-config) + (define (quote string) + (if string + (list "'" string "'") + (list))) + + (define contents + (append-map + (match-lambda + ((key) (list)) + ((key . #f) (list)) + ((key values ...) `(,key " = " ,@values "\n"))) + + `(("log_destination" ,@(quote log-destination)) + ("hba_file" ,@(quote hba-file)) + ("ident_file" ,@(quote ident-file)) + ,@extra-config))) + + (gexp->derivation + "postgresql.conf" + #~(call-with-output-file (ungexp output "out") + (lambda (port) + (display + (string-append #$@contents) + port))) + #:local-build? #t)))) + (define-record-type* <postgresql-configuration> postgresql-configuration make-postgresql-configuration postgresql-configuration? @@ -78,27 +141,10 @@ (locale postgresql-configuration-locale (default "en_US.utf8")) (config-file postgresql-configuration-file - (default %default-postgres-config)) + (default (postgresql-config-file))) (data-directory postgresql-configuration-data-directory (default "/var/lib/postgresql/data"))) -(define %default-postgres-hba - (plain-file "pg_hba.conf" - " -local all all trust -host all all 127.0.0.1/32 trust -host all all ::1/128 trust")) - -(define %default-postgres-ident - (plain-file "pg_ident.conf" - "# MAPNAME SYSTEM-USERNAME PG-USERNAME")) - -(define %default-postgres-config - (mixed-text-file "postgresql.conf" - "log_destination = 'syslog'\n" - "hba_file = '" %default-postgres-hba "'\n" - "ident_file = '" %default-postgres-ident "'\n")) - (define %postgresql-accounts (list (user-group (name "postgres") (system? #t)) (user-account @@ -192,7 +238,7 @@ host all all ::1/128 trust")) (define* (postgresql-service #:key (postgresql postgresql) (port 5432) (locale "en_US.utf8") - (config-file %default-postgres-config) + (config-file (postgresql-config-file)) (data-directory "/var/lib/postgresql/data")) "Return a service that runs @var{postgresql}, the PostgreSQL database server. -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Sun, 04 Mar 2018 19:17:03 GMT) Full text and rfc822 format available.Message #17 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 2/4] services: Use a external pid file for PostgreSQL. Date: Sun, 4 Mar 2018 19:16:31 +0000
* gnu/services/databases.scm (postgresql-pid-file-for-version): New procedure. (<postgresql-config-file>): Add a new external-pid-file field. (postgresql-config-file-compiler): Add support for the external-pid-file. (postgresql-activation): Create the directory for the pid file. (postgresql-shepherd-service): Use the pid-file when starting the service. --- gnu/services/databases.scm | 48 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm index 9ffb6a5e9..4090277a7 100644 --- a/gnu/services/databases.scm +++ b/gnu/services/databases.scm @@ -26,10 +26,12 @@ #:use-module (gnu system shadow) #:use-module (gnu packages admin) #:use-module (gnu packages databases) + #:use-module (guix packages) #:use-module (guix store) #:use-module (guix modules) #:use-module (guix records) #:use-module (guix gexp) + #:use-module (guix utils) #:use-module (srfi srfi-1) #:use-module (ice-9 match) #:export (<postgresql-config-file> @@ -38,6 +40,7 @@ postgresql-config-file-log-destination postgresql-config-file-hba-file postgresql-config-file-ident-file + postgresql-config-file-external-pid-file postgresql-config-file-extra-config postgresql-configuration? @@ -88,23 +91,32 @@ host all all ::1/128 trust")) (plain-file "pg_ident.conf" "# MAPNAME SYSTEM-USERNAME PG-USERNAME")) +(define (postgresql-pid-file-for-version version) + (string-append "/var/run/postgresql/" + (version-major+minor version) + "-main.pid")) + (define-record-type* <postgresql-config-file> postgresql-config-file make-postgresql-config-file postgresql-config-file? - (log-destination postgresql-config-file-log-destination - (default "syslog")) - (hba-file postgresql-config-file-hba-file - (default %default-postgres-hba)) - (ident-file postgresql-config-file-ident-file - (default %default-postgres-ident)) - (extra-config postgresql-config-file-extra-config - (default '()))) + (log-destination postgresql-config-file-log-destination + (default "syslog")) + (hba-file postgresql-config-file-hba-file + (default %default-postgres-hba)) + (ident-file postgresql-config-file-ident-file + (default %default-postgres-ident)) + (external-pid-file postgresql-config-file-external-pid-file + (default (postgresql-pid-file-for-version + (package-version postgresql)))) + (extra-config postgresql-config-file-extra-config + (default '()))) (define-gexp-compiler (postgresql-config-file-compiler (file <postgresql-config-file>) system target) (match file (($ <postgresql-config-file> log-destination hba-file - ident-file extra-config) + ident-file external-pid-file + extra-config) (define (quote string) (if string (list "'" string "'") @@ -120,6 +132,7 @@ host all all ::1/128 trust")) `(("log_destination" ,@(quote log-destination)) ("hba_file" ,@(quote hba-file)) ("ident_file" ,@(quote ident-file)) + ("external_pid_file" ,@(quote external-pid-file)) ,@extra-config))) (gexp->derivation @@ -140,6 +153,9 @@ host all all ::1/128 trust")) (default 5432)) (locale postgresql-configuration-locale (default "en_US.utf8")) + (pid-file postgresql-configuration-pid-file + (default (postgresql-pid-file-for-version + (package-version postgresql)))) (config-file postgresql-configuration-file (default (postgresql-config-file))) (data-directory postgresql-configuration-data-directory @@ -157,7 +173,8 @@ host all all ::1/128 trust")) (define postgresql-activation (match-lambda - (($ <postgresql-configuration> postgresql port locale config-file data-directory) + (($ <postgresql-configuration> postgresql port locale pid-file + config-file data-directory) #~(begin (use-modules (guix build utils) (ice-9 match)) @@ -173,6 +190,10 @@ host all all ::1/128 trust")) (mkdir-p #$data-directory) (chown #$data-directory (passwd:uid user) (passwd:gid user)) + ;; Create a directory for the pid file + (mkdir-p #$(dirname pid-file)) + (chown #$(dirname pid-file) (passwd:uid user) (passwd:gid user)) + ;; Drop privileges and init state directory in a new ;; process. Wait for it to finish before proceeding. (match (primitive-fork) @@ -195,7 +216,8 @@ host all all ::1/128 trust")) (define postgresql-shepherd-service (match-lambda - (($ <postgresql-configuration> postgresql port locale config-file data-directory) + (($ <postgresql-configuration> postgresql port locale pid-file + config-file data-directory) (let* ((pg_ctl-wrapper ;; Wrapper script that switches to the 'postgres' user before ;; launching daemon. @@ -221,7 +243,9 @@ host all all ::1/128 trust")) (provision '(postgres)) (documentation "Run the PostgreSQL daemon.") (requirement '(user-processes loopback syslogd)) - (start (action "start")) + (start #~(make-forkexec-constructor + '(#$pg_ctl-wrapper "start") + #:pid-file #$pid-file)) (stop (action "stop")))))))) (define postgresql-service-type -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 00:33:02 GMT) Full text and rfc822 format available.Message #20 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type. Date: Mon, 05 Mar 2018 01:32:10 +0100
Hi Christopher, Christopher Baines <mail <at> cbaines.net> writes: > For the default config file representation. This makes it possible to more > easily change the configuration file, and have dynamic content. In particular, > I'm looking at adding a pid file location to the config file. > > * gnu/services/databases.scm (<postgresql-config-file>): New record type. > (%default-postgres-config): Remove this, it's been replaced by the > configuration file. > (<postgresql-configuration>): Alter the default for the config file field. > (postgresql-service): Alter the default value for the config-file parameter. > --- > gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- > 1 file changed, 66 insertions(+), 20 deletions(-) Thank you for this work! > +(define-gexp-compiler (postgresql-config-file-compiler > + (file <postgresql-config-file>) system target) > + (match file > + (($ <postgresql-config-file> log-destination hba-file > + ident-file extra-config) > + (define (quote string) > + (if string > + (list "'" string "'") > + (list))) I don't think it's a good thing to hide one of the most important lisp functions :-).
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 00:33:02 GMT) Full text and rfc822 format available.Message #23 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 2/4] services: Use a external pid file for PostgreSQL. Date: Mon, 05 Mar 2018 01:32:18 +0100
Christopher Baines <mail <at> cbaines.net> writes: > * gnu/services/databases.scm (postgresql-pid-file-for-version): New procedure. > (<postgresql-config-file>): Add a new external-pid-file field. > (postgresql-config-file-compiler): Add support for the external-pid-file. > (postgresql-activation): Create the directory for the pid file. > (postgresql-shepherd-service): Use the pid-file when starting the service. > --- > gnu/services/databases.scm | 48 ++++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 12 deletions(-) ... > +(define (postgresql-pid-file-for-version version) > + (string-append "/var/run/postgresql/" > + (version-major+minor version) > + "-main.pid")) > + > (define-record-type* <postgresql-config-file> > postgresql-config-file make-postgresql-config-file > postgresql-config-file? > - (log-destination postgresql-config-file-log-destination > - (default "syslog")) > - (hba-file postgresql-config-file-hba-file > - (default %default-postgres-hba)) > - (ident-file postgresql-config-file-ident-file > - (default %default-postgres-ident)) > - (extra-config postgresql-config-file-extra-config > - (default '()))) > + (log-destination postgresql-config-file-log-destination > + (default "syslog")) > + (hba-file postgresql-config-file-hba-file > + (default %default-postgres-hba)) > + (ident-file postgresql-config-file-ident-file > + (default %default-postgres-ident)) > + (external-pid-file postgresql-config-file-external-pid-file > + (default (postgresql-pid-file-for-version > + (package-version postgresql)))) Why does external-pid-file have a default value. It's optional, and the user already has access to $DATA/postmaster.pid anyway. > @@ -140,6 +153,9 @@ host all all ::1/128 trust")) > (default 5432)) > (locale postgresql-configuration-locale > (default "en_US.utf8")) > + (pid-file postgresql-configuration-pid-file > + (default (postgresql-pid-file-for-version > + (package-version postgresql)))) The main PID file is chosen by Postgres, and put at $DATA/postmaster.pid. I don't think it's customizable. This setting (pid-file) probably doesn't affect the daemon the way you think it does. > (config-file postgresql-configuration-file > (default (postgresql-config-file))) > (data-directory postgresql-configuration-data-directory > @@ -157,7 +173,8 @@ host all all ::1/128 trust")) > > + ;; Create a directory for the pid file > + (mkdir-p #$(dirname pid-file)) > + (chown #$(dirname pid-file) (passwd:uid user) (passwd:gid user)) I think it would make more sense to create the directory for the external-pid-file. > (define postgresql-shepherd-service > (match-lambda > - (($ <postgresql-configuration> postgresql port locale config-file data-directory) > + (($ <postgresql-configuration> postgresql port locale pid-file > + config-file data-directory) > (let* ((pg_ctl-wrapper > ;; Wrapper script that switches to the 'postgres' user before > ;; launching daemon. > @@ -221,7 +243,9 @@ host all all ::1/128 trust")) > (provision '(postgres)) > (documentation "Run the PostgreSQL daemon.") > (requirement '(user-processes loopback syslogd)) > - (start (action "start")) > + (start #~(make-forkexec-constructor > + '(#$pg_ctl-wrapper "start") > + #:pid-file #$pid-file)) > (stop (action "stop")))))))) If pid-file != external-pid-file, Sherpherd will wait for a file that doesn't exist won't it? Because Postgresql will never be aware of that pid-file. Plus, there is no reason to use make-forkexec-constructor on pg_ctl because pg_ctl returns after it has checked that the daemon is running. For the same reason, Shepherd doesn't need to know about Postgres' PID. All the hard work is done by pg_ctl.
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 00:33:02 GMT) Full text and rfc822 format available.Message #26 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 3/4] tests: databases: Add a system test for PostgreSQL. Date: Mon, 05 Mar 2018 01:32:32 +0100
Christopher Baines <mail <at> cbaines.net> writes: > * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables. > (run-postgresql-test): New procedure. > --- > gnu/tests/databases.scm | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) ... > + (test-assert "service running" > + (marionette-eval > + '(begin > + (use-modules (gnu services herd)) > + (match (start-service 'postgres) > + (#f #f) > + (('service response-parts ...) > + (match (assq-ref response-parts 'running) > + ((pid) (number? pid)))))) > + marionette)) I don't understand the point of the PID check here. pg_ctl will ensure that the daemon has started (by checking its PID), so I don't think there is any need to redo its work. I guess the PID you'll get here is the one of pg_ctl, which is probably not what you want. I believe that (start-service 'postgres) returning true means pg_ctl succeeded in its check that the daemon is running. So this is probably enough: (test-assert "service running" (marionette-eval '(begin (use-modules (gnu services herd)) (start-service 'postgres)) marionette)) Clément
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 07:53:01 GMT) Full text and rfc822 format available.Message #29 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type. Date: Mon, 05 Mar 2018 07:52:25 +0000
[Message part 1 (text/plain, inline)]
Clément Lassieur <clement <at> lassieur.org> writes: > Hi Christopher, > > Christopher Baines <mail <at> cbaines.net> writes: > >> For the default config file representation. This makes it possible to more >> easily change the configuration file, and have dynamic content. In particular, >> I'm looking at adding a pid file location to the config file. >> >> * gnu/services/databases.scm (<postgresql-config-file>): New record type. >> (%default-postgres-config): Remove this, it's been replaced by the >> configuration file. >> (<postgresql-configuration>): Alter the default for the config file field. >> (postgresql-service): Alter the default value for the config-file parameter. >> --- >> gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 66 insertions(+), 20 deletions(-) > > Thank you for this work! No problem, I've finally got around to going through some patches I've had sitting around for a while. >> +(define-gexp-compiler (postgresql-config-file-compiler >> + (file <postgresql-config-file>) system target) >> + (match file >> + (($ <postgresql-config-file> log-destination hba-file >> + ident-file extra-config) >> + (define (quote string) >> + (if string >> + (list "'" string "'") >> + (list))) > > I don't think it's a good thing to hide one of the most important lisp > functions :-). I don't quite follow. I was trying to use '() rather than (list) if that is what you mean, but I kept getting odd errors from Guile, so I gave up, and ended up going with this.
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 08:22:02 GMT) Full text and rfc822 format available.Message #32 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 2/4] services: Use a external pid file for PostgreSQL. Date: Mon, 05 Mar 2018 08:21:35 +0000
[Message part 1 (text/plain, inline)]
Clément Lassieur <clement <at> lassieur.org> writes: > Christopher Baines <mail <at> cbaines.net> writes: > >> * gnu/services/databases.scm (postgresql-pid-file-for-version): New procedure. >> (<postgresql-config-file>): Add a new external-pid-file field. >> (postgresql-config-file-compiler): Add support for the external-pid-file. >> (postgresql-activation): Create the directory for the pid file. >> (postgresql-shepherd-service): Use the pid-file when starting the service. >> --- >> gnu/services/databases.scm | 48 ++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 36 insertions(+), 12 deletions(-) > > ... > >> +(define (postgresql-pid-file-for-version version) >> + (string-append "/var/run/postgresql/" >> + (version-major+minor version) >> + "-main.pid")) >> + >> (define-record-type* <postgresql-config-file> >> postgresql-config-file make-postgresql-config-file >> postgresql-config-file? >> - (log-destination postgresql-config-file-log-destination >> - (default "syslog")) >> - (hba-file postgresql-config-file-hba-file >> - (default %default-postgres-hba)) >> - (ident-file postgresql-config-file-ident-file >> - (default %default-postgres-ident)) >> - (extra-config postgresql-config-file-extra-config >> - (default '()))) >> + (log-destination postgresql-config-file-log-destination >> + (default "syslog")) >> + (hba-file postgresql-config-file-hba-file >> + (default %default-postgres-hba)) >> + (ident-file postgresql-config-file-ident-file >> + (default %default-postgres-ident)) >> + (external-pid-file postgresql-config-file-external-pid-file >> + (default (postgresql-pid-file-for-version >> + (package-version postgresql)))) > > Why does external-pid-file have a default value. It's optional, and the > user already has access to $DATA/postmaster.pid anyway. I ended up adding this as I was writing the system test. I was coping previous tests that I have written, in which I've been checking that the shepherd gets the PID back when it starts the process. Before I set out in writing this, I didn't realise that PostgreSQL stores the PID in the data directory by default, or that pg_ctl waits for actions to complete by default either. I'm not particularly attached to this patch, I guess it mostly offers consistency with other services. >> @@ -140,6 +153,9 @@ host all all ::1/128 trust")) >> (default 5432)) >> (locale postgresql-configuration-locale >> (default "en_US.utf8")) >> + (pid-file postgresql-configuration-pid-file >> + (default (postgresql-pid-file-for-version >> + (package-version postgresql)))) > > The main PID file is chosen by Postgres, and put at > $DATA/postmaster.pid. I don't think it's customizable. This setting > (pid-file) probably doesn't affect the daemon the way you think it does. This part of the configuration is just meant to be where the Guix part of the service expects to find the pid-file (if one is used, and it's not #f). >> (config-file postgresql-configuration-file >> (default (postgresql-config-file))) >> (data-directory postgresql-configuration-data-directory >> @@ -157,7 +173,8 @@ host all all ::1/128 trust")) >> >> + ;; Create a directory for the pid file >> + (mkdir-p #$(dirname pid-file)) >> + (chown #$(dirname pid-file) (passwd:uid user) (passwd:gid user)) > > I think it would make more sense to create the directory for the > external-pid-file. As far as I understand it, this is what this does. >> (define postgresql-shepherd-service >> (match-lambda >> - (($ <postgresql-configuration> postgresql port locale config-file data-directory) >> + (($ <postgresql-configuration> postgresql port locale pid-file >> + config-file data-directory) >> (let* ((pg_ctl-wrapper >> ;; Wrapper script that switches to the 'postgres' user before >> ;; launching daemon. >> @@ -221,7 +243,9 @@ host all all ::1/128 trust")) >> (provision '(postgres)) >> (documentation "Run the PostgreSQL daemon.") >> (requirement '(user-processes loopback syslogd)) >> - (start (action "start")) >> + (start #~(make-forkexec-constructor >> + '(#$pg_ctl-wrapper "start") >> + #:pid-file #$pid-file)) >> (stop (action "stop")))))))) > > If pid-file != external-pid-file, Sherpherd will wait for a file that > doesn't exist won't it? Because Postgresql will never be aware of that > pid-file. Yep. > Plus, there is no reason to use make-forkexec-constructor on pg_ctl > because pg_ctl returns after it has checked that the daemon is running. > For the same reason, Shepherd doesn't need to know about Postgres' PID. > All the hard work is done by pg_ctl. As the comment I made at the top, I did this when I was writing the system test. If you remove this patch, when you call (start-service 'postgres), it will return #f if the service starts successfully. If you tweak the service to make it fail to start (e.g. by changing the "start" action to something else), you get the same observable behaviour, start-service returns #f. The way this works for other services, normally through make-forkexec-constructor is that calling start-service will return the PID. While the system test does still add some value even without checking if the service has started, doing so would be really good. Even if it's not using the PID file approach, maybe the exit code of pg_ctl could be used? I'm not really sure why it isn't working like that already, as invoke usually returns either #t or #f...
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 08:28:02 GMT) Full text and rfc822 format available.Message #35 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 2/4] services: Use a external pid file for PostgreSQL. Date: Mon, 05 Mar 2018 08:27:55 +0000
[Message part 1 (text/plain, inline)]
Christopher Baines <mail <at> cbaines.net> writes: > Clément Lassieur <clement <at> lassieur.org> writes: > >> Plus, there is no reason to use make-forkexec-constructor on pg_ctl >> because pg_ctl returns after it has checked that the daemon is running. >> For the same reason, Shepherd doesn't need to know about Postgres' PID. >> All the hard work is done by pg_ctl. > > As the comment I made at the top, I did this when I was writing the > system test. If you remove this patch, when you call (start-service > 'postgres), it will return #f if the service starts successfully. If you > tweak the service to make it fail to start (e.g. by changing the "start" > action to something else), you get the same observable behaviour, > start-service returns #f. > > The way this works for other services, normally through > make-forkexec-constructor is that calling start-service will return the > PID. > > While the system test does still add some value even without checking if > the service has started, doing so would be really good. Even if it's not > using the PID file approach, maybe the exit code of pg_ctl could be > used? I'm not really sure why it isn't working like that already, as > invoke usually returns either #t or #f... Ah, I've just realised why this is the case, I was misreading the system test results, it does actually return #t/#f, but as the system test was expecting a number, it just returns #f to indicate failure.
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 08:35:02 GMT) Full text and rfc822 format available.Message #38 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 3/4] tests: databases: Add a system test for PostgreSQL. Date: Mon, 05 Mar 2018 08:34:33 +0000
[Message part 1 (text/plain, inline)]
Clément Lassieur <clement <at> lassieur.org> writes: > Christopher Baines <mail <at> cbaines.net> writes: > >> * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables. >> (run-postgresql-test): New procedure. >> --- >> gnu/tests/databases.scm | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) > ... > >> + (test-assert "service running" >> + (marionette-eval >> + '(begin >> + (use-modules (gnu services herd)) >> + (match (start-service 'postgres) >> + (#f #f) >> + (('service response-parts ...) >> + (match (assq-ref response-parts 'running) >> + ((pid) (number? pid)))))) >> + marionette)) > > I don't understand the point of the PID check here. pg_ctl will ensure > that the daemon has started (by checking its PID), so I don't think > there is any need to redo its work. I guess the PID you'll get here is > the one of pg_ctl, which is probably not what you want. Because of make-forkexec-constructor, it is the main PID as set in the external pid file created by PostgreSQL. > I believe that (start-service 'postgres) returning true means pg_ctl > succeeded in its check that the daemon is running. So this is probably > enough: > > (test-assert "service running" > (marionette-eval > '(begin > (use-modules (gnu services herd)) > (start-service 'postgres)) > marionette)) Sure, I'm happy with this. I'll send some new patches soonish.
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 08:40:01 GMT) Full text and rfc822 format available.Message #41 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 2/3] tests: databases: Add a system test for PostgreSQL. Date: Mon, 5 Mar 2018 08:39:16 +0000
* gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables. (run-postgresql-test): New procedure. --- gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm index e7097690a..583f484d7 100644 --- a/gnu/tests/databases.scm +++ b/gnu/tests/databases.scm @@ -30,6 +30,7 @@ #:use-module (guix store) #:export (%test-memcached %test-mongodb + %test-postgresql %test-mysql)) (define %memcached-os @@ -208,6 +209,60 @@ (value (run-mongodb-test)))) +;;; +;;; The PostgreSQL service. +;;; + +(define %postgresql-os + (simple-operating-system + (service postgresql-service-type))) + +(define* (run-postgresql-test) + "Run tests in %POSTGRESQL-OS." + (define os + (marionette-operating-system + %postgresql-os + #:imported-modules '((gnu services herd) + (guix combinators)))) + + (define vm + (virtual-machine + (operating-system os) + (memory-size 512))) + + (define test + (with-imported-modules '((gnu build marionette)) + #~(begin + (use-modules (srfi srfi-11) (srfi srfi-64) + (gnu build marionette)) + + (define marionette + (make-marionette (list #$vm))) + + (mkdir #$output) + (chdir #$output) + + (test-begin "postgresql") + + (test-assert "service running" + (marionette-eval + '(begin + (use-modules (gnu services herd)) + (start-service 'postgres)) + marionette)) + + (test-end) + (exit (= (test-runner-fail-count (test-runner-current)) 0))))) + + (gexp->derivation "postgresql-test" test)) + +(define %test-postgresql + (system-test + (name "postgresql") + (description "Start the PostgreSQL service.") + (value (run-postgresql-test)))) + + ;;; ;;; The MySQL service. ;;; -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 08:40:02 GMT) Full text and rfc822 format available.Message #44 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 3/3] services: databases: Add postgresql-configuration record exports. Date: Mon, 5 Mar 2018 08:39:17 +0000
* gnu/services/databases.scm: Export the record type, and all the field accessors. --- gnu/services/databases.scm | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm index 9ffb6a5e9..b05c0442f 100644 --- a/gnu/services/databases.scm +++ b/gnu/services/databases.scm @@ -40,7 +40,15 @@ postgresql-config-file-ident-file postgresql-config-file-extra-config + <postgresql-configuration> + postgresql-configuration postgresql-configuration? + postgresql-configuration-postgresql + postgresql-configuration-port + postgresql-configuration-locale + postgresql-configuration-file + postgresql-configuration-data-directory + postgresql-service postgresql-service-type -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 08:40:03 GMT) Full text and rfc822 format available.Message #47 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type. Date: Mon, 5 Mar 2018 08:39:15 +0000
For the default config file representation. This makes it possible to more easily change the configuration file, and have dynamic content. In particular, I'm looking at adding a pid file location to the config file. * gnu/services/databases.scm (<postgresql-config-file>): New record type. (%default-postgres-config): Remove this, it's been replaced by the configuration file. (<postgresql-configuration>): Alter the default for the config file field. (postgresql-service): Alter the default value for the config-file parameter. --- gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm index 3ca8f471f..9ffb6a5e9 100644 --- a/gnu/services/databases.scm +++ b/gnu/services/databases.scm @@ -26,11 +26,20 @@ #:use-module (gnu system shadow) #:use-module (gnu packages admin) #:use-module (gnu packages databases) + #:use-module (guix store) #:use-module (guix modules) #:use-module (guix records) #:use-module (guix gexp) + #:use-module (srfi srfi-1) #:use-module (ice-9 match) - #:export (postgresql-configuration + #:export (<postgresql-config-file> + postgresql-config-file + postgresql-config-file? + postgresql-config-file-log-destination + postgresql-config-file-hba-file + postgresql-config-file-ident-file + postgresql-config-file-extra-config + postgresql-configuration? postgresql-service postgresql-service-type @@ -68,6 +77,60 @@ ;;; ;;; Code: +(define %default-postgres-hba + (plain-file "pg_hba.conf" + " +local all all trust +host all all 127.0.0.1/32 trust +host all all ::1/128 trust")) + +(define %default-postgres-ident + (plain-file "pg_ident.conf" + "# MAPNAME SYSTEM-USERNAME PG-USERNAME")) + +(define-record-type* <postgresql-config-file> + postgresql-config-file make-postgresql-config-file + postgresql-config-file? + (log-destination postgresql-config-file-log-destination + (default "syslog")) + (hba-file postgresql-config-file-hba-file + (default %default-postgres-hba)) + (ident-file postgresql-config-file-ident-file + (default %default-postgres-ident)) + (extra-config postgresql-config-file-extra-config + (default '()))) + +(define-gexp-compiler (postgresql-config-file-compiler + (file <postgresql-config-file>) system target) + (match file + (($ <postgresql-config-file> log-destination hba-file + ident-file extra-config) + (define (quote string) + (if string + (list "'" string "'") + (list))) + + (define contents + (append-map + (match-lambda + ((key) (list)) + ((key . #f) (list)) + ((key values ...) `(,key " = " ,@values "\n"))) + + `(("log_destination" ,@(quote log-destination)) + ("hba_file" ,@(quote hba-file)) + ("ident_file" ,@(quote ident-file)) + ,@extra-config))) + + (gexp->derivation + "postgresql.conf" + #~(call-with-output-file (ungexp output "out") + (lambda (port) + (display + (string-append #$@contents) + port))) + #:local-build? #t)))) + (define-record-type* <postgresql-configuration> postgresql-configuration make-postgresql-configuration postgresql-configuration? @@ -78,27 +141,10 @@ (locale postgresql-configuration-locale (default "en_US.utf8")) (config-file postgresql-configuration-file - (default %default-postgres-config)) + (default (postgresql-config-file))) (data-directory postgresql-configuration-data-directory (default "/var/lib/postgresql/data"))) -(define %default-postgres-hba - (plain-file "pg_hba.conf" - " -local all all trust -host all all 127.0.0.1/32 trust -host all all ::1/128 trust")) - -(define %default-postgres-ident - (plain-file "pg_ident.conf" - "# MAPNAME SYSTEM-USERNAME PG-USERNAME")) - -(define %default-postgres-config - (mixed-text-file "postgresql.conf" - "log_destination = 'syslog'\n" - "hba_file = '" %default-postgres-hba "'\n" - "ident_file = '" %default-postgres-ident "'\n")) - (define %postgresql-accounts (list (user-group (name "postgres") (system? #t)) (user-account @@ -192,7 +238,7 @@ host all all ::1/128 trust")) (define* (postgresql-service #:key (postgresql postgresql) (port 5432) (locale "en_US.utf8") - (config-file %default-postgres-config) + (config-file (postgresql-config-file)) (data-directory "/var/lib/postgresql/data")) "Return a service that runs @var{postgresql}, the PostgreSQL database server. -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 08:53:01 GMT) Full text and rfc822 format available.Message #50 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type. Date: Mon, 05 Mar 2018 09:52:11 +0100
Christopher Baines <mail <at> cbaines.net> writes: > Clément Lassieur <clement <at> lassieur.org> writes: > >> Hi Christopher, >> >> Christopher Baines <mail <at> cbaines.net> writes: >> >>> For the default config file representation. This makes it possible to more >>> easily change the configuration file, and have dynamic content. In particular, >>> I'm looking at adding a pid file location to the config file. >>> >>> * gnu/services/databases.scm (<postgresql-config-file>): New record type. >>> (%default-postgres-config): Remove this, it's been replaced by the >>> configuration file. >>> (<postgresql-configuration>): Alter the default for the config file field. >>> (postgresql-service): Alter the default value for the config-file parameter. >>> --- >>> gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- >>> 1 file changed, 66 insertions(+), 20 deletions(-) >> >> Thank you for this work! > > No problem, I've finally got around to going through some patches I've > had sitting around for a while. > >>> +(define-gexp-compiler (postgresql-config-file-compiler >>> + (file <postgresql-config-file>) system target) >>> + (match file >>> + (($ <postgresql-config-file> log-destination hba-file >>> + ident-file extra-config) >>> + (define (quote string) >>> + (if string >>> + (list "'" string "'") >>> + (list))) >> >> I don't think it's a good thing to hide one of the most important lisp >> functions :-). > > I don't quite follow. I was trying to use '() rather than (list) if that > is what you mean, but I kept getting odd errors from Guile, so I gave > up, and ended up going with this. Sorry for not being clear. I meant that if you define 'quote', you won't be able to use the Guile 'quote' anymore. See https://www.gnu.org/software/guile/manual/html_node/Expression-Syntax.html. You could just rename it to 'quote-val' or something else.
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 09:36:01 GMT) Full text and rfc822 format available.Message #53 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 1/4] services: Rework the PostgreSQL config file to use a record type. Date: Mon, 05 Mar 2018 10:35:49 +0100
Christopher Baines <mail <at> cbaines.net> writes: > Clément Lassieur <clement <at> lassieur.org> writes: > >> Hi Christopher, >> >> Christopher Baines <mail <at> cbaines.net> writes: >> >>> For the default config file representation. This makes it possible to more >>> easily change the configuration file, and have dynamic content. In particular, >>> I'm looking at adding a pid file location to the config file. >>> >>> * gnu/services/databases.scm (<postgresql-config-file>): New record type. >>> (%default-postgres-config): Remove this, it's been replaced by the >>> configuration file. >>> (<postgresql-configuration>): Alter the default for the config file field. >>> (postgresql-service): Alter the default value for the config-file parameter. >>> --- >>> gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- >>> 1 file changed, 66 insertions(+), 20 deletions(-) >> >> Thank you for this work! > > No problem, I've finally got around to going through some patches I've > had sitting around for a while. > >>> +(define-gexp-compiler (postgresql-config-file-compiler >>> + (file <postgresql-config-file>) system target) >>> + (match file >>> + (($ <postgresql-config-file> log-destination hba-file >>> + ident-file extra-config) >>> + (define (quote string) >>> + (if string >>> + (list "'" string "'") >>> + (list))) >> >> I don't think it's a good thing to hide one of the most important lisp >> functions :-). > > I don't quite follow. I was trying to use '() rather than (list) if that > is what you mean, but I kept getting odd errors from Guile, so I gave > up, and ended up going with this. You couldn't use '() because '() is the same thing as (quote ()) and you redefined 'quote'.
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 11:55:01 GMT) Full text and rfc822 format available.Message #56 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 2/3] tests: databases: Add a system test for PostgreSQL. Date: Mon, 05 Mar 2018 12:54:53 +0100
Christopher Baines <mail <at> cbaines.net> writes: > * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables. > (run-postgresql-test): New procedure. > --- > gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm > index e7097690a..583f484d7 100644 > --- a/gnu/tests/databases.scm > +++ b/gnu/tests/databases.scm > @@ -30,6 +30,7 @@ > #:use-module (guix store) > #:export (%test-memcached > %test-mongodb > + %test-postgresql > %test-mysql)) > > (define %memcached-os > @@ -208,6 +209,60 @@ > (value (run-mongodb-test)))) > > > +;;; > +;;; The PostgreSQL service. > +;;; > + > +(define %postgresql-os > + (simple-operating-system > + (service postgresql-service-type))) > + > +(define* (run-postgresql-test) define, instead of define* > + "Run tests in %POSTGRESQL-OS." > + (define os > + (marionette-operating-system > + %postgresql-os > + #:imported-modules '((gnu services herd) > + (guix combinators)))) > + > + (define vm > + (virtual-machine > + (operating-system os) > + (memory-size 512))) > + > + (define test > + (with-imported-modules '((gnu build marionette)) > + #~(begin > + (use-modules (srfi srfi-11) (srfi srfi-64) I think srfi-11 is useless. > + (gnu build marionette)) > + > + (define marionette > + (make-marionette (list #$vm))) > + > + (mkdir #$output) > + (chdir #$output) > + > + (test-begin "postgresql") > + > + (test-assert "service running" > + (marionette-eval > + '(begin > + (use-modules (gnu services herd)) > + (start-service 'postgres)) > + marionette)) > + > + (test-end) > + (exit (= (test-runner-fail-count (test-runner-current)) 0))))) > + > + (gexp->derivation "postgresql-test" test)) > + > +(define %test-postgresql > + (system-test > + (name "postgresql") > + (description "Start the PostgreSQL service.") > + (value (run-postgresql-test)))) > + > + > ;;; > ;;; The MySQL service. > ;;;
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 12:16:02 GMT) Full text and rfc822 format available.Message #59 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 2/4] services: Use a external pid file for PostgreSQL. Date: Mon, 05 Mar 2018 13:15:36 +0100
Christopher Baines <mail <at> cbaines.net> writes: > Clément Lassieur <clement <at> lassieur.org> writes: > >> Christopher Baines <mail <at> cbaines.net> writes: >> >>> * gnu/services/databases.scm (postgresql-pid-file-for-version): New procedure. >>> (<postgresql-config-file>): Add a new external-pid-file field. >>> (postgresql-config-file-compiler): Add support for the external-pid-file. >>> (postgresql-activation): Create the directory for the pid file. >>> (postgresql-shepherd-service): Use the pid-file when starting the service. >>> --- >>> gnu/services/databases.scm | 48 ++++++++++++++++++++++++++++++++++------------ >>> 1 file changed, 36 insertions(+), 12 deletions(-) >> >> ... >> >>> +(define (postgresql-pid-file-for-version version) >>> + (string-append "/var/run/postgresql/" >>> + (version-major+minor version) >>> + "-main.pid")) >>> + >>> (define-record-type* <postgresql-config-file> >>> postgresql-config-file make-postgresql-config-file >>> postgresql-config-file? >>> - (log-destination postgresql-config-file-log-destination >>> - (default "syslog")) >>> - (hba-file postgresql-config-file-hba-file >>> - (default %default-postgres-hba)) >>> - (ident-file postgresql-config-file-ident-file >>> - (default %default-postgres-ident)) >>> - (extra-config postgresql-config-file-extra-config >>> - (default '()))) >>> + (log-destination postgresql-config-file-log-destination >>> + (default "syslog")) >>> + (hba-file postgresql-config-file-hba-file >>> + (default %default-postgres-hba)) >>> + (ident-file postgresql-config-file-ident-file >>> + (default %default-postgres-ident)) >>> + (external-pid-file postgresql-config-file-external-pid-file >>> + (default (postgresql-pid-file-for-version >>> + (package-version postgresql)))) >> >> Why does external-pid-file have a default value. It's optional, and the >> user already has access to $DATA/postmaster.pid anyway. > > I ended up adding this as I was writing the system test. I was coping > previous tests that I have written, in which I've been checking that the > shepherd gets the PID back when it starts the process. > > Before I set out in writing this, I didn't realise that PostgreSQL > stores the PID in the data directory by default, or that pg_ctl waits > for actions to complete by default either. > > I'm not particularly attached to this patch, I guess it mostly offers > consistency with other services. > >>> @@ -140,6 +153,9 @@ host all all ::1/128 trust")) >>> (default 5432)) >>> (locale postgresql-configuration-locale >>> (default "en_US.utf8")) >>> + (pid-file postgresql-configuration-pid-file >>> + (default (postgresql-pid-file-for-version >>> + (package-version postgresql)))) >> >> The main PID file is chosen by Postgres, and put at >> $DATA/postmaster.pid. I don't think it's customizable. This setting >> (pid-file) probably doesn't affect the daemon the way you think it does. > > This part of the configuration is just meant to be where the Guix part > of the service expects to find the pid-file (if one is used, and it's > not #f). > >>> (config-file postgresql-configuration-file >>> (default (postgresql-config-file))) >>> (data-directory postgresql-configuration-data-directory >>> @@ -157,7 +173,8 @@ host all all ::1/128 trust")) >>> >>> + ;; Create a directory for the pid file >>> + (mkdir-p #$(dirname pid-file)) >>> + (chown #$(dirname pid-file) (passwd:uid user) (passwd:gid user)) >> >> I think it would make more sense to create the directory for the >> external-pid-file. > > As far as I understand it, this is what this does. > >>> (define postgresql-shepherd-service >>> (match-lambda >>> - (($ <postgresql-configuration> postgresql port locale config-file data-directory) >>> + (($ <postgresql-configuration> postgresql port locale pid-file >>> + config-file data-directory) >>> (let* ((pg_ctl-wrapper >>> ;; Wrapper script that switches to the 'postgres' user before >>> ;; launching daemon. >>> @@ -221,7 +243,9 @@ host all all ::1/128 trust")) >>> (provision '(postgres)) >>> (documentation "Run the PostgreSQL daemon.") >>> (requirement '(user-processes loopback syslogd)) >>> - (start (action "start")) >>> + (start #~(make-forkexec-constructor >>> + '(#$pg_ctl-wrapper "start") >>> + #:pid-file #$pid-file)) >>> (stop (action "stop")))))))) >> >> If pid-file != external-pid-file, Sherpherd will wait for a file that >> doesn't exist won't it? Because Postgresql will never be aware of that >> pid-file. > > Yep. > >> Plus, there is no reason to use make-forkexec-constructor on pg_ctl >> because pg_ctl returns after it has checked that the daemon is running. >> For the same reason, Shepherd doesn't need to know about Postgres' PID. >> All the hard work is done by pg_ctl. > > As the comment I made at the top, I did this when I was writing the > system test. If you remove this patch, when you call (start-service > 'postgres), it will return #f if the service starts successfully. If you > tweak the service to make it fail to start (e.g. by changing the "start" > action to something else), you get the same observable behaviour, > start-service returns #f. > > The way this works for other services, normally through > make-forkexec-constructor is that calling start-service will return the > PID. > > While the system test does still add some value even without checking if > the service has started, doing so would be really good. Even if it's not > using the PID file approach, maybe the exit code of pg_ctl could be > used? I'm not really sure why it isn't working like that already, as > invoke usually returns either #t or #f... The PID approch only makes sense when the executable run by Shepherd doesn't return; i.e. it blocks. Then Shepherd needs to fork a child process (with make-forkexec-constructor), so that it doesn't block. And the only way to know if the daemon has started is to find a "proof", which is the PID file. That's the point of the #:pid-file parameter of make-forkexec-constructor. But the PostgreSQL package is more elaborate: it provides an 'pg_ctl' executable that does all the hard work: - starting the daemon, - checking its PID to make sure the daemon is running, - exiting with 0 if the daemon is running. So Shepherd doesn't need to fork, it just checks the return value of pg_ctl. Conclusion: (start-service 'postgres) returning #t means that pg_ctl exit code is 0, which means that the daemon is running. I hope it makes more sense :-)
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 12:17:01 GMT) Full text and rfc822 format available.Message #62 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 4/4] services: databases: Add postgresql-configuration record exports. Date: Mon, 05 Mar 2018 13:16:46 +0100
Christopher Baines <mail <at> cbaines.net> writes: > * gnu/services/databases.scm: Export the record type, and all the field > accessors. ^ Small nit-pick: there is no indentation here :-). You can check "git log" to see how other people do. > --- > gnu/services/databases.scm | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm > index 4090277a7..1e410cd39 100644 > --- a/gnu/services/databases.scm > +++ b/gnu/services/databases.scm > @@ -43,7 +43,16 @@ > postgresql-config-file-external-pid-file > postgresql-config-file-extra-config > > + <postgresql-configuration> > + postgresql-configuration > postgresql-configuration? > + postgresql-configuration-postgresql > + postgresql-configuration-port > + postgresql-configuration-locale > + postgresql-configuration-pid-file > + postgresql-configuration-file > + postgresql-configuration-data-directory > + > postgresql-service > postgresql-service-type
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 19:26:02 GMT) Full text and rfc822 format available.Message #65 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 2/3] tests: databases: Add a system test for PostgreSQL. Date: Mon, 05 Mar 2018 19:25:38 +0000
[Message part 1 (text/plain, inline)]
Clément Lassieur <clement <at> lassieur.org> writes: > Christopher Baines <mail <at> cbaines.net> writes: > >> * gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables. >> (run-postgresql-test): New procedure. >> --- >> gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> >> diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm >> index e7097690a..583f484d7 100644 >> --- a/gnu/tests/databases.scm >> +++ b/gnu/tests/databases.scm >> @@ -30,6 +30,7 @@ >> #:use-module (guix store) >> #:export (%test-memcached >> %test-mongodb >> + %test-postgresql >> %test-mysql)) >> >> (define %memcached-os >> @@ -208,6 +209,60 @@ >> (value (run-mongodb-test)))) >> >> >> +;;; >> +;;; The PostgreSQL service. >> +;;; >> + >> +(define %postgresql-os >> + (simple-operating-system >> + (service postgresql-service-type))) >> + >> +(define* (run-postgresql-test) > > define, instead of define* > >> + "Run tests in %POSTGRESQL-OS." >> + (define os >> + (marionette-operating-system >> + %postgresql-os >> + #:imported-modules '((gnu services herd) >> + (guix combinators)))) >> + >> + (define vm >> + (virtual-machine >> + (operating-system os) >> + (memory-size 512))) >> + >> + (define test >> + (with-imported-modules '((gnu build marionette)) >> + #~(begin >> + (use-modules (srfi srfi-11) (srfi srfi-64) > > I think srfi-11 is useless. Good spot, I'll make these changes and send some new patches.
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 19:38:02 GMT) Full text and rfc822 format available.Message #68 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 2/3] tests: databases: Add a system test for PostgreSQL. Date: Mon, 5 Mar 2018 19:37:18 +0000
* gnu/tests/databases.scm (%postgresql-os, %test-postgresql): New variables. (run-postgresql-test): New procedure. --- gnu/tests/databases.scm | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm index e7097690a..5c8ca85c1 100644 --- a/gnu/tests/databases.scm +++ b/gnu/tests/databases.scm @@ -30,6 +30,7 @@ #:use-module (guix store) #:export (%test-memcached %test-mongodb + %test-postgresql %test-mysql)) (define %memcached-os @@ -208,6 +209,60 @@ (value (run-mongodb-test)))) +;;; +;;; The PostgreSQL service. +;;; + +(define %postgresql-os + (simple-operating-system + (service postgresql-service-type))) + +(define (run-postgresql-test) + "Run tests in %POSTGRESQL-OS." + (define os + (marionette-operating-system + %postgresql-os + #:imported-modules '((gnu services herd) + (guix combinators)))) + + (define vm + (virtual-machine + (operating-system os) + (memory-size 512))) + + (define test + (with-imported-modules '((gnu build marionette)) + #~(begin + (use-modules (srfi srfi-64) + (gnu build marionette)) + + (define marionette + (make-marionette (list #$vm))) + + (mkdir #$output) + (chdir #$output) + + (test-begin "postgresql") + + (test-assert "service running" + (marionette-eval + '(begin + (use-modules (gnu services herd)) + (start-service 'postgres)) + marionette)) + + (test-end) + (exit (= (test-runner-fail-count (test-runner-current)) 0))))) + + (gexp->derivation "postgresql-test" test)) + +(define %test-postgresql + (system-test + (name "postgresql") + (description "Start the PostgreSQL service.") + (value (run-postgresql-test)))) + + ;;; ;;; The MySQL service. ;;; -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 19:38:02 GMT) Full text and rfc822 format available.Message #71 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 3/3] services: databases: Add postgresql-configuration record exports. Date: Mon, 5 Mar 2018 19:37:19 +0000
* gnu/services/databases.scm: Export the record type, and all the field accessors. --- gnu/services/databases.scm | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm index f7d5fffd0..4c9a50a5a 100644 --- a/gnu/services/databases.scm +++ b/gnu/services/databases.scm @@ -40,7 +40,15 @@ postgresql-config-file-ident-file postgresql-config-file-extra-config + <postgresql-configuration> + postgresql-configuration postgresql-configuration? + postgresql-configuration-postgresql + postgresql-configuration-port + postgresql-configuration-locale + postgresql-configuration-file + postgresql-configuration-data-directory + postgresql-service postgresql-service-type -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 19:38:03 GMT) Full text and rfc822 format available.Message #74 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: 30701 <at> debbugs.gnu.org Subject: [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type. Date: Mon, 5 Mar 2018 19:37:17 +0000
For the default config file representation. This makes it possible to more easily change the configuration file, and have dynamic content. In particular, I'm looking at adding a pid file location to the config file. * gnu/services/databases.scm (<postgresql-config-file>): New record type. (%default-postgres-config): Remove this, it's been replaced by the configuration file. (<postgresql-configuration>): Alter the default for the config file field. (postgresql-service): Alter the default value for the config-file parameter. --- gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm index 3ca8f471f..f7d5fffd0 100644 --- a/gnu/services/databases.scm +++ b/gnu/services/databases.scm @@ -26,11 +26,20 @@ #:use-module (gnu system shadow) #:use-module (gnu packages admin) #:use-module (gnu packages databases) + #:use-module (guix store) #:use-module (guix modules) #:use-module (guix records) #:use-module (guix gexp) + #:use-module (srfi srfi-1) #:use-module (ice-9 match) - #:export (postgresql-configuration + #:export (<postgresql-config-file> + postgresql-config-file + postgresql-config-file? + postgresql-config-file-log-destination + postgresql-config-file-hba-file + postgresql-config-file-ident-file + postgresql-config-file-extra-config + postgresql-configuration? postgresql-service postgresql-service-type @@ -68,6 +77,60 @@ ;;; ;;; Code: +(define %default-postgres-hba + (plain-file "pg_hba.conf" + " +local all all trust +host all all 127.0.0.1/32 trust +host all all ::1/128 trust")) + +(define %default-postgres-ident + (plain-file "pg_ident.conf" + "# MAPNAME SYSTEM-USERNAME PG-USERNAME")) + +(define-record-type* <postgresql-config-file> + postgresql-config-file make-postgresql-config-file + postgresql-config-file? + (log-destination postgresql-config-file-log-destination + (default "syslog")) + (hba-file postgresql-config-file-hba-file + (default %default-postgres-hba)) + (ident-file postgresql-config-file-ident-file + (default %default-postgres-ident)) + (extra-config postgresql-config-file-extra-config + (default '()))) + +(define-gexp-compiler (postgresql-config-file-compiler + (file <postgresql-config-file>) system target) + (match file + (($ <postgresql-config-file> log-destination hba-file + ident-file extra-config) + (define (with-single-quotes string) + (if string + (list "'" string "'") + '())) + + (define contents + (append-map + (match-lambda + ((key) '()) + ((key . #f) '()) + ((key values ...) `(,key " = " ,@values "\n"))) + + `(("log_destination" ,@(with-single-quotes log-destination)) + ("hba_file" ,@(with-single-quotes hba-file)) + ("ident_file" ,@(with-single-quotes ident-file)) + ,@extra-config))) + + (gexp->derivation + "postgresql.conf" + #~(call-with-output-file (ungexp output "out") + (lambda (port) + (display + (string-append #$@contents) + port))) + #:local-build? #t)))) + (define-record-type* <postgresql-configuration> postgresql-configuration make-postgresql-configuration postgresql-configuration? @@ -78,27 +141,10 @@ (locale postgresql-configuration-locale (default "en_US.utf8")) (config-file postgresql-configuration-file - (default %default-postgres-config)) + (default (postgresql-config-file))) (data-directory postgresql-configuration-data-directory (default "/var/lib/postgresql/data"))) -(define %default-postgres-hba - (plain-file "pg_hba.conf" - " -local all all trust -host all all 127.0.0.1/32 trust -host all all ::1/128 trust")) - -(define %default-postgres-ident - (plain-file "pg_ident.conf" - "# MAPNAME SYSTEM-USERNAME PG-USERNAME")) - -(define %default-postgres-config - (mixed-text-file "postgresql.conf" - "log_destination = 'syslog'\n" - "hba_file = '" %default-postgres-hba "'\n" - "ident_file = '" %default-postgres-ident "'\n")) - (define %postgresql-accounts (list (user-group (name "postgres") (system? #t)) (user-account @@ -192,7 +238,7 @@ host all all ::1/128 trust")) (define* (postgresql-service #:key (postgresql postgresql) (port 5432) (locale "en_US.utf8") - (config-file %default-postgres-config) + (config-file (postgresql-config-file)) (data-directory "/var/lib/postgresql/data")) "Return a service that runs @var{postgresql}, the PostgreSQL database server. -- 2.16.0
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 19:38:03 GMT) Full text and rfc822 format available.Message #77 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 4/4] services: databases: Add postgresql-configuration record exports. Date: Mon, 05 Mar 2018 19:37:48 +0000
[Message part 1 (text/plain, inline)]
Clément Lassieur <clement <at> lassieur.org> writes: > Christopher Baines <mail <at> cbaines.net> writes: > >> * gnu/services/databases.scm: Export the record type, and all the field >> accessors. > ^ > Small nit-pick: there is no indentation here :-). You can check "git > log" to see how other people do. I've updated the git commit messages, and sent some updated patches. Thanks for taking a look :)
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Mon, 05 Mar 2018 21:34:02 GMT) Full text and rfc822 format available.Message #80 received at 30701 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701 <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type. Date: Mon, 05 Mar 2018 22:33:49 +0100
Christopher Baines <mail <at> cbaines.net> writes: > For the default config file representation. This makes it possible to more > easily change the configuration file, and have dynamic content. In particular, > I'm looking at adding a pid file location to the config file. ^ Could you please remove this last sentence (with the pid file)? > * gnu/services/databases.scm (<postgresql-config-file>): New record type. > (%default-postgres-config): Remove this, it's been replaced by the > configuration file. > (<postgresql-configuration>): Alter the default for the config file field. > (postgresql-service): Alter the default value for the config-file parameter. > --- > gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- > 1 file changed, 66 insertions(+), 20 deletions(-) > > diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm > index 3ca8f471f..f7d5fffd0 100644 > --- a/gnu/services/databases.scm > +++ b/gnu/services/databases.scm > @@ -26,11 +26,20 @@ > #:use-module (gnu system shadow) > #:use-module (gnu packages admin) > #:use-module (gnu packages databases) > + #:use-module (guix store) I don't think (guix store) is used. Is it? > #:use-module (guix modules) > #:use-module (guix records) > #:use-module (guix gexp) ... > + `(("log_destination" ,@(with-single-quotes log-destination)) > + ("hba_file" ,@(with-single-quotes hba-file)) > + ("ident_file" ,@(with-single-quotes ident-file)) ^ Could you please use a shorter name? Like "enclose", so that we won't go over 80 columns too easily :-). Apart from those small things, the three patches LGTM. Thank you again! Clément
Christopher Baines <mail <at> cbaines.net>
:Christopher Baines <mail <at> cbaines.net>
:Message #85 received at 30701-done <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701-done <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type. Date: Tue, 13 Mar 2018 17:37:30 +0000
[Message part 1 (text/plain, inline)]
Sorry, I made some changes, and merged these patches on the weekend, but I forgot to reply. Clément Lassieur <clement <at> lassieur.org> writes: > Christopher Baines <mail <at> cbaines.net> writes: > >> For the default config file representation. This makes it possible to more >> easily change the configuration file, and have dynamic content. In particular, >> I'm looking at adding a pid file location to the config file. > > ^ > Could you please remove this last sentence (with the pid file)? Done. >> * gnu/services/databases.scm (<postgresql-config-file>): New record type. >> (%default-postgres-config): Remove this, it's been replaced by the >> configuration file. >> (<postgresql-configuration>): Alter the default for the config file field. >> (postgresql-service): Alter the default value for the config-file parameter. >> --- >> gnu/services/databases.scm | 86 +++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 66 insertions(+), 20 deletions(-) >> >> diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm >> index 3ca8f471f..f7d5fffd0 100644 >> --- a/gnu/services/databases.scm >> +++ b/gnu/services/databases.scm >> @@ -26,11 +26,20 @@ >> #:use-module (gnu system shadow) >> #:use-module (gnu packages admin) >> #:use-module (gnu packages databases) >> + #:use-module (guix store) > > I don't think (guix store) is used. Is it? It wasn't, I've removed it. >> #:use-module (guix modules) >> #:use-module (guix records) >> #:use-module (guix gexp) > > ... > >> + `(("log_destination" ,@(with-single-quotes log-destination)) >> + ("hba_file" ,@(with-single-quotes hba-file)) >> + ("ident_file" ,@(with-single-quotes ident-file)) > ^ > Could you please use a shorter name? Like "enclose", so that we won't > go over 80 columns too easily :-). I went with quote' as I think that works well. > Apart from those small things, the three patches LGTM. Thank you > again! Thanks for taking a look and for your comments :) Chris
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Wed, 14 Mar 2018 17:38:03 GMT) Full text and rfc822 format available.Message #88 received at 30701-done <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701-done <at> debbugs.gnu.org Subject: Re: bug#30701: [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type. Date: Wed, 14 Mar 2018 18:37:40 +0100
Christopher Baines <mail <at> cbaines.net> writes: > Sorry, I made some changes, and merged these patches on the weekend, but > I forgot to reply. [...] >>> + `(("log_destination" ,@(with-single-quotes log-destination)) >>> + ("hba_file" ,@(with-single-quotes hba-file)) >>> + ("ident_file" ,@(with-single-quotes ident-file)) >> ^ >> Could you please use a shorter name? Like "enclose", so that we won't >> go over 80 columns too easily :-). > > I went with quote' as I think that works well. I don't like it because: • The extra \' doesn't help describing what the function does. One could believe it's a variant of 'quote', but it's actually very different. • It doesn't follow our coding style. See https://mumble.net/~campbell/scheme/style.txt. "Symbolic names are written with English words separated by hyphens." See also the part about "Funny Characters". > Thanks for taking a look and for your comments :) You're welcome :-) Clément
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Sat, 17 Mar 2018 20:36:01 GMT) Full text and rfc822 format available.Message #91 received at 30701-done <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Clément Lassieur <clement <at> lassieur.org> Cc: 30701-done <at> debbugs.gnu.org Subject: Re: bug#30701: [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type. Date: Sat, 17 Mar 2018 20:35:03 +0000
[Message part 1 (text/plain, inline)]
Clément Lassieur <clement <at> lassieur.org> writes: > Christopher Baines <mail <at> cbaines.net> writes: > >> Sorry, I made some changes, and merged these patches on the weekend, but >> I forgot to reply. > > [...] > >>>> + `(("log_destination" ,@(with-single-quotes log-destination)) >>>> + ("hba_file" ,@(with-single-quotes hba-file)) >>>> + ("ident_file" ,@(with-single-quotes ident-file)) >>> ^ >>> Could you please use a shorter name? Like "enclose", so that we won't >>> go over 80 columns too easily :-). >> >> I went with quote' as I think that works well. > > I don't like it because: > > • The extra \' doesn't help describing what the function does. One > could believe it's a variant of 'quote', but it's actually very > different. > > • It doesn't follow our coding style. See > https://mumble.net/~campbell/scheme/style.txt. "Symbolic names are > written with English words separated by hyphens." See also the part > about "Funny Characters". Fair enough, I've changed it to single-quote in 533808383f7fca6563aee1452f5202e0cd1b66b8.
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#30701
; Package guix-patches
.
(Sun, 18 Mar 2018 00:35:02 GMT) Full text and rfc822 format available.Message #94 received at 30701-done <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Christopher Baines <mail <at> cbaines.net> Cc: 30701-done <at> debbugs.gnu.org Subject: Re: [bug#30701] [PATCH 1/3] services: Rework the PostgreSQL config file to use a record type. Date: Sun, 18 Mar 2018 01:34:48 +0100
Christopher Baines <mail <at> cbaines.net> writes: > Clément Lassieur <clement <at> lassieur.org> writes: > >> Christopher Baines <mail <at> cbaines.net> writes: >> >>> Sorry, I made some changes, and merged these patches on the weekend, but >>> I forgot to reply. >> >> [...] >> >>>>> + `(("log_destination" ,@(with-single-quotes log-destination)) >>>>> + ("hba_file" ,@(with-single-quotes hba-file)) >>>>> + ("ident_file" ,@(with-single-quotes ident-file)) >>>> ^ >>>> Could you please use a shorter name? Like "enclose", so that we won't >>>> go over 80 columns too easily :-). >>> >>> I went with quote' as I think that works well. >> >> I don't like it because: >> >> • The extra \' doesn't help describing what the function does. One >> could believe it's a variant of 'quote', but it's actually very >> different. >> >> • It doesn't follow our coding style. See >> https://mumble.net/~campbell/scheme/style.txt. "Symbolic names are >> written with English words separated by hyphens." See also the part >> about "Funny Characters". > > Fair enough, I've changed it to single-quote in > 533808383f7fca6563aee1452f5202e0cd1b66b8. Thank you Christopher!
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org
.
(Sun, 15 Apr 2018 11:24:04 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.