Some GWL sub-commands such as `graph' do not accept a log event configuration. This results in returning `#f' from `(%config 'log-events)' Fix this by checking that 'log-events was configured. --- gwl/ui.scm | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gwl/ui.scm b/gwl/ui.scm index 35bd127..92cf451 100644 --- a/gwl/ui.scm +++ b/gwl/ui.scm @@ -65,8 +65,10 @@ (define (log-event type . message) (define print? - (or (member 'all (%config 'log-events)) - (member type (%config 'log-events)))) + (let ((log-events (%config 'log-events))) + (and log-events + (or (member 'all (%config 'log-events)) + (member type (%config 'log-events)))))) (when print? (case type ((error) -- 2.36.1
Olivier Dion <olivier.dion@polymtl.ca> writes:
> Some GWL sub-commands such as `graph' do not accept a log event configuration.
> This results in returning `#f' from `(%config 'log-events)'
I don’t understand how this leads to a problem. Do any of the features
of “graph” use “log-event”?
Generally, I’d prefer to give “log-events” a better default. I can’t
imagine a situation where #F (rather than the empty list) would be
appropriate.
--
Ricardo
On Sat, 04 Jun 2022, Ricardo Wurmus <rekado@elephly.net> wrote: > Olivier Dion <olivier.dion@polymtl.ca> writes: > >> Some GWL sub-commands such as `graph' do not accept a log event configuration. >> This results in returning `#f' from `(%config 'log-events)' > > I don’t understand how this leads to a problem. Do any of the features > of “graph” use “log-event”? Yes. `load-workflow' -> `load-workflow*' -> `load*' -> `log-event 'info "Loading workflow file"'. > Generally, I’d prefer to give “log-events” a better default. I can’t > imagine a situation where #F (rather than the empty list) would be > appropriate. The empty list would be a more sane choice I agree. I'll send a v2 with this change. Also, the `print?` predicate could be memoize with `mlambda'? -- Olivier Dion oldiob.dev
Olivier Dion <olivier.dion@polymtl.ca> writes: > On Sat, 04 Jun 2022, Ricardo Wurmus <rekado@elephly.net> wrote: >> Olivier Dion <olivier.dion@polymtl.ca> writes: >> >>> Some GWL sub-commands such as `graph' do not accept a log event configuration. >>> This results in returning `#f' from `(%config 'log-events)' >> >> I don’t understand how this leads to a problem. Do any of the features >> of “graph” use “log-event”? > > Yes. `load-workflow' -> `load-workflow*' -> `load*' -> `log-event 'info > "Loading workflow file"'. I see. Does it make sense to support the --log-events option on “graph”? Or should we just work around it in the log-event procedure? >> Generally, I’d prefer to give “log-events” a better default. I can’t >> imagine a situation where #F (rather than the empty list) would be >> appropriate. > > The empty list would be a more sane choice I agree. I'll send a v2 with > this change. Also, the `print?` predicate could be memoize with `mlambda'? Yes, “print?” could me memoized. -- Ricardo
On Mon, 06 Jun 2022, Ricardo Wurmus <rekado@elephly.net> wrote:
> Olivier Dion <olivier.dion@polymtl.ca> writes:
>
>> On Sat, 04 Jun 2022, Ricardo Wurmus <rekado@elephly.net> wrote:
>>> Olivier Dion <olivier.dion@polymtl.ca> writes:
>>>
>>>> Some GWL sub-commands such as `graph' do not accept a log event configuration.
>>>> This results in returning `#f' from `(%config 'log-events)'
>>>
>>> I don’t understand how this leads to a problem. Do any of the features
>>> of “graph” use “log-event”?
>>
>> Yes. `load-workflow' -> `load-workflow*' -> `load*' -> `log-event 'info
>> "Loading workflow file"'.
>
> I see.
>
> Does it make sense to support the --log-events option on “graph”? Or
> should we just work around it in the log-event procedure?
I think that in all cases, the default of `(%config 'log-events)' should
be '() like you proposed. In that way, if new commands are added in the
future, this undesired behavior won't re-surface.
As for adding the `--log-events` option to the `graph' command. The
thing is that the output of the graph is printed on stdout. Adding
log-event would add an extra step in order to separate the logs from the
desired output. Thus, I think that if we do that, we also need to add a
`--output=DOT' option for redirecting the graph's output away from the
terminal.
WDY?
--
Olivier Dion
oldiob.dev
On Mon, 06 Jun 2022, Olivier Dion <olivier.dion@polymtl.ca> wrote:
>> Does it make sense to support the --log-events option on “graph”? Or
>> should we just work around it in the log-event procedure?
>
> I think that in all cases, the default of `(%config 'log-events)' should
> be '() like you proposed. In that way, if new commands are added in the
> future, this undesired behavior won't re-surface.
The only clean way I see of doing this is to set the switch as global.
$ guix workflow --log-event=all run|graph|web
--
Olivier Dion
oldiob.dev
Some GWL sub-commands might not accept the log-events switch. This results in returning `#f' from `(%config 'log-events)' Fix this by checking that 'log-events was configured. Also memoize the result. --- gwl/ui.scm | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/gwl/ui.scm b/gwl/ui.scm index 35bd127..bfe20a9 100644 --- a/gwl/ui.scm +++ b/gwl/ui.scm @@ -17,6 +17,8 @@ #:use-module (gwl config) #:use-module (gwl errors) #:use-module (guix colors) + #:use-module ((guix memoization) + #:select (mlambdaq)) #:use-module (srfi srfi-26) #:use-module (ice-9 format) #:export (G_ @@ -63,11 +65,16 @@ (format (current-error-port) "~a" (prefix-color prefix)))) +(define print? + (mlambdaq (type) + (let ((log-events (%config 'log-events))) + (and log-events + (or (member 'all log-events) + (member type log-events)) + #t)))) + (define (log-event type . message) - (define print? - (or (member 'all (%config 'log-events)) - (member type (%config 'log-events)))) - (when print? + (when (print? type) (case type ((error) (print-diagnostic-prefix (G_ "error: ") #:colors %error-color)) -- 2.36.1
The alternative would be to define the switch at the top configuration. Using `(wanted '((keywords . (log-events))))' for sub-configurations. However, there's seem to be a problem in guile-config for correctly parsing global keywords and sub-commands at the same time. --- gwl/config.scm.in | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/gwl/config.scm.in b/gwl/config.scm.in index 9ab58e5..728eec5 100644 --- a/gwl/config.scm.in +++ b/gwl/config.scm.in @@ -42,6 +42,20 @@ (define %gwl-git-browse-url "https://git.sv.gnu.org/cgit/gwl.git") +(define log-event-switch + (switch + (name 'log-events) + (character #\l) + (synopsis "Events to log") + (default '(execute error info)) + (example "error,info,execute,cache,debug,guix") + (test list?) + (handler + (lambda (value) + (when (string? value) + (map string->symbol + (string-split value #\,))))))) + (define config (configuration (name '#{guix workflow}#) @@ -71,6 +85,7 @@ workflow.") (example "/path/to/my-workflow.w")))) (keywords (list + log-event-switch (switch (name 'verbosity) (character #\v) @@ -78,18 +93,6 @@ workflow.") (default 2) (test integer?) (handler string->number)) - (switch - (name 'log-events) - (character #\l) - (synopsis "Events to log") - (default '(execute error info)) - (example "error,info,execute,cache,debug,guix") - (test list?) - (handler - (lambda (value) - (when (string? value) - (map string->symbol - (string-split value #\,)))))) (switch (name 'input) (character #\i) @@ -143,6 +146,9 @@ workflow.") (description "This command generates a visualization (e.g. in Graphviz Dot format) of the specified workflow.") + (keywords + (list + log-event-switch)) (arguments (list (argument -- 2.36.1
--- gwl/config.scm.in | 9 ++++++++- gwl/main.scm | 13 ++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/gwl/config.scm.in b/gwl/config.scm.in index 728eec5..1e0b4d0 100644 --- a/gwl/config.scm.in +++ b/gwl/config.scm.in @@ -148,7 +148,14 @@ workflow.") format) of the specified workflow.") (keywords (list - log-event-switch)) + log-event-switch + (switch + (name 'output) + (character #\o) + (default #false) + (test string?) + (synopsis "Write dot graph to <file>.") + (example "<file>")))) (arguments (list (argument diff --git a/gwl/main.scm b/gwl/main.scm index 78494fd..7f0fd14 100644 --- a/gwl/main.scm +++ b/gwl/main.scm @@ -17,6 +17,7 @@ (define-module (gwl main) #:use-module (config) #:use-module (config api) + #:use-module ((guix build utils) #:select (mkdir-p)) #:use-module (gwl errors) #:use-module (gwl process-engines) #:use-module (gwl web-interface) @@ -95,5 +96,15 @@ (parameterize ((*current-filename* file-name)) (match (load-workflow file-name) ((? workflow? wf) - (format #t "~a\n" (workflow->dot wf))) + + (define (flush) + (format #t "~a\n" (workflow->dot wf))) + + (define output (%config 'output)) + + (if output + (begin + (mkdir-p (dirname output)) + (with-output-to-file output flush)) + (flush))) (_ (leave (G_ "Failed to process the workflow.~%"))))))))))) -- 2.36.1
Olivier Dion <olivier.dion@polymtl.ca> writes:
> The alternative would be to define the switch at the top configuration. Using
> `(wanted '((keywords . (log-events))))' for sub-configurations. However,
> there's seem to be a problem in guile-config for correctly parsing global
> keywords and sub-commands at the same time.
Hmm, this patch doesn’t look pretty. What is that problem in
guile-config? Could you please share some details?
--
Ricardo
Hi Olivier,
> diff --git a/gwl/main.scm b/gwl/main.scm
> index 78494fd..7f0fd14 100644
> --- a/gwl/main.scm
> +++ b/gwl/main.scm
> @@ -17,6 +17,7 @@
> (define-module (gwl main)
> #:use-module (config)
> #:use-module (config api)
> + #:use-module ((guix build utils) #:select (mkdir-p))
> #:use-module (gwl errors)
> #:use-module (gwl process-engines)
> #:use-module (gwl web-interface)
> @@ -95,5 +96,15 @@
> (parameterize ((*current-filename* file-name))
> (match (load-workflow file-name)
> ((? workflow? wf)
> - (format #t "~a\n" (workflow->dot wf)))
> +
> + (define (flush)
> + (format #t "~a\n" (workflow->dot wf)))
> +
> + (define output (%config 'output))
> +
> + (if output
> + (begin
> + (mkdir-p (dirname output))
> + (with-output-to-file output flush))
> + (flush)))
> (_ (leave (G_ "Failed to process the workflow.~%")))))))))))
The name “flush” really confused me, because it already has a meaning
with regard to buffering.
I’d prefer to rewrite this
(format #t "~a\n" (workflow->dot wf))
to
(format port "~a\n" (workflow->dot wf))
and then arrange for port to either be (current-output-port) or some
file port.
--
Ricardo
Olivier Dion <olivier.dion@polymtl.ca> writes: > Some GWL sub-commands might not accept the log-events switch. > This results in returning `#f' from `(%config 'log-events)' > > Fix this by checking that 'log-events was configured. Also memoize the result. […] > +(define print? > + (mlambdaq (type) > + (let ((log-events (%config 'log-events))) > + (and log-events > + (or (member 'all log-events) > + (member type log-events)) > + #t)))) > + You don’t need the #t at the end of the “and”; the previous two values are already “truthy” or “falsy”. You could also do this: (and=> (%config 'log-events) (lambda (log-events) (or (member 'all log-events) (member type log-events)))) -- Ricardo