Re: [PATCH] gnu: rottlog: rotate messages daily.

[Ludovic Courtès]

Hello!

Jan Nieuwenhuizen <address@hidden> skribis:

> Ludovic Courtès writes:

[...]

>>> +  (jobs rottlog-jobs                             ; list of <mcron-job>
>>> +        (default
>>> +          (list #~(job
>>> +                   '(next-hour '(0))
>>> +                   (lambda ()
>>> +                     (system (string-append #$rottlog "/sbin/rottlog"))))
>>> +                #~(job
>>> +                   '(next-hour '(12))
>>> +                   (lambda ()
>>> +                     (system (string-append #$rottlog 
>>> "/sbin/rottlog"))))))))
>>
>> Please move (list …) to a global variable, to avoid code duplication
>> when the macro is expanded.
>
> ...moved to a function now...but I don't see what macro you mean (#~ ?)
> and when it gets expanded and how that leads to duplication.

The ‘rottlog-configuration’ is actually a macro.  So at every call site
where a default value is used, the default value code is duplicated,
leading to code bloat if that default value is a big expression.

> I'm not sure if we should export the %default-rotations or if we should
> describe their contents in the manual.

Probably worth exporting and documenting, so people can ‘cons’ on it.

> From ee1be88f60d70de46009069da020c1bdc4993fd8 Mon Sep 17 00:00:00 2001
> From: Jan Nieuwenhuizen <address@hidden>
> Date: Thu, 8 Sep 2016 01:20:43 +0200
> Subject: [PATCH] gnu: services: add rottlog.
>
> * gnu/services/admin.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> * doc/guix.texi (Log Rotation): Document it.

[...]

> +(define (rotation-config file kill)
> +  (string-append file " {
> +     sharedscripts
> +     postrotate
> +" kill
> +"    endscript
> +     nocompress
> +}
> +"))
> +
> +(define (syslog-rotation-config file)
> +  (let ((coreutils 
> "/gnu/store/56x9fvx59i300wav3c193h84cp80bslr-coreutils-8.25")) ;; FIXME

Good point…

> +    (rotation-config
> +     file
> +     (string-append
> +      "      "
> +      coreutils "/bin/kill -HUP $(cat /var/run/syslog.pid) 2> /dev/null
> +"))))
> +
> +(define (simple-rotation-config file)
> +  (rotation-config file ""))
> +
> +(define %default-rotations
> +  `(("weekly" ,(plain-file "rottlog.weekly"
> +                           (string-append (string-join
> +                                           (map syslog-rotation-config
> +                                                %rotated-files)
> +                                           "")
> +                                          (simple-rotation-config
> +                                           "/var/log/shepherd.log"))))))

I think we cannot use ‘plain-file’ here because of the computed ‘kill’
file name.  So instead, this would be something along the lines of
(moving ‘string-append’ from the host side to the build side):

  (define (syslog-rotation-config file)
    #~(string-append #$file " {\n" …
                     #$coreutils "/bin/kill -HUP …"
                     "}\n"))

and:

  (define %default-rotations
    `(("weekly" ,(computed-file "rottlog.weekly"
                                #~(call-with-output-file #$output
                                    (lambda (port)
                                      (display #$(syslog-rotation-file …)
                                               port)))))))
HTH!

Thanks again for taking the time and coping with half-baked advice!
;-)

Ludo’.