[PATCH] gwl/ui: Check for log-events configuration

[PATCH] gwl/ui: Check for log-events configuration

[Olivier Dion]

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

Re: [PATCH] gwl/ui: Check for log-events configuration

[Ricardo Wurmus]

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

Re: [PATCH] gwl/ui: Check for log-events configuration

[Olivier Dion via]

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

Re: [PATCH] gwl/ui: Check for log-events configuration

[Ricardo Wurmus]

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

Re: [PATCH] gwl/ui: Check for log-events configuration

[Olivier Dion via]

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

Re: [PATCH] gwl/ui: Check for log-events configuration

[Olivier Dion via]

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

[PATCH 1/3] gwl/ui: Check for log-events configuration

[Olivier Dion]

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

[PATCH 2/3] gwl/config: Share log-event switch with subcommands

[Olivier Dion]

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

Re: [PATCH 2/3] gwl/config: Share log-event switch with subcommands

[Ricardo Wurmus]

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

[PATCH 3/3] graph: Add output switch

[Olivier Dion]

---
 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

Re: [PATCH 3/3] graph: Add output switch

[Ricardo Wurmus]

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

Re: [PATCH 1/3] gwl/ui: Check for log-events configuration

[Ricardo Wurmus]

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