[bug#68946] [PATCH v2] guix: Add logging module.

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi,
>
> On sam., 10 févr. 2024 at 23:43, Maxim Cournoyer <maxim.cournoyer@gmail.com> 
> wrote:
>> * configure.ac: Require Guile-Lib.
>> * guix/logging.scm: New module.
>> * Makefile.am (MODULES): Register it.
>> * guix/ui.scm (show-guix-help): Document --log-level global option.
>> (%log-level): New parameter.
>> (run-guix-command): Init logging.
>> (run-guix): Parse new --log-level option.
>
> Cool!
>
> How to synchronize this patch with the Guile-Lib counter-part
>
>         [Guile-Lib PATCH v3 0/7] Make log-msg accept source properties for 
> displaying source location
>         Maxim Cournoyer <maxim.cournoyer@gmail.com>
>         Thu, 08 Feb 2024 23:50:41 -0500
>         id:20240209045150.17210-1-maxim.cournoyer@gmail.com
>
> ?

I have commit access to guile-lib; I've pinged its maintainer to see if
they have any objection, and otherwise I'll merge it soon.

> Aside, just to be sure to well understand, this introduces the logger
> but it is not used yet, right?

Correct.

>
>> -  guix/lint.scm                             \
>> +  guix/lint.scm                                     \
>> +  guix/logging.scm                          \
>
> Well, I assume being the nitpick person here. :-)
>
> The line guix/lint.scm should not change here.

I fixed the number of tabs on the line above as it looked odd in Emacs
:-).

>> +;;; Copyright © 2023, 2024 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>
> I think what makes the date of the Copyright is something publicly
> declared (published) and not the date when the author started to work on
> it.  At least, it is how it works for book in France.
>
> So here the Copyright would be 2024 only, I guess.

Most of that code also exists in guile-hall, was released in 2023, hence
the copyright year start [0].

[0]  
https://gitlab.com/a-sassmannshausen/guile-hall/-/blob/master/hall/logging.scm

>> +(define-syntax define-log-level
>> +  ;; This macro defines a log-level enum type bound to ENUM-NAME for the
>> +  ;; provided levels.  The levels should be specified in increasing order of
>> +  ;; severity.  It also defines 'log-LEVEL' syntax to more conveniently log 
>> at
>> +  ;; LEVEL, with location information.
>
> Why not also a docstring?

Only procedures can have docstrings, unfortunately.

>> +  (lambda (x)
>> +    (define-syntax-rule (id parts ...)
>> +      ;; Assemble PARTS into a raw (unhygienic) identifier.
>> +      (datum->syntax x (symbol-append (syntax->datum parts) ...)))
>
>> diff --git a/guix/ui.scm b/guix/ui.scm
>> index 962d291d2e..f5a6966854 100644
>> --- a/guix/ui.scm
>> +++ b/guix/ui.scm
>> @@ -44,6 +44,7 @@ (define-module (guix ui)
>>    #:use-module (guix utils)
>>    #:use-module (guix store)
>>    #:use-module (guix config)
>> +  #:use-module (guix logging)
>
> [...]
>
>> +      (((? (cut string-prefix? "--log-level=" <>) o) args ...)
>> +       (parameterize ((%log-level (string->symbol
>> +                                   (second (string-split o #\=)))))
>> +         (apply run-guix args)))
>> +      (("--log-level" level args ...)
>
> This make “--log-level debug” valid, right?
>
> I think the convention is --long-option[=PARAMETER] and that
> --long-option PARAMTER is unconventional.  Although I do not find the
> reference.

It's not as much a convention as a limitation of the SRFI 37 option
parser.  GNU getopt, which SRFI 37 aims to emulate,  doesn't have such a
limitation, for example.  We should improve SRFI 37 to lift such
limitation, in my opinion.

-- 
Thanks,
Maxim