GNU bug report logs - #28769
[PATCH] gnu: services: Add php-fpm.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 28769 in the body.
You can then email your comments to 28769 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: nee <nee <at> cock.li>
To: guix-patches <at> gnu.org
Subject: [PATCH] gnu: services: Add php-fpm.
Date: Mon, 9 Oct 2017 23:54:24 +0200

[Message part 1 (text/plain, inline)]

Hello, this adds a php-fpm service. php-fpm is an alternative fcgi
implementation that is already build with the php package.

About the accounts:
Nginx needs write access to the unix socket of php-fpm. I didn't want to
set nginx as default user for php-fpm in case we add other webservers,
so I added the nginx to the newly created www-data group.

Thank you for reading. I'm looking forward to the revisions,
and happy Hacking!

[0001-guix-utils-add-version-major.patch (text/x-patch, attachment)]

[0002-gnu-services-Add-php-fpm.patch (text/x-patch, attachment)]

Message #8 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: nee <nee <at> cock.li>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Fri, 13 Oct 2017 21:06:08 +0100

[Message part 1 (text/plain, inline)]

On Mon, 9 Oct 2017 23:54:24 +0200
nee <nee <at> cock.li> wrote:

> Subject: [PATCH 1/2] guix: utils: add version-major.
> 
> * guix/utils.scm (version-major): New function.

I think procedure, rather than function is the usual, at least within
the context of commit messages in the guix repository.

> ---
>  guix/utils.scm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/guix/utils.scm b/guix/utils.scm
> index de4aa6531..cec209a8f 100644
> --- a/guix/utils.scm
> +++ b/guix/utils.scm
> @@ -72,6 +72,7 @@
>              version>=?  
>              version-prefix
>              version-major+minor
> +            version-major
>              guile-version>?
>              string-replace-substring
>              arguments-from-environment-variable
> @@ -488,6 +489,10 @@ For example, (version-prefix \"2.1.47.4.23\" 3)
> returns \"2.1.47\"" minor version numbers from version-string."
>    (version-prefix version-string 2))
>  
> +(define (version-major version-string)
> +  "Return the major version number as string from the
> version-string."
> +  (version-prefix version-string 1))
> +

Seems fine to me. I'm guessing that you added this because it was
convenient, but I can't see it being used in the next patch, have I
missed it?

[Message part 2 (application/pgp-signature, inline)]

Message #11 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: nee <nee <at> cock.li>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Fri, 13 Oct 2017 21:09:00 +0100

[Message part 1 (text/plain, inline)]

On Fri, 13 Oct 2017 21:06:08 +0100
Christopher Baines <mail <at> cbaines.net> wrote:

> On Mon, 9 Oct 2017 23:54:24 +0200
> nee <nee <at> cock.li> wrote:

...

> > +(define (version-major version-string)
> > +  "Return the major version number as string from the
> > version-string."
> > +  (version-prefix version-string 1))
> > +  
> 
> Seems fine to me. I'm guessing that you added this because it was
> convenient, but I can't see it being used in the next patch, have I
> missed it?

Yep, I missed it, but I see it now, problem solved :)

[Message part 2 (application/pgp-signature, inline)]

Message #14 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: nee <nee <at> cock.li>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Fri, 13 Oct 2017 22:37:29 +0100

[Message part 1 (text/plain, inline)]

On Mon, 9 Oct 2017 23:54:24 +0200
nee <nee <at> cock.li> wrote:

> Subject: [PATCH 2/2] gnu: services: Add php-fpm.

Hey, I've never used php-fpm, but I'll have a go at reviewing this :)

> * gnu/services/web.scm (<php-fpm-configuration>,
>   <php-fpm-process-manager-configuration>): New record types.
>   (php-fpm-configuration?,
>    php-fpm-process-manager-configuration?,
>    php-fpm-service-type,
>    nginx-php-location): New procedures.
> * doc/guix.texi (Web-Services): document php-fpm service.

Very minor, but I'd suggest "(Web Services): Document the php-fpm
service." here.

> ---
>  doc/guix.texi        |  93 +++++++++++++++++++++++++++
>  gnu/services/web.scm | 173
> ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed,
> 264 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index f0a59a6b4..ed4336f64 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -14529,6 +14529,99 @@ capability also has to be configured on the
> front-end as well. @end deftp
>  
>  
> +@cindex php-fpm
> +PHP-FPM (FastCGI Process Manager) is an alternative PHP FastCGI implementation
> +with some additional features useful for sites of any size.

If these additional features are worth mentioning, it would be best to
be specific as to what they are, and what benefit they provide.

> +@defvr {Scheme Variable} php-fpm-service-type
> +A Service type for @code{php-fpm}.
> +@end defvr
> +

...

> +@deftp {Data type} php-fpm-process-manager-configuration
> +Data Type for php-fpm worker process limits.
> +@table @asis
> +@item @code {type} (default: @code{"dynamic"})
> +@table @asis
> +@item @code{"dynamic"}
> +Spare worker processes are kept around based on the set @code{php-fpm-process-manager-configuration} limits.
> +@item @code{"static"}
> +A static number of worker processes is created.
> +@item @code{"ondemand"}
> +Worker processes are only created on demand.
> +@end table
> +@item @code {max-children} (default: @code{5})
> +Maximum of worker processes. Applies when the type is @code{"static"}, @code{"dynamic"}, or @code{"ondemand"}.
> +@item @code {start-servers} (default: @code{2})
> +How many worker processes should be started on start-up. Only applies when type is @code{"dynamic"}.
> +@item @code {min-spare-servers} (default: @code{1})
> +How many spare worker processes should be kept around at minimum. Only applies when type is @code{"dynamic"}.
> +@item @code {max-spare-servers} (default: @code{3})
> +How many spare worker processes should be kept around at maximum. Only applies when type is @code{"dynamic"}.
> +@item @code {process-idle-timeout} (default: @code{10})
> +The time in seconds after which a process with no requests is killed. Only applies when type is @code{"ondemand"}.
> +@end table
> +@end deftp

Reading this makes me think that maybe having record types per process
manager types might be useful, e.g.
<php-fpm-dynamic-process-manager-configuration>
<php-fpm-static-process-manager-configuration>
<php-fpm-process-manager-configuration>

> +@defvr {Scheme Variable} nginx-php-fpm-location
> +A helper function to quickly add php to an
> @code{nginx-server-configuration}. +@end defvr
> +
> +A simple services setup for nginx with php can look like this:
> +@example
> +(services (cons* (dhcp-client-service)
> +                 (service php-fpm-service-type
> +                          (php-fpm-configuration))

The default-value for the service type means that you can omit the
(php-fpm-configuration) here, as (service php-fpm-service-type) should
work.

> +                 (service nginx-service-type
> +                          (nginx-server-configuration
> +                           (server-name '("example.com"))
> +                           (root "/srv/http/")
> +                           (locations
> +                            (list (nginx-php-location)))
> +                           (https-port #f)
> +                           (ssl-certificate #f)
> +                           (ssl-certificate-key #f)))
> +                 %base-services))
> +@end example
> +
> +
>  @node DNS Services
>  @subsubsection DNS Services
>  @cindex DNS (domain name system)
> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
> index 9d713003c..fd63b15bb 100644
> --- a/gnu/services/web.scm
> +++ b/gnu/services/web.scm
> @@ -26,8 +26,11 @@
>    #:use-module (gnu system shadow)
>    #:use-module (gnu packages admin)
>    #:use-module (gnu packages web)
> +  #:use-module (gnu packages php)
>    #:use-module (guix records)
>    #:use-module (guix gexp)
> +  #:use-module ((guix utils) #:select (version-major))
> +  #:use-module ((guix packages) #:select (package-version))
>    #:use-module (srfi srfi-1)
>    #:use-module (ice-9 match)
>    #:export (<nginx-configuration>
> @@ -76,7 +79,14 @@
>  
>              fcgiwrap-configuration
>              fcgiwrap-configuration?
> -            fcgiwrap-service-type))
> +            fcgiwrap-service-type
> +
> +            php-fpm-configuration
> +            php-fpm-configuration?
> +            php-fpm-process-manager-configuration
> +            php-fpm-process-manager-configuration?
> +            php-fpm-service-type
> +            nginx-php-location))

When using other Guix services, I've run in to problems when field
accessors and record types were not exported. The biggest cost I can
think of is the lines of code, but I'd still suggest to export
everything by default here.

>  ;;; Commentary:
>  ;;;
> @@ -256,10 +266,12 @@ of index files."
>            "events {}\n")))
>  
>  (define %nginx-accounts
> -  (list (user-group (name "nginx") (system? #t))
> +  (list (user-group (name "www-data") (system? #t))
> +        (user-group (name "nginx") (system? #t))
>          (user-account
>           (name "nginx")
>           (group "nginx")
> +         (supplementary-groups '("www-data"))
>           (system? #t)
>           (comment "nginx server user")
>           (home-directory "/var/empty")

Pulling in your comment about the accounts...

> About the accounts:
> Nginx needs write access to the unix socket of php-fpm. I didn't want
> to set nginx as default user for php-fpm in case we add other
> webservers, so I added the nginx to the newly created www-data group.

This sounds like it would work, however, the defaults for the
socket-user and socket-group of the php-fpm-configuration are currently
"nginx", which seems to disagree with what you say above?

Also, the idea that came to my mind for this is to add php-fpm as a
supplementary group for the nginx user. In my mind, this seems the
neatest way of giving nginx access to the socket. What do you think?

I suggest this, as I'm not sure the name of the www-data group does a
good job of describing that it's controlling access to a socket. Also,
giving nginx explicit access to things owned by the php-fpm group could
be more secure than using a more generic group, especially as other
services that might need access to the socket, could end up getting it
because they need access to other things using the www-data group.

If the above sounds like a good idea, I think it could be implemented
by adding a configurable list of supplementary groups to the
nginx-service-type. Then maybe even adding support for configuring this
via the service extensions mechanism, then having the
php-fpm-service-type extending the nginx-service-type...

The above is definitely to complicated to do all in one go, especially
since the nginx-service-type doesn't support anything more complicated
than adding server blocks through extension at the moment.

> @@ -385,3 +397,160 @@ of index files."
>  		       (service-extension account-service-type
>                                            fcgiwrap-accounts)))
>                  (default-value (fcgiwrap-configuration))))
> +
> +(define-record-type* <php-fpm-configuration> php-fpm-configuration
> +  make-php-fpm-configuration
> +  php-fpm-configuration?
> +  (php             php-fpm-configuration-php ;<package>
> +                   (default php))
> +  (socket          php-fpm-configuration-socket
> +                   (default (string-append "/var/run/php"
> +                                         (version-major
> (package-version php))
> +                                         "-fpm.sock")))
> +  (user            php-fpm-configuration-user
> +                   (default "php-fpm"))
> +  (group           php-fpm-configuration-group
> +                   (default "php-fpm"))
> +  (socket-user     php-fpm-configuration-socket-user
> +                   (default "nginx"))
> +  (socket-group    php-fpm-configuration-socket-group
> +                   (default "nginx"))

As above, I'm not sure the use of nginx here matches the
description you gave...?

> +  (process-manager php-fpm-configuration-process-manager
> +                   (default (php-fpm-process-manager-configuration)))
> +  (display-errors  php-fpm-configuration-display-errors
> +                   (default #f))
> +  (workers-logfile php-fpm-configuration-workers-logfile
> +                   (default (string-append "/var/log/php"
> +                                         (version-major
> (package-version php))
> +                                         "-fpm.log")))

I'm not sure the php is adding much to the log file name, but then
again I've never used php... what do you think?

> +  (file          php-fpm-configuration-file ;#f | file-like
> +                 (default #f)))
> +
> +(define-record-type* <php-fpm-process-manager-configuration>
> php-fpm-process-manager-configuration
> +  make-php-fpm-process-manager-configuration
> +  php-fpm-process-manager-configuration?
> +  (type                 php-fpm-process-manager-configuration-type
> +                        (default "dynamic"))
> +  (max-children
> php-fpm-process-manager-configuration-max-children
> +                        (default 5))
> +  (start-servers
> php-fpm-process-manager-configuration-start-servers
> +                        (default 2))
> +  (min-spare-servers
> php-fpm-process-manager-configuration-min-spare-servers
> +                        (default 1))
> +  (max-spare-servers
> php-fpm-process-manager-configuration-max-spare-servers
> +                        (default 3))
> +  (process-idle-timeout
> php-fpm-process-manager-configuration-process-idle-timeout
> +                        (default 10)))
> +
> +(define php-fpm-accounts
> +  (match-lambda
> +    (($ <php-fpm-configuration> php socket user group socket-user
> socket-group _ _ _ _)
> +     (filter identity
> +             (list
> +              (user-group (name "www-data") (system? #t))
> +              (and (equal? group "php-fpm")
> +                   (user-group
> +                    (name "php-fpm")
> +                    (system? #t)))
> +              (and (equal? user "php-fpm")
> +                   (user-account
> +                    (name "php-fpm")
> +                    (group group)
> +                    (supplementary-groups '("www-data"))
> +                    (system? #t)
> +                    (comment "web services group")
> +                    (home-directory "/var/empty")
> +                    (shell (file-append shadow
> "/sbin/nologin"))))))))) +

As you can specify the user and group in the <php-fpm-configuration>, I
think it might be better to just use the user and group names from
there, and always setup a user and group with those names, I'm guessing
that this will be what the average user would expect to happen.

> +(define (default-php-fpm-config socket user group socket-user
> socket-group
> +          pm display-errors workers-logfile)
> +  (match
> +      pm
> +    (($ <php-fpm-process-manager-configuration> pm.type
> +                                                pm.max-children
> +                                                pm.start-servers
> +                                                pm.min-spare-servers
> +                                                pm.max-spare-servers
> +
> pm.process-idle-timeout)
> +     (apply mixed-text-file "php-fpm.conf"
> +            "[global]\n"
> +            "[www]\n"
> +            "user =" user "\n"
> +            "group =" group "\n"
> +            "listen =" socket "\n"
> +            "listen.owner =" socket-user "\n"
> +            "listen.group =" socket-group "\n"
> +
> +            "pm =" pm.type "\n"
> +            "pm.max_children =" (number->string pm.max-children) "\n"
> +            "pm.start_servers =" (number->string pm.start-servers)
> "\n"
> +            "pm.min_spare_servers =" (number->string
> pm.min-spare-servers) "\n"
> +            "pm.max_spare_servers =" (number->string
> pm.max-spare-servers) "\n"
> +            "pm.process_idle_timeout =" (number->string
> pm.process-idle-timeout) "s\n" +
> +            "php_flag[display_errors] = " (if display-errors "on"
> "off") "\n" +
> +            (if workers-logfile
> +                (list "catch_workers_output = yes\n"
> +                      "php_admin_value[error_log] =" workers-logfile
> "\n"
> +                      "php_admin_flag[log_errors] = on\n")
> +                (list "catch_workers_output = no\n"))))))
> +
> +(define php-fpm-shepherd-service
> +  (match-lambda
> +    (($ <php-fpm-configuration> php socket user group socket-user
> socket-group
> +                                pm display-errors workers-logfile
> file)
> +     (list (shepherd-service
> +            (provision '(php-fpm))
> +            (documentation "Run the php-fpm daemon.")
> +            (requirement '(networking))
> +            (start #~(make-forkexec-constructor
> +                      '(#$(file-append php "/sbin/php-fpm")
> +                        "--nodaemonize" "-p" "/var" "--fpm-config"
> +                        #$(or file
> +                              (default-php-fpm-config socket user
> group
> +                                socket-user socket-group pm
> display-errors
> +                                workers-logfile)))))
> +            (stop #~(make-kill-destructor)))))))

As php-fpm supports using a pid file, I'd recommend configuring both
php-fpm and shepherd to use this by default. It means that the shepherd
service will wait until php-fpm creates the PID file before starting
services that say they require it, which can prevent some issues.

> +(define php-fpm-activation
> +  (match-lambda
> +    (($ <php-fpm-configuration> _ _ user _ _ _ _ _ workers-logfile _)
> +     #~(begin
> +         (use-modules (guix build utils))
> +         (let ((user (getpwnam #$user))
> +               (touch (lambda (file-name)
> +                        (call-with-output-file file-name (const
> #t)))))
> +           ;; prepare writable logfile
> +           (when #$workers-logfile
> +             (when (not (file-exists? #$workers-logfile))
> +               (touch #$workers-logfile))
> +             (chown #$workers-logfile (passwd:uid user) (passwd:gid
> user))
> +             (chmod #$workers-logfile #o660)))))))
> +
> +
> +(define php-fpm-service-type
> +  (service-type (name 'php-fpm)
> +                (extensions
> +                 (list (service-extension shepherd-root-service-type
> +                                          php-fpm-shepherd-service)
> +                       (service-extension activation-service-type
> +                                          php-fpm-activation)
> +                       (service-extension account-service-type
> +                                          php-fpm-accounts)))
> +                (default-value (php-fpm-configuration))))

Filling in the description (a relatively new field on the service type)
would be a great addition here.

> +(define* (nginx-php-location
> +          #:key
> +          (nginx-package nginx)
> +          (socket (string-append "/var/run/php"
> +                                 (version-major (package-version
> php))
> +                                 "-fpm.sock")))
> +  "Return a nginx-location-configuration that makes nginx run .php
> files."
> +  (nginx-location-configuration
> +   (uri "~ \\.php$")
> +   (body (list
> +          "fastcgi_split_path_info ^(.+\\.php)(/.+)$;"
> +          (string-append "fastcgi_pass unix:" socket ";")
> +          "fastcgi_index index.php;"
> +          (list "include " nginx-package
> "/share/nginx/conf/fastcgi.conf;")))))

This helper seems good, although I think putting the "include"
elsewhere would be better, and I recognise that this isn't possible
with the nginx service yet.

Overall, I think this is great. Excellent use of record types, gexps
and match expressions. I think it would be good to think more on the
accounts issue before merging, but that is all that comes to mind
currently.

Also, if you feel like it, as service test would be a great addition to
this patch. There is a test for nginx already in gnu/tests/web.scm, and
I think you could get most of the benefit by having a test for nginx
with php-fpm, as that would give you some coverage over the php-fpm
service, as well as the nginx configuration.

[Message part 2 (application/pgp-signature, inline)]

Message #17 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: nee <nee <at> cock.li>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Mon, 16 Oct 2017 23:38:50 +0200

[Message part 1 (text/plain, inline)]

Hello, here is an updated version.
I'm weirdly having trouble with getting the log-file to log something
(see below), everything else should be addressed.

Am 13.10.2017 um 23:37 schrieb Christopher Baines:
> On Mon, 9 Oct 2017 23:54:24 +0200
> nee <nee <at> cock.li> wrote:
> 
>> Subject: [PATCH 2/2] gnu: services: Add php-fpm.
> 
> Hey, I've never used php-fpm, but I'll have a go at reviewing this :)
> 
>> * gnu/services/web.scm (<php-fpm-configuration>,
>>   <php-fpm-process-manager-configuration>): New record types.
>>   (php-fpm-configuration?,
>>    php-fpm-process-manager-configuration?,
>>    php-fpm-service-type,
>>    nginx-php-location): New procedures.
>> * doc/guix.texi (Web-Services): document php-fpm service.
> 
> Very minor, but I'd suggest "(Web Services): Document the php-fpm
> service." here.
> 
Okay, I changed the first letter to capital case and from the other
patch 'function' to 'procedure'.

>> ---
>>  doc/guix.texi        |  93 +++++++++++++++++++++++++++
>>  gnu/services/web.scm | 173
>> ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed,
>> 264 insertions(+), 2 deletions(-)
>>
>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index f0a59a6b4..ed4336f64 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -14529,6 +14529,99 @@ capability also has to be configured on the
>> front-end as well. @end deftp
>>  
>>  
>> +@cindex php-fpm
>> +PHP-FPM (FastCGI Process Manager) is an alternative PHP FastCGI implementation
>> +with some additional features useful for sites of any size.
> 
> If these additional features are worth mentioning, it would be best to
> be specific as to what they are, and what benefit they provide.
> 
Okay I copied the list of features form the php-fpm homepage.
I didn't add configuration for all of the features, tough.

There is some advanced stuff with pools that use different php.inis,
chroot, and using sendmail to email errors.
I'm not much of a php user and I don't know how to test everything.
I want to get a basic service working at first.

>> +@defvr {Scheme Variable} php-fpm-service-type
>> +A Service type for @code{php-fpm}.
>> +@end defvr
>> +
> 
> ...
> 
>> +@deftp {Data type} php-fpm-process-manager-configuration
>> +Data Type for php-fpm worker process limits.
>> +@table @asis
>> +@item @code {type} (default: @code{"dynamic"})
>> +@table @asis
>> +@item @code{"dynamic"}
>> +Spare worker processes are kept around based on the set @code{php-fpm-process-manager-configuration} limits.
>> +@item @code{"static"}
>> +A static number of worker processes is created.
>> +@item @code{"ondemand"}
>> +Worker processes are only created on demand.
>> +@end table
>> +@item @code {max-children} (default: @code{5})
>> +Maximum of worker processes. Applies when the type is @code{"static"}, @code{"dynamic"}, or @code{"ondemand"}.
>> +@item @code {start-servers} (default: @code{2})
>> +How many worker processes should be started on start-up. Only applies when type is @code{"dynamic"}.
>> +@item @code {min-spare-servers} (default: @code{1})
>> +How many spare worker processes should be kept around at minimum. Only applies when type is @code{"dynamic"}.
>> +@item @code {max-spare-servers} (default: @code{3})
>> +How many spare worker processes should be kept around at maximum. Only applies when type is @code{"dynamic"}.
>> +@item @code {process-idle-timeout} (default: @code{10})
>> +The time in seconds after which a process with no requests is killed. Only applies when type is @code{"ondemand"}.
>> +@end table
>> +@end deftp
> 
> Reading this makes me think that maybe having record types per process
> manager types might be useful, e.g.
> <php-fpm-dynamic-process-manager-configuration>
> <php-fpm-static-process-manager-configuration>
> <php-fpm-process-manager-configuration>
> 
Good point! That makes misconfiguration less likely.
I changed it to be that way.


>> +@defvr {Scheme Variable} nginx-php-fpm-location
>> +A helper function to quickly add php to an
>> @code{nginx-server-configuration}. +@end defvr
>> +
>> +A simple services setup for nginx with php can look like this:
>> +@example
>> +(services (cons* (dhcp-client-service)
>> +                 (service php-fpm-service-type
>> +                          (php-fpm-configuration))
> 
> The default-value for the service type means that you can omit the
> (php-fpm-configuration) here, as (service php-fpm-service-type) should
> work.
> 
Okay, I changed the example.

>> +                 (service nginx-service-type
>> +                          (nginx-server-configuration
>> +                           (server-name '("example.com"))
>> +                           (root "/srv/http/")
>> +                           (locations
>> +                            (list (nginx-php-location)))
>> +                           (https-port #f)
>> +                           (ssl-certificate #f)
>> +                           (ssl-certificate-key #f)))
>> +                 %base-services))
>> +@end example
>> +
>> +
>>  @node DNS Services
>>  @subsubsection DNS Services
>>  @cindex DNS (domain name system)
>> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
>> index 9d713003c..fd63b15bb 100644
>> --- a/gnu/services/web.scm
>> +++ b/gnu/services/web.scm
>> @@ -26,8 +26,11 @@
>>    #:use-module (gnu system shadow)
>>    #:use-module (gnu packages admin)
>>    #:use-module (gnu packages web)
>> +  #:use-module (gnu packages php)
>>    #:use-module (guix records)
>>    #:use-module (guix gexp)
>> +  #:use-module ((guix utils) #:select (version-major))
>> +  #:use-module ((guix packages) #:select (package-version))
>>    #:use-module (srfi srfi-1)
>>    #:use-module (ice-9 match)
>>    #:export (<nginx-configuration>
>> @@ -76,7 +79,14 @@
>>  
>>              fcgiwrap-configuration
>>              fcgiwrap-configuration?
>> -            fcgiwrap-service-type))
>> +            fcgiwrap-service-type
>> +
>> +            php-fpm-configuration
>> +            php-fpm-configuration?
>> +            php-fpm-process-manager-configuration
>> +            php-fpm-process-manager-configuration?
>> +            php-fpm-service-type
>> +            nginx-php-location))
> 
> When using other Guix services, I've run in to problems when field
> accessors and record types were not exported. The biggest cost I can
> think of is the lines of code, but I'd still suggest to export
> everything by default here.
> 
Okay, I export them now.
The fcgiwrapper package does not export them though.
That should be addressed in a separate patch later.


>>  ;;; Commentary:
>>  ;;;
>> @@ -256,10 +266,12 @@ of index files."
>>            "events {}\n")))
>>  
>>  (define %nginx-accounts
>> -  (list (user-group (name "nginx") (system? #t))
>> +  (list (user-group (name "www-data") (system? #t))
>> +        (user-group (name "nginx") (system? #t))
>>          (user-account
>>           (name "nginx")
>>           (group "nginx")
>> +         (supplementary-groups '("www-data"))
>>           (system? #t)
>>           (comment "nginx server user")
>>           (home-directory "/var/empty")
> 
> Pulling in your comment about the accounts...
> 
>> About the accounts:
>> Nginx needs write access to the unix socket of php-fpm. I didn't want
>> to set nginx as default user for php-fpm in case we add other
>> webservers, so I added the nginx to the newly created www-data group.
> 
> This sounds like it would work, however, the defaults for the
> socket-user and socket-group of the php-fpm-configuration are currently
> "nginx", which seems to disagree with what you say above?
> 
That's a mistake. Seems like I actually forgot to change that.

> Also, the idea that came to my mind for this is to add php-fpm as a
> supplementary group for the nginx user. In my mind, this seems the
> neatest way of giving nginx access to the socket. What do you think?
> 
> I suggest this, as I'm not sure the name of the www-data group does a
> good job of describing that it's controlling access to a socket. Also,
> giving nginx explicit access to things owned by the php-fpm group could
> be more secure than using a more generic group, especially as other
> services that might need access to the socket, could end up getting it
> because they need access to other things using the www-data group.
> 
Using more specialized groups sounds good in general, I changed the
group name.
It would be weird to have the nginx user in a bunch of groups for
software that you did not install though.
> If the above sounds like a good idea, I think it could be implemented
> by adding a configurable list of supplementary groups to the
> nginx-service-type. Then maybe even adding support for configuring this
> via the service extensions mechanism, then having the
> php-fpm-service-type extending the nginx-service-type...
> 
> The above is definitely to complicated to do all in one go, especially
> since the nginx-service-type doesn't support anything more complicated
> than adding server blocks through extension at the moment.
> 
This seems to be a solution.

>> @@ -385,3 +397,160 @@ of index files."
>>  		       (service-extension account-service-type
>>                                            fcgiwrap-accounts)))
>>                  (default-value (fcgiwrap-configuration))))
>> +
>> +(define-record-type* <php-fpm-configuration> php-fpm-configuration
>> +  make-php-fpm-configuration
>> +  php-fpm-configuration?
>> +  (php             php-fpm-configuration-php ;<package>
>> +                   (default php))
>> +  (socket          php-fpm-configuration-socket
>> +                   (default (string-append "/var/run/php"
>> +                                         (version-major
>> (package-version php))
>> +                                         "-fpm.sock")))
>> +  (user            php-fpm-configuration-user
>> +                   (default "php-fpm"))
>> +  (group           php-fpm-configuration-group
>> +                   (default "php-fpm"))
>> +  (socket-user     php-fpm-configuration-socket-user
>> +                   (default "nginx"))
>> +  (socket-group    php-fpm-configuration-socket-group
>> +                   (default "nginx"))
> 
> As above, I'm not sure the use of nginx here matches the
> description you gave...?
> 
Mistake on my side. I changed it to php-fpm now.

>> +  (process-manager php-fpm-configuration-process-manager
>> +                   (default (php-fpm-process-manager-configuration)))
>> +  (display-errors  php-fpm-configuration-display-errors
>> +                   (default #f))
>> +  (workers-logfile php-fpm-configuration-workers-logfile
>> +                   (default (string-append "/var/log/php"
>> +                                         (version-major
>> (package-version php))
>> +                                         "-fpm.log")))
> 
> I'm not sure the php is adding much to the log file name, but then
> again I've never used php... what do you think?
> 
For the workers-logfile:
This is the logfile for the php workers error's
It only logs errors that were printed with error_log by a php worker
process.
Try this example file:

<?php
error_log("some error\n"); // should be in the log
echo "Hello from php"; // should be in the browser

Then visit the err.php file with curl or a browser using the nginx
example above.

Also:
I forgot to add a setting for the log-file of the php-fpm master
process. This file should log messages when php-fpm has started or stopped.

For some reason it stays empty now. I had it log stuff before, when I
manually killed the process, but I can't reproduce it anymore. I changed
the permissions to ugo+wrx and it still stays empty. I'm not sure what's
going on.

I renamed the default workers-log-file to php7-fpm.www.log as it is
usually called. The php-fpm log-file is now called php7-fpm.log.

>> +  (file          php-fpm-configuration-file ;#f | file-like
>> +                 (default #f)))
>> +
>> +(define-record-type* <php-fpm-process-manager-configuration>
>> php-fpm-process-manager-configuration
>> +  make-php-fpm-process-manager-configuration
>> +  php-fpm-process-manager-configuration?
>> +  (type                 php-fpm-process-manager-configuration-type
>> +                        (default "dynamic"))
>> +  (max-children
>> php-fpm-process-manager-configuration-max-children
>> +                        (default 5))
>> +  (start-servers
>> php-fpm-process-manager-configuration-start-servers
>> +                        (default 2))
>> +  (min-spare-servers
>> php-fpm-process-manager-configuration-min-spare-servers
>> +                        (default 1))
>> +  (max-spare-servers
>> php-fpm-process-manager-configuration-max-spare-servers
>> +                        (default 3))
>> +  (process-idle-timeout
>> php-fpm-process-manager-configuration-process-idle-timeout
>> +                        (default 10)))
>> +
>> +(define php-fpm-accounts
>> +  (match-lambda
>> +    (($ <php-fpm-configuration> php socket user group socket-user
>> socket-group _ _ _ _)
>> +     (filter identity
>> +             (list
>> +              (user-group (name "www-data") (system? #t))
>> +              (and (equal? group "php-fpm")
>> +                   (user-group
>> +                    (name "php-fpm")
>> +                    (system? #t)))
>> +              (and (equal? user "php-fpm")
>> +                   (user-account
>> +                    (name "php-fpm")
>> +                    (group group)
>> +                    (supplementary-groups '("www-data"))
>> +                    (system? #t)
>> +                    (comment "web services group")
>> +                    (home-directory "/var/empty")
>> +                    (shell (file-append shadow
>> "/sbin/nologin"))))))))) +
> 
> As you can specify the user and group in the <php-fpm-configuration>, I
> think it might be better to just use the user and group names from
> there, and always setup a user and group with those names, I'm guessing
> that this will be what the average user would expect to happen.
> 
I also thought that, but I took the config from the fastcgi service as
example. The consequence of that is that you have to define your users
yourself and get an error if you didn't.

I'm changing it for this patch.

>> +(define (default-php-fpm-config socket user group socket-user
>> socket-group
>> +          pm display-errors workers-logfile)
>> +  (match
>> +      pm
>> +    (($ <php-fpm-process-manager-configuration> pm.type
>> +                                                pm.max-children
>> +                                                pm.start-servers
>> +                                                pm.min-spare-servers
>> +                                                pm.max-spare-servers
>> +
>> pm.process-idle-timeout)
>> +     (apply mixed-text-file "php-fpm.conf"
>> +            "[global]\n"
>> +            "[www]\n"
>> +            "user =" user "\n"
>> +            "group =" group "\n"
>> +            "listen =" socket "\n"
>> +            "listen.owner =" socket-user "\n"
>> +            "listen.group =" socket-group "\n"
>> +
>> +            "pm =" pm.type "\n"
>> +            "pm.max_children =" (number->string pm.max-children) "\n"
>> +            "pm.start_servers =" (number->string pm.start-servers)
>> "\n"
>> +            "pm.min_spare_servers =" (number->string
>> pm.min-spare-servers) "\n"
>> +            "pm.max_spare_servers =" (number->string
>> pm.max-spare-servers) "\n"
>> +            "pm.process_idle_timeout =" (number->string
>> pm.process-idle-timeout) "s\n" +
>> +            "php_flag[display_errors] = " (if display-errors "on"
>> "off") "\n" +
>> +            (if workers-logfile
>> +                (list "catch_workers_output = yes\n"
>> +                      "php_admin_value[error_log] =" workers-logfile
>> "\n"
>> +                      "php_admin_flag[log_errors] = on\n")
>> +                (list "catch_workers_output = no\n"))))))
>> +
>> +(define php-fpm-shepherd-service
>> +  (match-lambda
>> +    (($ <php-fpm-configuration> php socket user group socket-user
>> socket-group
>> +                                pm display-errors workers-logfile
>> file)
>> +     (list (shepherd-service
>> +            (provision '(php-fpm))
>> +            (documentation "Run the php-fpm daemon.")
>> +            (requirement '(networking))
>> +            (start #~(make-forkexec-constructor
>> +                      '(#$(file-append php "/sbin/php-fpm")
>> +                        "--nodaemonize" "-p" "/var" "--fpm-config"
>> +                        #$(or file
>> +                              (default-php-fpm-config socket user
>> group
>> +                                socket-user socket-group pm
>> display-errors
>> +                                workers-logfile)))))
>> +            (stop #~(make-kill-destructor)))))))
> 
> As php-fpm supports using a pid file, I'd recommend configuring both
> php-fpm and shepherd to use this by default. It means that the shepherd
> service will wait until php-fpm creates the PID file before starting
> services that say they require it, which can prevent some issues.
> 
I didn't know about this. It sounds like a good feature. I added it to
the config and to the make-forkexec-constructor key now. Thanks for
noting it!

I also renamed the logfile variables to log-file to match the pid-file
naming.

>> +(define php-fpm-activation
>> +  (match-lambda
>> +    (($ <php-fpm-configuration> _ _ user _ _ _ _ _ workers-logfile _)
>> +     #~(begin
>> +         (use-modules (guix build utils))
>> +         (let ((user (getpwnam #$user))
>> +               (touch (lambda (file-name)
>> +                        (call-with-output-file file-name (const
>> #t)))))
>> +           ;; prepare writable logfile
>> +           (when #$workers-logfile
>> +             (when (not (file-exists? #$workers-logfile))
>> +               (touch #$workers-logfile))
>> +             (chown #$workers-logfile (passwd:uid user) (passwd:gid
>> user))
>> +             (chmod #$workers-logfile #o660)))))))
>> +
>> +
>> +(define php-fpm-service-type
>> +  (service-type (name 'php-fpm)
>> +                (extensions
>> +                 (list (service-extension shepherd-root-service-type
>> +                                          php-fpm-shepherd-service)
>> +                       (service-extension activation-service-type
>> +                                          php-fpm-activation)
>> +                       (service-extension account-service-type
>> +                                          php-fpm-accounts)))
>> +                (default-value (php-fpm-configuration))))
> 
> Filling in the description (a relatively new field on the service type)
> would be a great addition here.
> 
Ah, yes I'm mostly using the web documentation and other services from
web.scm as reference. Thanks for the update.
What would be a good value for this field? I just used "The php-fpm
service-type." for now.



>> +(define* (nginx-php-location
>> +          #:key
>> +          (nginx-package nginx)
>> +          (socket (string-append "/var/run/php"
>> +                                 (version-major (package-version
>> php))
>> +                                 "-fpm.sock")))
>> +  "Return a nginx-location-configuration that makes nginx run .php
>> files."
>> +  (nginx-location-configuration
>> +   (uri "~ \\.php$")
>> +   (body (list
>> +          "fastcgi_split_path_info ^(.+\\.php)(/.+)$;"
>> +          (string-append "fastcgi_pass unix:" socket ";")
>> +          "fastcgi_index index.php;"
>> +          (list "include " nginx-package
>> "/share/nginx/conf/fastcgi.conf;")))))
> 
> This helper seems good, although I think putting the "include"
> elsewhere would be better, and I recognise that this isn't possible
> with the nginx service yet.
> 
Alright, so I'll keep it like this for now.

> Overall, I think this is great. Excellent use of record types, gexps
> and match expressions. I think it would be good to think more on the
> accounts issue before merging, but that is all that comes to mind
> currently.
> 
Good to hear, thank you! :)

> Also, if you feel like it, as service test would be a great addition to
> this patch. There is a test for nginx already in gnu/tests/web.scm, and
> I think you could get most of the benefit by having a test for nginx
> with php-fpm, as that would give you some coverage over the php-fpm
> service, as well as the nginx configuration.
> 
That sounds great I love the idea of system tests and will have a look
at it later.

I also added a line to copyright headers of each file. Except for the
utils.scm file, because that change is trivial copy pasta.

Thank you for reviewing this very big patch.
Happy hacking!

[0001-guix-utils-add-version-major.patch (text/x-patch, attachment)]

[0002-gnu-services-Add-php-fpm.patch (text/x-patch, attachment)]

Message #20 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: nee <nee <at> cock.li>
To: Christopher Baines <mail <at> cbaines.net>, 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Tue, 24 Oct 2017 00:26:35 +0200

[Message part 1 (text/plain, inline)]

Hello again,
I fixed the empty log-file. Here are the updated patches.

The problem was this line:
>+                        "--nodaemonize" "-p" "/var" "--fpm-config"

When --nodaemonize is set, the log is printed to stdout instead.
I also removed -p "/var". It is not required when the log, pid and
socket locations are defined in the config.
Both were caused, because I didn't use pid-files from the start.

Happy Hacking!

[0001-guix-utils-add-version-major.patch (text/x-patch, attachment)]

[0002-gnu-services-Add-php-fpm.patch (text/x-patch, attachment)]

Message #23 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: nee <nee <at> cock.li>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Thu, 2 Nov 2017 08:16:38 +0000

[Message part 1 (text/plain, inline)]

On Tue, 24 Oct 2017 00:26:35 +0200
nee <nee <at> cock.li> wrote:

> Hello again,
> I fixed the empty log-file. Here are the updated patches.
> 
> The problem was this line:
> >+                        "--nodaemonize" "-p" "/var" "--fpm-config"  
> 
> When --nodaemonize is set, the log is printed to stdout instead.
> I also removed -p "/var". It is not required when the log, pid and
> socket locations are defined in the config.
> Both were caused, because I didn't use pid-files from the start.
> 
> Happy Hacking!

Hey,

Just wanted to say I've started looking at this again, I'll hopefully
get around to replying later today, or tomorrow.

Chris

[Message part 2 (application/pgp-signature, inline)]

Message #26 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: nee <nee <at> cock.li>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Thu, 2 Nov 2017 19:17:08 +0000

[Message part 1 (text/plain, inline)]

On Mon, 16 Oct 2017 23:38:50 +0200
nee <nee <at> cock.li> wrote:

I've also read your subsequent message about fixing the log files, but
I'll reply here, as there is more to reply to.

> Hello, here is an updated version.

...

> > Also, the idea that came to my mind for this is to add php-fpm as a
> > supplementary group for the nginx user. In my mind, this seems the
> > neatest way of giving nginx access to the socket. What do you think?
> > 
> > I suggest this, as I'm not sure the name of the www-data group does a
> > good job of describing that it's controlling access to a socket. Also,
> > giving nginx explicit access to things owned by the php-fpm group could
> > be more secure than using a more generic group, especially as other
> > services that might need access to the socket, could end up getting it
> > because they need access to other things using the www-data group.
> >   
> Using more specialized groups sounds good in general, I changed the
> group name.
> It would be weird to have the nginx user in a bunch of groups for
> software that you did not install though.
> > If the above sounds like a good idea, I think it could be implemented
> > by adding a configurable list of supplementary groups to the
> > nginx-service-type. Then maybe even adding support for configuring this
> > via the service extensions mechanism, then having the
> > php-fpm-service-type extending the nginx-service-type...
> > 
> > The above is definitely to complicated to do all in one go, especially
> > since the nginx-service-type doesn't support anything more complicated
> > than adding server blocks through extension at the moment.
> >   
> This seems to be a solution.

I've now attached a system test for php-fpm, that sets it up with
nginx, creates a basic php file, and checks that it can be requested.

This seems to work, which I was a little surprised at, as I was
suspecting problems with the socket permissions.

I had a look, and while the nginx workers in the test system are not
root, the nginx master process is, so maybe that allows it to work...

Anyway, providing that the test reflects how the service might be used,
it seems like this is a non-issue for now.

> >> @@ -385,3 +397,160 @@ of index files."
> >>  		       (service-extension account-service-type
> >>                                            fcgiwrap-accounts)))
> >>                  (default-value (fcgiwrap-configuration))))

...

> >> +                                         (version-major
> >> (package-version php))
> >> +                                         "-fpm.log")))  
> > 
> > I'm not sure the php is adding much to the log file name, but then
> > again I've never used php... what do you think?
> >   
> For the workers-logfile:
> This is the logfile for the php workers error's
> It only logs errors that were printed with error_log by a php worker
> process.
> Try this example file:
> 
> <?php
> error_log("some error\n"); // should be in the log
> echo "Hello from php"; // should be in the browser
> 
> Then visit the err.php file with curl or a browser using the nginx
> example above.
> 
> Also:
> I forgot to add a setting for the log-file of the php-fpm master
> process. This file should log messages when php-fpm has started or stopped.
> 
> For some reason it stays empty now. I had it log stuff before, when I
> manually killed the process, but I can't reproduce it anymore. I changed
> the permissions to ugo+wrx and it still stays empty. I'm not sure what's
> going on.
> 
> I renamed the default workers-log-file to php7-fpm.www.log as it is
> usually called. The php-fpm log-file is now called php7-fpm.log.

What I think I meant to say here was, I'm not sure the php _version_ is
adding much to the log file name (rather than "I'm not sure the php is
adding").

The php package version is used in a few places, and while I can
imagine this being consistent with other distributions, it does add a
bit of complexity to the default values in the service, and I'm not
sure what benefit it brings?

> >> +  (file          php-fpm-configuration-file ;#f | file-like
> >> +                 (default #f)))

...

> >> +                       (service-extension account-service-type
> >> +                                          php-fpm-accounts)))
> >> +                (default-value (php-fpm-configuration))))  
> > 
> > Filling in the description (a relatively new field on the service type)
> > would be a great addition here.
> >   
> Ah, yes I'm mostly using the web documentation and other services from
> web.scm as reference. Thanks for the update.
> What would be a good value for this field? I just used "The php-fpm
> service-type." for now.

That is ok, but it could probably be more useful. I think ideally it
would describe more about what the service offers.

Users might encounter this when searching for services for example:

→ guix system search php
name: php-fpm
location: gnu/services/web.scm:607:2
extends: shepherd-root activate account
description: The php-fpm service-type.
relevance: 5


> >> +(define* (nginx-php-location
> >> +          #:key
> >> +          (nginx-package nginx)

...

> > Overall, I think this is great. Excellent use of record types, gexps
> > and match expressions. I think it would be good to think more on the
> > accounts issue before merging, but that is all that comes to mind
> > currently.
> >   
> Good to hear, thank you! :)
> 
> > Also, if you feel like it, as service test would be a great addition to
> > this patch. There is a test for nginx already in gnu/tests/web.scm, and
> > I think you could get most of the benefit by having a test for nginx
> > with php-fpm, as that would give you some coverage over the php-fpm
> > service, as well as the nginx configuration.
> >   
> That sounds great I love the idea of system tests and will have a look
> at it later.
> 
> I also added a line to copyright headers of each file. Except for the
> utils.scm file, because that change is trivial copy pasta.

I've attached a patch containing a couple of copy+paste fixes, and a
system test.

It would be good to get your opinion on the system test, does it test
the right things?

If you like the look of it, I'd suggest including those changes in the
commit that adds the service, and then sending the updated patches.
I've included a changelog in the commit message.

Now that there is a system test, and the php-fpm service seems to work
fine with the nginx service, I think that means that doing anything
extra with accounts can be done later.

[0001-Add-fixes-and-system-test-for-PHP-FPM.patch (text/x-patch, attachment)]

[Message part 3 (application/pgp-signature, inline)]

Message #29 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: nee <nee <at> cock.li>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Thu, 23 Nov 2017 21:11:48 +0100

[Message part 1 (text/plain, inline)]

Hello sorry about not replying for such a long time, and thank you for
reviewing my patches again.

Am 02.11.2017 um 20:17 schrieb Christopher Baines:
> 
> I've now attached a system test for php-fpm, that sets it up with
> nginx, creates a basic php file, and checks that it can be requested.

That's great thanks for saving me a bunch of work with this. ;-)

> 
> This seems to work, which I was a little surprised at, as I was
> suspecting problems with the socket permissions.
> 
> I had a look, and while the nginx workers in the test system are not
> root, the nginx master process is, so maybe that allows it to work...
I don't think that's the reason, because I remember it not working at
first when I didn't have the permissions set.

>> I renamed the default workers-log-file to php7-fpm.www.log as it is
>> usually called. The php-fpm log-file is now called php7-fpm.log.
> 
> What I think I meant to say here was, I'm not sure the php _version_ is
> adding much to the log file name (rather than "I'm not sure the php is
> adding").
> 
> The php package version is used in a few places, and while I can
> imagine this being consistent with other distributions, it does add a
> bit of complexity to the default values in the service, and I'm not
> sure what benefit it brings?
> 
If users want to run multiple php versions, they only have to change the
version in the php-package and pass that package along all the services.

My perception of the php landscape was that the major releases aren't
100% reliably backwards compatible and some applications depend on older
stable releases, so that it is not too uncommon to run multiple php
versions the same system.

Here is a quote from: https://wordpress.org/about/requirements/

"""
Why do we support older versions?

We strongly recommend the latest versions of PHP and MySQL, but we
understand that this isn’t right for everyone, and that sometimes hosts
can be slow or hesitant to upgrade their customers since upgrades to PHP
and MySQL have historically broken applications.
"""

An alternative could be to include the php-package hash in the socket
name, but I'm not sure if that would work with nginx and it's currently
missing reload when a system-reconfigure is done. Or services could be
generally more isolated somehow.

WDYT?

>>>> +  (file          php-fpm-configuration-file ;#f | file-like
>>>> +                 (default #f)))
> 
> ...
> 
>>>> +                       (service-extension account-service-type
>>>> +                                          php-fpm-accounts)))
>>>> +                (default-value (php-fpm-configuration))))  
>>>
>>> Filling in the description (a relatively new field on the service type)
>>> would be a great addition here.
>>>   
>> Ah, yes I'm mostly using the web documentation and other services from
>> web.scm as reference. Thanks for the update.
>> What would be a good value for this field? I just used "The php-fpm
>> service-type." for now.
> 
> That is ok, but it could probably be more useful. I think ideally it
> would describe more about what the service offers.
> 
> Users might encounter this when searching for services for example:
> 
> → guix system search php
> name: php-fpm
> location: gnu/services/web.scm:607:2
> extends: shepherd-root activate account
> description: The php-fpm service-type.
> relevance: 5

I ran `guix sysytem search *` and it seems most descriptions start with
'Run' or 'Provide' I changed it to:
 "Run `php-fpm' to provide a fastcgi socket for calling php through a
webserver."


> I've attached a patch containing a couple of copy+paste fixes, and a
> system test.
> 
> It would be good to get your opinion on the system test, does it test
> the right things?
> 
The tests look good to me.
I added another test that adds two numbers and checks for the result in
the response to see if php actually did something with the file.

> If you like the look of it, I'd suggest including those changes in the
> commit that adds the service, and then sending the updated patches.
> I've included a changelog in the commit message.
> 
Here are the updated patches. Thank you for your tests!

[0001-guix-utils-add-version-major.patch (text/x-patch, attachment)]

[0002-gnu-services-Add-php-fpm.patch (text/x-patch, attachment)]

Message #32 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: nee <nee <at> cock.li>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Sat, 09 Dec 2017 22:08:40 +0000

[Message part 1 (text/plain, inline)]

nee writes:

> Hello sorry about not replying for such a long time, and thank you for
> reviewing my patches again.
>
> Am 02.11.2017 um 20:17 schrieb Christopher Baines:
>>
>> I've now attached a system test for php-fpm, that sets it up with
>> nginx, creates a basic php file, and checks that it can be requested.
>
> That's great thanks for saving me a bunch of work with this. ;-)
>
>>
>> This seems to work, which I was a little surprised at, as I was
>> suspecting problems with the socket permissions.
>>
>> I had a look, and while the nginx workers in the test system are not
>> root, the nginx master process is, so maybe that allows it to work...
> I don't think that's the reason, because I remember it not working at
> first when I didn't have the permissions set.

Yep, my mistake. I didn't spot the changes to the nginx service.

>>> I renamed the default workers-log-file to php7-fpm.www.log as it is
>>> usually called. The php-fpm log-file is now called php7-fpm.log.
>>
>> What I think I meant to say here was, I'm not sure the php _version_ is
>> adding much to the log file name (rather than "I'm not sure the php is
>> adding").
>>
>> The php package version is used in a few places, and while I can
>> imagine this being consistent with other distributions, it does add a
>> bit of complexity to the default values in the service, and I'm not
>> sure what benefit it brings?
>>
> If users want to run multiple php versions, they only have to change the
> version in the php-package and pass that package along all the services.
>
> My perception of the php landscape was that the major releases aren't
> 100% reliably backwards compatible and some applications depend on older
> stable releases, so that it is not too uncommon to run multiple php
> versions the same system.
>
> Here is a quote from: https://wordpress.org/about/requirements/
>
> """
> Why do we support older versions?
>
> We strongly recommend the latest versions of PHP and MySQL, but we
> understand that this isn’t right for everyone, and that sometimes hosts
> can be slow or hesitant to upgrade their customers since upgrades to PHP
> and MySQL have historically broken applications.
> """
>
> An alternative could be to include the php-package hash in the socket
> name, but I'm not sure if that would work with nginx and it's currently
> missing reload when a system-reconfigure is done. Or services could be
> generally more isolated somehow.
>
> WDYT?

That sounds good to me. I don't know too much about this area though.

>>>>> +  (file          php-fpm-configuration-file ;#f | file-like
>>>>> +                 (default #f)))
>>
>> ...
>>
>>>>> +                       (service-extension account-service-type
>>>>> +                                          php-fpm-accounts)))
>>>>> +                (default-value (php-fpm-configuration))))
>>>>
>>>> Filling in the description (a relatively new field on the service type)
>>>> would be a great addition here.
>>>>
>>> Ah, yes I'm mostly using the web documentation and other services from
>>> web.scm as reference. Thanks for the update.
>>> What would be a good value for this field? I just used "The php-fpm
>>> service-type." for now.
>>
>> That is ok, but it could probably be more useful. I think ideally it
>> would describe more about what the service offers.
>>
>> Users might encounter this when searching for services for example:
>>
>> → guix system search php
>> name: php-fpm
>> location: gnu/services/web.scm:607:2
>> extends: shepherd-root activate account
>> description: The php-fpm service-type.
>> relevance: 5
>
> I ran `guix sysytem search *` and it seems most descriptions start with
> 'Run' or 'Provide' I changed it to:
>  "Run `php-fpm' to provide a fastcgi socket for calling php through a
> webserver."
>
>
>> I've attached a patch containing a couple of copy+paste fixes, and a
>> system test.
>>
>> It would be good to get your opinion on the system test, does it test
>> the right things?
>>
> The tests look good to me.
> I added another test that adds two numbers and checks for the result in
> the response to see if php actually did something with the file.
>
>> If you like the look of it, I'd suggest including those changes in the
>> commit that adds the service, and then sending the updated patches.
>> I've included a changelog in the commit message.
>>
> Here are the updated patches. Thank you for your tests!

Thanks for picking this up again. Unfortunately it's also take me a long
time to get back around to looking at this.

I've attached some changes that I thought would be good when I was
looking through this. To give a rough summary:

 - Minor improvements to the docs, either content, markup or formatting.
 - Removing trailing whitespace.
 - Removing the changes to the nginx-service, in favour of changing the
   default socket group.
 - Change some indentation to avoid long lines.

By changing the default socket group, the system test passes, even
without the changes to the nginx service. I think this is a bit better,
and while it's definately not perfect, I think it would be ok to merge
with this change.

To also try and move the first patch forward, I've submitted that within
#29629, with an additional patch to get other services using
version-major.

It would be good to get your thoughts on the changes in the attached
patch, and then if you could send an updated set of patches, that would
be great. As far as I remember, the changes to the nginx service were
the only thing I felt needed addressing before merging this.

Thanks,

Chris

[patch (text/x-patch, inline)]

diff --git a/doc/guix.texi b/doc/guix.texi
index f2ef941b4..dcab3bd76 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -15173,7 +15173,7 @@ with some additional features useful for sites of any size.
 These features include:
 @itemize @bullet
 @item Adaptive process spawning
-@item Basic statistics (ala Apache's mod_status)
+@item Basic statistics (similar to Apache's mod_status)
 @item Advanced process management with graceful stop/start
 @item Ability to start workers with different uid/gid/chroot/environment
 and different php.ini (replaces safe_mode)
@@ -15194,7 +15194,9 @@ A Service type for @code{php-fpm}.
 @deftp {Data Type} php-fpm-configuration
 Data Type for php-fpm service configuration.
 @table @asis
-@item @code {socket} (default: @code{(string-append "/var/run/php" (version-major (package-version php)) "-fpm.sock")})
+@item @code{php} (default: @code{php})
+The php package to use.
+@item @code{socket} (default: @code{(string-append "/var/run/php" (version-major (package-version php)) "-fpm.sock")})
 The address on which to accept FastCGI requests.  Valid syntaxes are:
 @table @asis
 @item @code{"ip.add.re.ss:port"}
@@ -15205,20 +15207,20 @@ Listen on a TCP socket to all addresses on a specific port.
 Listen on a unix socket.
 @end table
 
-@item @code {user} (default: @code{php-fpm})
+@item @code{user} (default: @code{php-fpm})
 User who will own the php worker processes.
-@item @code {group} (default: @code{php-fpm})
+@item @code{group} (default: @code{php-fpm})
 Group of the worker processes.
-@item @code {socket-user} (default: @code{php-fpm})
+@item @code{socket-user} (default: @code{php-fpm})
 User who can speak to the php-fpm socket.
-@item @code {socket-group} (default: @code{php-fpm})
+@item @code{socket-group} (default: @code{php-fpm})
 Group that can speak to the php-fpm socket.
-@item @code {pid-file} (default: @code{(string-append "/var/log/php" (version-major (package-version php)) "-fpm.pid")})
+@item @code{pid-file} (default: @code{(string-append "/var/run/php" (version-major (package-version php)) "-fpm.pid")})
 The process id of the php-fpm process is written to this file
 once the service has started.
-@item @code {log-file} (default: @code{(string-append "/var/log/php" (version-major (package-version php)) "-fpm.log")})
+@item @code{log-file} (default: @code{(string-append "/var/log/php" (version-major (package-version php)) "-fpm.log")})
 Log for the php-fpm master process.
-@item @code {process-manager} (default: @code{(php-fpm-dynamic-process-manager-configuration)})
+@item @code{process-manager} (default: @code{(php-fpm-dynamic-process-manager-configuration)})
 Detailed settings for the php-fpm process manager.
 Must be either:
 @table @asis
@@ -15226,62 +15228,66 @@ Must be either:
 @item @code{<php-fpm-static-process-manager-configuration>}
 @item @code{<php-fpm-on-demand-process-manager-configuration>}
 @end table
-@item @code {display-errors} (default @code{#f})
+@item @code{display-errors} (default @code{#f})
 Determines wether php errors and warning should be sent to clients
 and displayed in their browsers.
 This is useful for local php development, but a security risk for public sites,
 as error messages can reveal passwords and personal data.
-@item @code {workers-logfile} (default @code{(string-append "/var/log/php" (version-major (package-version php)) "-fpm.www.log")})
+@item @code{workers-logfile} (default @code{(string-append "/var/log/php" (version-major (package-version php)) "-fpm.www.log")})
 This file will log the @code{stderr} outputs of php worker processes.
 Can be set to @code{#f} to disable logging.
-@item @code {file} (default @code{#f})
+@item @code{file} (default @code{#f})
 An optional override of the whole configuration.
 You can use the @code{mixed-text-file} function or an absolute filepath for it.
 @end table
 @end deftp
 
 @deftp {Data type} php-fpm-dynamic-process-manager-configuration
-Data Type for the @code{dynamic} php-fpm process manager.
-With the @code{dynamic} process manager spare worker processes are kept around
+Data Type for the @code{dynamic} php-fpm process manager.  With the
+@code{dynamic} process manager, spare worker processes are kept around
 based on it's configured limits.
 @table @asis
-@item @code {max-children} (default: @code{5})
+@item @code{max-children} (default: @code{5})
 Maximum of worker processes.
-@item @code {start-servers} (default: @code{2})
+@item @code{start-servers} (default: @code{2})
 How many worker processes should be started on start-up.
-@item @code {min-spare-servers} (default: @code{1})
+@item @code{min-spare-servers} (default: @code{1})
 How many spare worker processes should be kept around at minimum.
-@item @code {max-spare-servers} (default: @code{3})
+@item @code{max-spare-servers} (default: @code{3})
 How many spare worker processes should be kept around at maximum.
 @end table
 @end deftp
 
 @deftp {Data type} php-fpm-static-process-manager-configuration
-Data Type for the @code{static} php-fpm process manager.
-With the @code{static} process manager an unchanging number
-of worker processes is created.
+Data Type for the @code{static} php-fpm process manager.  With the
+@code{static} process manager, an unchanging number of worker processes
+are created.
 @table @asis
-@item @code {max-children} (default: @code{5})
+@item @code{max-children} (default: @code{5})
 Maximum of worker processes.
 @end table
 @end deftp
 
 @deftp {Data type} php-fpm-on-demand-process-manager-configuration
-Data Type for the @code{on-demand} php-fpm process manager.
-With the @code{on-demand} process manager worker processes are only created
-as requests arrive.
+Data Type for the @code{on-demand} php-fpm process manager.  With the
+@code{on-demand} process manager, worker processes are only created as
+requests arrive.
 @table @asis
-@item @code {max-children} (default: @code{5})
+@item @code{max-children} (default: @code{5})
 Maximum of worker processes.
-@item @code {process-idle-timeout} (default: @code{10})
+@item @code{process-idle-timeout} (default: @code{10})
 The time in seconds after which a process with no requests is killed.
 @end table
 @end deftp
 
 
-@defvr {Scheme Variable} nginx-php-fpm-location
+@deffn {Scheme Procedure} nginx-php-fpm-location @
+       [#:nginx-package nginx] @
+       [socket (string-append "/var/run/php" @
+                              (version-major (package-version php)) @
+                              "-fpm.sock")]
 A helper function to quickly add php to an @code{nginx-server-configuration}.
-@end defvr
+@end deffn
 
 A simple services setup for nginx with php can look like this:
 @example
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 3b3be215a..00599df6d 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -98,7 +98,7 @@
             php-fpm-configuration-display-errors
             php-fpm-configuration-workers-log-file
             php-fpm-configuration-file
-            
+
             <php-fpm-dynamic-process-manager-configuration>
             php-fpm-dynamic-process-manager-configuration
             make-php-fpm-dynamic-process-manager-configuration
@@ -107,13 +107,13 @@
             php-fpm-dynamic-process-manager-configuration-start-servers
             php-fpm-dynamic-process-manager-configuration-min-spare-servers
             php-fpm-dynamic-process-manager-configuration-max-spare-servers
-            
+
             <php-fpm-static-process-manager-configuration>
             php-fpm-static-process-manager-configuration
             make-php-fpm-static-process-manager-configuration
             php-fpm-static-process-manager-configuration?
             php-fpm-static-process-manager-configuration-max-children
-            
+
             <php-fpm-on-demand-process-manager-configuration>
             php-fpm-on-demand-process-manager-configuration
             make-php-fpm-on-demand-process-manager-configuration
@@ -302,12 +302,10 @@ of index files."
           "events {}\n")))
 
 (define %nginx-accounts
-  (list (user-group (name "php-fpm") (system? #t))
-        (user-group (name "nginx") (system? #t))
+  (list (user-group (name "nginx") (system? #t))
         (user-account
          (name "nginx")
          (group "nginx")
-         (supplementary-groups '("php-fpm"))
          (system? #t)
          (comment "nginx server user")
          (home-directory "/var/empty")
@@ -450,7 +448,7 @@ of index files."
   (socket-user      php-fpm-configuration-socket-user
                     (default "php-fpm"))
   (socket-group     php-fpm-configuration-socket-group
-                    (default "php-fpm"))
+                    (default "nginx"))
   (pid-file         php-fpm-configuration-pid-file
                     (default (string-append "/var/run/php"
                                             (version-major (package-version php))
@@ -542,13 +540,13 @@ of index files."
               "pm.start_servers =" (number->string pm.start-servers) "\n"
               "pm.min_spare_servers =" (number->string pm.min-spare-servers) "\n"
               "pm.max_spare_servers =" (number->string pm.max-spare-servers) "\n"))
-            
+
             (($ <php-fpm-static-process-manager-configuration>
                 pm.max-children)
              (list
               "pm = static\n"
               "pm.max_children =" (number->string pm.max-children) "\n"))
-            
+
             (($ <php-fpm-on-demand-process-manager-configuration>
                 pm.max-children
                 pm.process-idle-timeout)
@@ -604,16 +602,19 @@ of index files."
 
 
 (define php-fpm-service-type
-  (service-type (name 'php-fpm)
-                (description "Run `php-fpm' to provide a fastcgi socket for calling php through a webserver.")
-                (extensions
-                 (list (service-extension shepherd-root-service-type
-                                          php-fpm-shepherd-service)
-                       (service-extension activation-service-type
-                                          php-fpm-activation)
-                       (service-extension account-service-type
-                                          php-fpm-accounts)))
-                (default-value (php-fpm-configuration))))
+  (service-type
+   (name 'php-fpm)
+   (description
+    "Run @command{php-fpm} to provide a fastcgi socket for calling php through
+a webserver.")
+   (extensions
+    (list (service-extension shepherd-root-service-type
+                             php-fpm-shepherd-service)
+          (service-extension activation-service-type
+                             php-fpm-activation)
+          (service-extension account-service-type
+                             php-fpm-accounts)))
+   (default-value (php-fpm-configuration))))
 
 (define* (nginx-php-location
           #:key

[signature.asc (application/pgp-signature, inline)]

Message #35 received at 28769 <at> debbugs.gnu.org (full text, mbox):

From: nee <nee <at> cock.li>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 28769 <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Mon, 11 Dec 2017 19:19:10 +0100

Am 09.12.2017 um 23:08 schrieb Christopher Baines:
> I've attached some changes that I thought would be good when I was
> looking through this. To give a rough summary:
> 
>  - Minor improvements to the docs, either content, markup or formatting.
>  - Removing trailing whitespace.
>  - Removing the changes to the nginx-service, in favour of changing the
>    default socket group.
>  - Change some indentation to avoid long lines.
> 
> By changing the default socket group, the system test passes, even
> without the changes to the nginx service. I think this is a bit better,
> and while it's definately not perfect, I think it would be ok to merge
> with this change.
> 
Looks all good to me. Thanks for all these style fixes.

> To also try and move the first patch forward, I've submitted that within
> #29629, with an additional patch to get other services using
> version-major.
> 
Thanks, good to see it used at some other spots already.

> It would be good to get your thoughts on the changes in the attached
> patch, and then if you could send an updated set of patches, that would
> be great. As far as I remember, the changes to the nginx service were
> the only thing I felt needed addressing before merging this.

The changes look good to me.
I have no edits to add, imo the patches can be squashed merged.

Message #40 received at 28769-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: nee <nee <at> cock.li>
Cc: 28769-done <at> debbugs.gnu.org
Subject: Re: [bug#28769] [PATCH] gnu: services: Add php-fpm.
Date: Tue, 12 Dec 2017 21:41:41 +0000

[Message part 1 (text/plain, inline)]

nee <nee <at> cock.li> writes:

> Am 09.12.2017 um 23:08 schrieb Christopher Baines:
>> I've attached some changes that I thought would be good when I was
>> looking through this. To give a rough summary:
>> 
>>  - Minor improvements to the docs, either content, markup or formatting.
>>  - Removing trailing whitespace.
>>  - Removing the changes to the nginx-service, in favour of changing the
>>    default socket group.
>>  - Change some indentation to avoid long lines.
>> 
>> By changing the default socket group, the system test passes, even
>> without the changes to the nginx service. I think this is a bit better,
>> and while it's definately not perfect, I think it would be ok to merge
>> with this change.
>> 
> Looks all good to me. Thanks for all these style fixes.
>
>> To also try and move the first patch forward, I've submitted that within
>> #29629, with an additional patch to get other services using
>> version-major.
>> 
> Thanks, good to see it used at some other spots already.
>
>> It would be good to get your thoughts on the changes in the attached
>> patch, and then if you could send an updated set of patches, that would
>> be great. As far as I remember, the changes to the nginx service were
>> the only thing I felt needed addressing before merging this.
>
> The changes look good to me.
> I have no edits to add, imo the patches can be squashed merged.

Great, I've now rebased and pushed this. Good job :)

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.