On Thu, 03 Aug 2017 18:20:31 +0300
Oleg Pykhalov <go.wigust@gmail.com> wrote:

> Hello Christopher,
> 
> Christopher Baines <mail@cbaines.net> writes:
> 
> > On Sat, 29 Jul 2017 14:03:49 +0300
> > Oleg Pykhalov <go.wigust@gmail.com> wrote:
> >  
> >> >> +@deffn {Scheme Variable} rsync-service-type
> >> >> +This is the type for the @uref{https://rsync.samba.org} rsync
> >> >> daemon, +@command{rsync-configuration} record as in this
> >> >> example: +
> >> >> +@example
> >> >> +(service rsync-service-type
> >> >> +         (rsync-configuration))
> >> >> +@end example
> >> >> +
> >> >> +See below for details about @code{rsync-configuration}.
> >> >> +@end deffn
> >> >> +
> >> >> +@deftp {Data Type} rsync-configuration
> >> >> +Data type representing the configuration for
> >> >> @code{rsync-service}. +
> >> >> +@table @asis
> >> >> +@item @code{package} (default: @var{rsync})
> >> >> +Package object of the Rsync utility for efficiently
> >> >> transferring and +synchronizing files.    
> >> >
> >> > Object doesn't really fit here, if anything its a record. Also, I
> >> > don't think this needs a description of what rsync does, it's
> >> > probably clearest to just say that this is the rsync package?    
> >> 
> >> OK, is it good?
> >> 
> >>     @code{rsync} package to use.  
> >
> > Yep, in my opinion, that is the right amount of information.
> >  
> >> >> +(define (rsync-config-file config)
> >> >> +  "Return the rsync configuration file corresponding to
> >> >> CONFIG."
> >> >> +  (computed-file
> >> >> +   "rsync.conf"
> >> >> +   #~(begin
> >> >> +       (call-with-output-file #$output
> >> >> +         (lambda (port)
> >> >> +           (display "# Generated by 'rsync-service'.\n" port)
> >> >> +           (format port "pid file = ~a\n"
> >> >> +                   #$(rsync-configuration-pid-file config))
> >> >> +           (format port "lock file = ~a\n"
> >> >> +                   #$(rsync-configuration-lock-file config))
> >> >> +           (format port "log file = ~a\n"
> >> >> +                   #$(rsync-configuration-log-file config))
> >> >> +           (format port "port = ~a\n"
> >> >> +                   #$(number->string
> >> >> +                      (rsync-configuration-port-number
> >> >> config)))
> >> >> +           (format port "use chroot = ~a\n"
> >> >> +                   #$(if (rsync-configuration-use-chroot?
> >> >> config)
> >> >> +                         "true" "false"))
> >> >> +           (display "[files]\n" port)
> >> >> +           (format port "path = ~a\n"
> >> >> +                   #$(rsync-configuration-share-path config))
> >> >> +           (format port "comment = ~a\n"
> >> >> +                   #$(rsync-configuration-share-comment
> >> >> config))
> >> >> +           (format port "read only = ~a\n"
> >> >> +                   #$(if (rsync-configuration-read-only?
> >> >> config)
> >> >> +                         "true" "false"))
> >> >> +           (format port "timeout = ~a\n"
> >> >> +                   #$(number->string
> >> >> +                      (rsync-configuration-timeout config)))
> >> >> +           #t)))))    
> >> >
> >> > It might be neater here to use mixed-text-file here. It might
> >> > look something like...
> >> >
> >> > (define (rsync-config-file config)
> >> >   "Return the rsync configuration file corresponding to CONFIG."
> >> >   (match config
> >> >     (($ <rsync-configuration> package port-number pid-file
> >> > lock-file log-file use-chroot? share-path
> >> >                               share-comment read-only? timeout)
> >> >      (mixed text-file "rsync.conf"
> >> >       "# Generated by 'rsync-service'.\n"
> >> >       "pid file = " pid-file "\n"
> >> >       "lock file = " lock-file "\n"
> >> >       "log file = " log-file "\n"
> >> >       "port = " (number->string port-number) "\n"
> >> >       "use chroot = " (if use-chroot? "true" "false") "\n"
> >> >       "[files]\n"
> >> >       "path = " share-path "\n"
> >> >       "comment = " share-comment "\n"
> >> >       "read only = " (if read-only? "true" "false") "\n"
> >> >       "timeout = " (number->string timeout) "\n"))))  
> 
> OK, patched.
> 
> >> > One thing I tried with the Tailon service, was to define a
> >> > gexp-compiler for the record type representing the configuration
> >> > file. One really big advantage in the case of Tailon is that it
> >> > easily allows the use of a custom file, just by providing a
> >> > different gexp.    
> >> 
> >> So, this way you could provide your existing Tailon config as a
> >> file in system declaration?  Nice.  
> >
> > Yep, I think something like the following should just work, but I
> > haven't tested it yet.
> >
> >   (service tailon-service-type
> >            (tailon-configuration
> >              (config-file (local-file "../tailon.yml"))))
> >
> > I bring this up, as I think it is great if this is a feature of all
> > services, I'm having trouble though working out how you might do
> > that in this case, with the use of values in the activation service
> > extension. I can see complicated approaches, but nothing simple and
> > clear enough.  
> 
> I tried to apply your work in tailon service, but I have troubles as
> you described.  So I just cleaned up code little bit.

Those changes are looking good :) Could you send the overall patch, as
that makes it easier at least for me to review and test?