| Message ID | 0b81168e693a21f7fb08bd7677a5cc3120418fc9.1707626620.git.maxim.cournoyer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#68946,v2] guix: Add logging module. | expand |
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 ? Aside, just to be sure to well understand, this introduces the logger but it is not used yet, right? > - 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. > +;;; 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. > +(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? > + (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. (also -sPARAMTER or -s PARAMETER)
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.
Hi! Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > * 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. > > Change-Id: I5026a0d62119615fec3cd0131309f9bcc346a7e9 Two quick notes: I think it’s be best to avoid depending on Guile-Lib “just” for this (it’s not clear to me what it’s maintenance status is), and it’d be great to have concrete use case. My concern with logging is that it’s not a good fit for the functional parts (unless it has a functional interface like ‘pk’, but that’s still sorta “cluttering” otherwise functional code), and I’m not sure which imperative part it could be used for (I don’t doubt there’s such a thing, but I lack imagination). Thanks, Ludo’.
Hi Maxim, On ven., 16 févr. 2024 at 14:03, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > Most of that code also exists in guile-hall, was released in 2023, hence > the copyright year start [0]. I think it would be best something like: --8<---------------cut here---------------start------------->8--- ;;; Copyright © 2024 Maxim Cournoyer <maxim.cournoyer@gmail.com> ;;; ;;; The procedures ’define-log-level', ’setup-logging’ and ;;; ’shutdown-logging’ are taken from the 'hall/logging.scm' file of ;;; Guile-lib (guile-hall). That file has the following copyright ;;; notice: ;;; ;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com> ;;; ;;; This file is part of Guix. ;;; ;;; GNU Guix is distributed in the hope that it will be useful, but … --8<---------------cut here---------------end--------------->8--- > [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. Ah? For instance ’define-with-syntax-properties’ or ’define-diagnostic’ or ’leave’ from (guix diagnostics); other instance in (gnu build accounts) or (gnu build secret-service) etc. Do I miss something? >> 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. I think that’s a bad choice. For two reasons, 1. Let’s keep an uniform across subcommands 2. It potentially opens bug as #50472; no space avoids ambiguous implementations as explained. :-) https://issues.guix.gnu.org/issue/50472 Cheers, simon]
Hi Ludovic, Ludovic Courtès <ludo@gnu.org> writes: > Hi! > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> * 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. >> >> Change-Id: I5026a0d62119615fec3cd0131309f9bcc346a7e9 > > Two quick notes: I think it’s be best to avoid depending on Guile-Lib > “just” for this (it’s not clear to me what it’s maintenance status is), > and it’d be great to have concrete use case. David Pirotte is doing great for Guile-Lib maintenance. It's better maintained, than say, Guile-Git, which we also depend on :-). It has a good test suite and is small. We already depend on it for a couple of our updaters, for its htmlprag HTML parser, so combined with this log facility, that seems enough to justify hard depending on it. > My concern with logging is that it’s not a good fit for the functional > parts (unless it has a functional interface like ‘pk’, but that’s still > sorta “cluttering” otherwise functional code), and I’m not sure which > imperative part it could be used for (I don’t doubt there’s such a > thing, but I lack imagination). I understand the concern, but I think to be able to trace execution easily at run time with a simple `--log-level=debug' would provide value enough (as a developer aid) to justify it.
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >> My concern with logging is that it’s not a good fit for the functional >> parts (unless it has a functional interface like ‘pk’, but that’s still >> sorta “cluttering” otherwise functional code), and I’m not sure which >> imperative part it could be used for (I don’t doubt there’s such a >> thing, but I lack imagination). > > I understand the concern, but I think to be able to trace execution > easily at run time with a simple `--log-level=debug' would provide value > enough (as a developer aid) to justify it. Again, I lack imagination; where would you use it today? I think we should only add this facility once it has at least one user. Otherwise people using ‘--log-level’ would be disappointed. :-) Ludo’.
Hi Ludo, Ludovic Courtès <ludo@gnu.org> writes: > Hi, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >>> My concern with logging is that it’s not a good fit for the functional >>> parts (unless it has a functional interface like ‘pk’, but that’s still >>> sorta “cluttering” otherwise functional code), and I’m not sure which >>> imperative part it could be used for (I don’t doubt there’s such a >>> thing, but I lack imagination). >> >> I understand the concern, but I think to be able to trace execution >> easily at run time with a simple `--log-level=debug' would provide value >> enough (as a developer aid) to justify it. > > Again, I lack imagination; where would you use it today? > > I think we should only add this facility once it has at least one user. > Otherwise people using ‘--log-level’ would be disappointed. :-) OK; I've added some basic logging to get the ball rolling; it's currently limited to showing the parsed arguments used with the guix command run, it looks like so: --8<---------------cut here---------------start------------->8--- $ ./pre-inst-env guix --log-level=debug build hello 2024-02-25 02:58:13 guix/logging.scm:92:5: (run-guix-command) DEBUG: logging initialized 2024-02-25 02:58:13 guix/ui.scm:2296:11: (run-guix-command) INFO: running guix command `build' with args: (hello) 2024-02-25 02:58:13 guix/scripts/build.scm:741:3: (guix-build) INFO: 'guix build' parsed options: ((argument . hello) (build-mode . 0) (graft? . #t) (substitutes? . #t) (offload? . #t) (print-build-trace? . #t) (print-extended-build-trace? . #t) (multiplexed-build-output? . #t) (verbosity . 3) (debug . 0)) /gnu/store/kf9cgiq5a2bbxf89h72scl6b5f4jmjpf-hello-2.12.1 --8<---------------cut here---------------end--------------->8--- While this is not super useful yet, we can see where execution started, and which options were passed to the lower level command API, which could be a helpful entry-point discoverability hint to a new hacker wanting to add a switch or look into how a particular command is implemented. I'll send a v3 with it soon.
diff --git a/Makefile.am b/Makefile.am index cef972880c..6929ee5f69 100644 --- a/Makefile.am +++ b/Makefile.am @@ -14,7 +14,7 @@ # Copyright © 2018 Oleg Pykhalov <go.wigust@gmail.com> # Copyright © 2018 Alex Vong <alexvong1995@gmail.com> # Copyright © 2019, 2023 Efraim Flashner <efraim@flashner.co.il> -# Copyright © 2020, 2021, 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com> +# Copyright © 2020, 2021, 2023, 2024 Maxim Cournoyer <maxim.cournoyer@gmail.com> # Copyright © 2021 Chris Marusich <cmmarusich@gmail.com> # Copyright © 2021 Andrew Tropin <andrew@trop.in> # Copyright © 2023 Clément Lassieur <clement@lassieur.org> @@ -125,7 +125,8 @@ MODULES = \ guix/substitutes.scm \ guix/upstream.scm \ guix/licenses.scm \ - guix/lint.scm \ + guix/lint.scm \ + guix/logging.scm \ guix/glob.scm \ guix/git.scm \ guix/git-authenticate.scm \ diff --git a/configure.ac b/configure.ac index ecbd596a34..133692c033 100644 --- a/configure.ac +++ b/configure.ac @@ -149,12 +149,12 @@ if test "x$guix_cv_have_recent_guile_git" != "xyes"; then AC_MSG_ERROR([A recent Guile-Git could not be found; please install it.]) fi -dnl Check for the optional Guile-Lib. +dnl Require Guile-Lib, for logging and HTML parsing. +GUILE_MODULE_REQUIRED([logging logger]) GUILE_MODULE_EXPORTS([have_guile_lib], [(htmlprag)], [%strict-tokenizer?]) AM_CONDITIONAL([HAVE_GUILE_LIB], [test "x$have_guile_lib" = "xyes"]) AM_COND_IF(HAVE_GUILE_LIB,, - [AC_MSG_WARN([The Guile-Lib requirement was not satisfied (>= 0.2.7); -Some features such as the Go importer will not be usable.])]) + [AC_MSG_ERROR([The Guile-Lib requirement was not satisfied (>= 0.2.7)])]) dnl Check for Guile-zlib. GUIX_CHECK_GUILE_ZLIB diff --git a/guix/logging.scm b/guix/logging.scm new file mode 100644 index 0000000000..913bc9ed3f --- /dev/null +++ b/guix/logging.scm @@ -0,0 +1,98 @@ +;;; GNU Guix --- Functional package management for GNU +;;; Copyright © 2023, 2024 Maxim Cournoyer <maxim.cournoyer@gmail.com> +;;; +;;; This file is part of GNU Guix. +;;; +;;; GNU Guix is free software; you can redistribute it and/or modify it +;;; under the terms of the GNU General Public License as published by +;;; the Free Software Foundation; either version 3 of the License, or (at +;;; your option) any later version. +;;; +;;; GNU Guix is distributed in the hope that it will be useful, but +;;; WITHOUT ANY WARRANTY; without even the implied warranty of +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;;; GNU General Public License for more details. +;;; +;;; You should have received a copy of the GNU General Public License +;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. + +(define-module (guix logging) + #:use-module (logging logger) + #:use-module (logging port-log) + + #:use-module (oop goops) + #:use-module (srfi srfi-209) + + #:export (setup-logging + shutdown-logging + + log-level + log-debug + log-info + log-warning + log-error + log-critical)) + +(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. + (lambda (x) + (define-syntax-rule (id parts ...) + ;; Assemble PARTS into a raw (unhygienic) identifier. + (datum->syntax x (symbol-append (syntax->datum parts) ...))) + + (syntax-case x () + ((_ enum-name (level ...)) + #`(begin + (define enum-name + (make-enum-type '(level ...))) + + #,@(map (lambda (lvl) + (with-syntax ((log (id 'log- lvl)) + (lvl lvl)) + #'(define-syntax log + (lambda (y) + (syntax-case y () + ((log . args) + #`(log-msg + '#,(datum->syntax + y (syntax-source #'log)) + 'lvl #,@#'args))))))) + #'(level ...))))))) + +(define-log-level log-level + (debug info warning error critical)) + +(define* (setup-logging #:key (level 'warning)) + "Configure and open logger at LEVEL)." + (let* ((level-enum (or (enum-name->enum log-level level) + (begin + (format (current-error-port) + "error: invalid log level~%") + (exit 1)))) + (lgr (make <logger>)) + (console (make <port-log> #:port (current-error-port))) + (levels-count (enum-type-size log-level))) + + ;; Register logging handlers. + (add-handler! lgr console) + + ;; Configure the log level. + (let loop ((enum (enum-min log-level))) + (let ((lvl (enum-name enum))) + (unless (eq? level lvl) + (disable-log-level! lgr lvl) + (loop (enum-next enum))))) + + ;; Activate the configured logger. + (set-default-logger! lgr) + (open-log! lgr) + (log-debug "logging initialized"))) + +(define (shutdown-logging) + "Flush and destroy logger." + (flush-log) + (close-log!) + (set-default-logger! #f)) 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) #:use-module (guix packages) #:use-module (guix profiles) #:use-module (guix derivations) @@ -2225,6 +2226,9 @@ (define (show-guix-help) -h, --help display this helpful text again and exit")) (display (G_ " -V, --version display version and copyright information and exit")) + (display (G_ " + --log-level=LEVEL set the logging verbosity to LEVEL, one of + debug, warning, error or critical")) (newline) (newline) @@ -2248,6 +2252,10 @@ (define (show-guix-help) categories)) (show-bug-report-information)) +(define %log-level (make-parameter (or (and=> (getenv "GUIX_LOG_LEVEL") + string->symbol) + 'warning))) + (define (run-guix-command command . args) "Run COMMAND with the given ARGS. Report an error when COMMAND is not found." @@ -2282,10 +2290,12 @@ (define (run-guix-command command . args) (symbol-append 'guix- command)))) (parameterize ((program-name command)) (dynamic-wind - (const #f) + (lambda () + (setup-logging #:level (%log-level))) (lambda () (apply command-main args)) (lambda () + (shutdown-logging) ;; Abuse 'exit-hook' (which is normally meant to be used by the ;; REPL) to run things like profiling hooks upon completion. (run-hook exit-hook)))))) @@ -2311,6 +2321,13 @@ (define (run-guix . args) (leave-on-EPIPE (show-guix-help))) ((or ("-V") ("--version")) (show-version-and-exit "guix")) + (((? (cut string-prefix? "--log-level=" <>) o) args ...) + (parameterize ((%log-level (string->symbol + (second (string-split o #\=))))) + (apply run-guix args))) + (("--log-level" level args ...) + (parameterize ((%log-level (string->symbol level))) + (apply run-guix args))) (((? option? o) args ...) (format (current-error-port) (G_ "guix: unrecognized option '~a'~%") o)