On Fri, 28 Jul 2017 21:46:52 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> Christopher Baines <mail@cbaines.net> skribis:
> 
> > * gnu/services/memcached.scm: New file.
> > * gnu/tests/memcached.scm: New file.
> > * doc/guix.texi (Cache Services): New node.
> > * gnu/local.mk (GNU_SYSTEM_MODULES): Add entry for
> > services/memcached.scm and tests/memcached.scm.  
> 
> [...]
> 
> > +@node Cache Services
> > +@subsubsection Cache Services
> > +
> > +@cindex cache  
> 
> Please write a couple of introductory sentences here.  :-)
> 
> I was going to suggest to document it under “Web”, but I guess this is
> not inherently web-specific, so it’s probably better this way.

I've moved things to the databases place everywhere, as I think that is
ok.

> > +(define-record-type* <memcached-configuration>
> > +  memcached-configuration make-memcached-configuration
> > +  memcached-configuration?
> > +  (memcached          memcached-configuration-memcached ;<package>
> > +                      (default memcached))
> > +  (interfaces         memcached-configuration-interfaces
> > +                      (default '("0.0.0.0")))  
> 
> Should it default to 127.0.0.1 to avoid bad surprises?

It could be, I set the default to the upstream default as a first step.
I'd be fine with only listening locally by default if we make a
specific decision to do that.

> > +              (start #~(make-forkexec-constructor
> > +                        `(#$(file-append memcached
> > "/bin/memcached")
> > +                          "-l" #$(string-join interfaces ",")
> > +                          "-p" #$(number->string tcp-port)
> > +                          "-U" #$(number->string udp-port)
> > +                          "-u" "memcached"
> > +                          ,#$@additional-options)
> > +                        #:log-file "/var/log/memcached"))
> > +              (stop #~(make-kill-destructor))))))))  
> 
> If memcached has an option to create a PID file, it’s better to use it
> and pass #:pid-file here (makes sure memcached is really listening
> when the service is started.)

Yep, I forgot about that. It does support PID files, so I'll use that.

> Perhaps a good candidate for ‘make-forkexec-constructor/container’?
> We can check that afterwards though.

Tried it, but the service fails to start with no log output. I'll leave
this for now, but might try and do some more debugging at some point.

> > +(define %test-memcached
> > +  (system-test
> > +   (name "memcached")
> > +   (description "Connect to a running MEMCACHED server.")
> > +   (value (run-memcached-test))))  
> 
> Awesome.
> 
> OK with these changes, thank you!

I'll send updated patches.