Re: [PATCH] improve nginx-service

[Ludovic Courtès]

Hi!

Julien Lepiller <address@hidden> skribis:

> On Mon, 24 Oct 2016 22:51:27 +0200
> address@hidden (Ludovic Courtès) wrote:

[...]

>> >> I suppose we could make ‘nginx-service-type’ extensible (info
>> >> "(guix) Service Types and Services") so that people can write
>> >> services that define new vhosts?  
>> >
>> > You mean something like udev-service-type where you could extend it
>> > with a list of vhosts?  
>> 
>> Yes, exactly.  So for example one could write a service for some
>> high-level Web service that would in turn create an nginx vhost.
>> WDYT?
>
> Sounds great. I tried to work on it, but I'm stuck.
>
> I'm trying to implement a service that extends nginx-service-type with
> a vhost (default-nginx-vhost-service-type), and it creates a root
> directory for a default empty index file. Now I would like to pass the
> absolute path of this directory to the <nginx-vhost-configuration>, but
> it is too early for that. System configuration with this new service
> fails with:
>
> Backtrace:
> In guix/ui.scm:
> 1220: 19 [run-guix-command system "reconfigure" "/etc/config.scm"]

[...]

> ERROR: In procedure string-append:
> ERROR: In procedure string-append: Wrong type (expecting string):
> #<<computed-file> name: "default-nginx-vhost" gexp: #<gexp (begin
> (mkdir #<gexp-output out>) (symlink #<gexp-input #<<plain-file> name:
> "index.html" content: "[...]" references: ()>:out> (string-append
> #<gexp-output out> "/index.html"))) 5959270> options: (#:local-build?
> #t)>

I think the backtrace, ahem, very clearly shows what’s wrong.

(Suddenly I’m scared: what if this backtrace gave you the same feeling
that I have when I get a C++ compilation error?!)

> From 13748b6bfffef19080f4fa3bde2a6ae7d5c8d067 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <address@hidden>
> Date: Wed, 26 Oct 2016 21:33:34 +0200
> Subject: [PATCH] services: Make nginx-service-type extensible
>
> gnu/services/web.scm (nginx-service): Removed.
> gnu/services/web.scm (nginx-service-sytsem): Make extensible.
> gnu/services/web.scm (default-nginx-vhost-service-type): New variable.

What I had in mind was just to add ‘compose’ and ‘extend’ to
‘nginx-service-type’ (this is where we define how extensions are
handled).  There’s a bit of extra bookeeping to do, in particular moving
the list of vhosts to <nginx-configuration>, as in this untested patch:

diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 59e1e54..a319450 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -27,6 +27,7 @@
   #:use-module (gnu packages web)
   #:use-module (guix records)
   #:use-module (guix gexp)
+  #:use-module (srfi srfi-1)
   #:use-module (ice-9 match)
   #:export (nginx-configuration
             nginx-configuration?
@@ -67,7 +68,9 @@
   (nginx         nginx-configuration-nginx)         ;<package>
   (log-directory nginx-configuration-log-directory) ;string
   (run-directory nginx-configuration-run-directory) ;string
-  (file          nginx-configuration-file))         ;string | file-like
+  (file          nginx-configuration-file)          ;string | file-like
+  (vhosts        nginx-configuration-vhosts ;list of 
<nginx-vhost-configuration>
+                 (default (list (nginx-vhost-configuration)))))
 
 (define (config-domain-strings names)
  "Return a string denoting the nginx config representation of NAMES, a list
@@ -148,7 +151,8 @@ of index files."
 
 (define nginx-activation
   (match-lambda
-    (($ <nginx-configuration> nginx log-directory run-directory config-file)
+    (($ <nginx-configuration> nginx log-directory run-directory
+                              config-file vhosts)
      #~(begin
          (use-modules (guix build utils))
 
@@ -164,7 +168,10 @@ of index files."
          (mkdir-p (string-append #$run-directory "/scgi_temp"))
          ;; Check configuration file syntax.
          (system* (string-append #$nginx "/sbin/nginx")
-                  "-c" #$config-file "-t")))))
+                  "-c" #$(or config-file
+                             (default-nginx-config log-directory
+                               run-directory vhosts))
+                  "-t")))))
 
 (define nginx-shepherd-service
   (match-lambda
@@ -192,14 +199,24 @@ of index files."
                        (service-extension activation-service-type
                                           nginx-activation)
                        (service-extension account-service-type
-                                          (const %nginx-accounts))))))
+                                          (const %nginx-accounts))))
+
+                ;; Concatenate the list of vhosts we're given
+                (compose concatenate)
+
+                ;; Append the list of vhosts we were passed to those already
+                ;; in the config.
+                (extend (lambda (config vhosts)
+                          (nginx-configuration
+                           (inherit config)
+                           (vhosts (append (nginx-configuration-vhosts config)
+                                           vhosts)))))))
 
 (define* (nginx-service #:key (nginx nginx)
                         (log-directory "/var/log/nginx")
                         (run-directory "/var/run/nginx")
                         (vhost-list (list (nginx-vhost-configuration)))
-                        (config-file
-                         (default-nginx-config log-directory run-directory 
vhost-list)))
+                        (config-file #f))
   "Return a service that runs NGINX, the nginx web server.
 
 The nginx daemon loads its runtime configuration from CONFIG-FILE, stores log
@@ -207,6 +224,7 @@ files in LOG-DIRECTORY, and stores temporary runtime files 
in RUN-DIRECTORY."
   (service nginx-service-type
            (nginx-configuration
             (nginx nginx)
+            (vhosts vhost-list)
             (log-directory log-directory)
             (run-directory run-directory)
             (file config-file))))

With that in place, it becomes possible to write another service that
provides additional vhosts, like:


  (define vh
    (nginx-vhost-configuration …))

  (define foo
    (service-type (name 'foo)
                  (extensions (list (service-extension nginx-service-type
                                                       (const (list vh)))))))

or simply:

  (simple-service 'my-extra-vhost nginx-service-type (list vh))

Does that make sense?

Of course feel free to start from the patch above!

HTH,
Ludo’.