Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Sjoerd van Leent]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

Hi,

I have been having a bit of a go at understanding boot-9.scm. While 
investigating it, i found out an interesting bit where SCM code is 
compiled to new .GO code when performing fresh-compilation. I found 
similar code in the C sources in load.c.

Now I wanted to add an option to disable the display of the compilation 
messages so went ahead with creating such an option. However, before I 
continue, I want a discussion about two things:

- Shouldn't it be that either the C code or the SCM is responsible for 
displaying the messages? If so, I would assume the C code should be 
changed to have formal procedures to handle these things.

- Also, is this option in general wanted? If this is the case, I would 
also like to clean up the above mess of having two procedures printing 
exactly the same kind of information, leading to maintenance issues.

I attached a simple diff file, so everyone could have their opinions shared.

[-- Attachment #2: discussion.patch --]
[-- Type: text/x-patch, Size: 5535 bytes --]

diff --git a/libguile/load.c b/libguile/load.c
index 5019201..48f7443 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -228,6 +228,9 @@ static SCM *scm_loc_fresh_auto_compile;
 /* The fallback path for auto-compilation */
 static SCM *scm_loc_compile_fallback_path;
 
+/* Whether or not to display messages for auto-compilation */
+static SCM *scm_loc_display_auto_compilation_messages;
+
 /* Ellipsis: "..." */
 static SCM scm_ellipsis;
 
@@ -763,9 +766,12 @@ do_try_auto_compile (void *data)
   SCM source = SCM_PACK_POINTER (data);
   SCM comp_mod, compile_file;
 
-  scm_puts_unlocked (";;; compiling ", scm_current_error_port ());
-  scm_display (source, scm_current_error_port ());
-  scm_newline (scm_current_error_port ());
+  if (scm_is_true (*scm_loc_display_auto_compilation_messages))
+    {
+      scm_puts_unlocked (";;; compiling ", scm_current_error_port ());
+      scm_display (source, scm_current_error_port ());
+      scm_newline (scm_current_error_port ());
+    }
 
   comp_mod = scm_c_resolve_module ("system base compile");
   compile_file = scm_module_variable (comp_mod, sym_compile_file);
@@ -792,9 +798,12 @@ do_try_auto_compile (void *data)
       /* Assume `*current-warning-prefix*' has an appropriate value.  */
       res = scm_call_n (scm_variable_ref (compile_file), args, 5);
 
-      scm_puts_unlocked (";;; compiled ", scm_current_error_port ());
-      scm_display (res, scm_current_error_port ());
-      scm_newline (scm_current_error_port ());
+      if (scm_is_true (*scm_loc_display_auto_compilation_messages))
+	{       
+	  scm_puts_unlocked (";;; compiled ", scm_current_error_port ());
+	  scm_display (res, scm_current_error_port ());
+	  scm_newline (scm_current_error_port ());
+	}
       return res;
     }
   else
@@ -840,6 +849,13 @@ SCM_DEFINE (scm_sys_warn_auto_compilation_enabled, "%warn-auto-compilation-enabl
 #define FUNC_NAME s_scm_sys_warn_auto_compilation_enabled
 {
   static int message_shown = 0;
+  
+  if (scm_is_false (*scm_loc_display_auto_compilation_messages))
+    {
+      /* If auto-compilation messages are not to be shown, this should be set
+         to disable further display of this message */
+      message_shown = 1;
+    }
 
   if (!message_shown)
     {
@@ -1136,6 +1152,8 @@ scm_init_load ()
     = SCM_VARIABLE_LOC (scm_c_define ("%load-should-auto-compile", SCM_BOOL_F));
   scm_loc_fresh_auto_compile
     = SCM_VARIABLE_LOC (scm_c_define ("%fresh-auto-compile", SCM_BOOL_F));
+  scm_loc_display_auto_compilation_messages
+    = SCM_VARIABLE_LOC (scm_c_define ("%display-auto-compilation-messages", SCM_BOOL_T));
 
   scm_ellipsis = scm_from_latin1_string ("...");
 
diff --git a/module/ice-9/boot-9.scm b/module/ice-9/boot-9.scm
index cac058c..5789db0 100644
--- a/module/ice-9/boot-9.scm
+++ b/module/ice-9/boot-9.scm
@@ -4091,18 +4091,27 @@ when none is available, reading FILE-NAME with READER."
        (if (and gostat (more-recent? gostat scmstat))
            go-file-name
            (begin
-             (if gostat
-                 (format (current-warning-port)
-                         ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
-                         name go-file-name))
-             (cond
-              (%load-should-auto-compile
-               (%warn-auto-compilation-enabled)
-               (format (current-warning-port) ";;; compiling ~a\n" name)
-               (let ((cfn (compile name)))
-                 (format (current-warning-port) ";;; compiled ~a\n" cfn)
-                 cfn))
-              (else #f)))))
+	     (let ((newer-message-note "note: source file")
+		   (newer-than-compiled-message "newer than compiled"))
+	       (if gostat
+		   (if %display-auto-compilation-messages
+		       (format (current-warning-port)
+			     ";;;~a ~a\n;;;       ~a ~a\n" 
+			     newer-message-note
+			     name
+			     newer-than-compiled-message
+			     go-file-name)))
+	       (cond
+		(%load-should-auto-compile
+		 (%warn-auto-compilation-enabled)
+		 (if %display-auto-compilation-messages
+		     (begin
+		       (format (current-warning-port) ";;; compiling ~a\n" name)))
+		 (let ((cfn (compile name)))
+		   (if %display-auto-compilation-messages
+		       (format (current-warning-port) ";;; compiled ~a\n" cfn))
+		   cfn))
+		(else #f))))))
      #:warning "WARNING: compilation of ~a failed:\n" name))
 
   (define (sans-extension file)
diff --git a/module/ice-9/command-line.scm b/module/ice-9/command-line.scm
index b387eb3..5171ee5 100644
--- a/module/ice-9/command-line.scm
+++ b/module/ice-9/command-line.scm
@@ -133,6 +133,8 @@ If FILE begins with `-' the -s switch is mandatory.
   --no-auto-compile  disable automatic source file compilation;
                  default is to enable auto-compilation of source
                  files.
+  --no-auto-compilation-messages disable display of auto-compilation 
+                 messages.
   --listen[=P]   listen on a local port or a path for REPL clients;
                  if P is not given, the default is local port 37146
   -q             inhibit loading of user init file
@@ -343,6 +345,10 @@ If FILE begins with `-' the -s switch is mandatory.
             (set! %load-should-auto-compile #t)
             (parse args out))
 
+           ((string=? arg "--no-auto-compilation-messages")
+	    (set! %display-auto-compilation-messages #f)
+	    (parse args out))
+
            ((string=? arg "--fresh-auto-compile")
             (set! %load-should-auto-compile #t)
             (set! %fresh-auto-compile #t)

Re: Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Ludovic Courtès]

Hi,

Sjoerd van Leent <svanleent@gmail.com> skribis:

> -  scm_puts_unlocked (";;; compiling ", scm_current_error_port ());
> -  scm_display (source, scm_current_error_port ());
> -  scm_newline (scm_current_error_port ());
> +  if (scm_is_true (*scm_loc_display_auto_compilation_messages))
> +    {
> +      scm_puts_unlocked (";;; compiling ", scm_current_error_port ());
> +      scm_display (source, scm_current_error_port ());
> +      scm_newline (scm_current_error_port ());
> +    }

FWIW, I think the approach should rather be to have a special port (a
fluid) for such things, say, ‘current-notification-port’.  We’d simply
replace scm_current_error_port by scm_current_notication_port above.

The command-line option could be --quiet.  It would bind both
current-notification-port and current-warning-port to a void port.

WDYT?  Would you like to adjust the patch accordingly?

Thanks,
Ludo’.

Re: Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Sjoerd van Leent]

I see where you are going and personally, I like the suggestion.

In any case, couldn't there be an approach similar to those used with 
most logging frameworks. In GLib, there are the following options:

G_LOG_LEVEL_ERROR
G_LOG_LEVEL_CRITICAL
G_LOG_LEVEL_WARNING
G_LOG_LEVEL_MESSAGE
G_LOG_LEVEL_INFO
G_LOG_LEVEL_DEBUG

I have no clue yet which ports exist, aside from (current-output-port), 
(current-warning-port) and (current-error-port).

I imagine to create 5 virtual ports: (current-debug-port), 
(current-info-port), (current-message-port), (current-warning-port) and 
(current-critical-port). These would either be hooked up to the 
(current-output-port), except for (current-critical-port) which should 
be hooked up to (current-error-port).

The normal guile operation would be to have warning, critical and error 
messages to be fired. Guile should then get two additional options:

--verbosity=<level>, where level should be "debug", "info", "message", 
"warning", "critical" and "error".
--quiet, which suggests to skipp verbosity altogether and to display 
nothing anymore aside from writes to (current-output-port).

Guile should by default function in the "info" verbosity mode.

If verbosity is a nice fluid, one could change the verbosity level when 
deemed necessary.

And I would be happy to work this out.

On 03/02/14 22:13, Ludovic Courtès wrote:
> Hi,
>
> Sjoerd van Leent <svanleent@gmail.com> skribis:
>
>> -  scm_puts_unlocked (";;; compiling ", scm_current_error_port ());
>> -  scm_display (source, scm_current_error_port ());
>> -  scm_newline (scm_current_error_port ());
>> +  if (scm_is_true (*scm_loc_display_auto_compilation_messages))
>> +    {
>> +      scm_puts_unlocked (";;; compiling ", scm_current_error_port ());
>> +      scm_display (source, scm_current_error_port ());
>> +      scm_newline (scm_current_error_port ());
>> +    }
> FWIW, I think the approach should rather be to have a special port (a
> fluid) for such things, say, ‘current-notification-port’.  We’d simply
> replace scm_current_error_port by scm_current_notication_port above.
>
> The command-line option could be --quiet.  It would bind both
> current-notification-port and current-warning-port to a void port.
>
> WDYT?  Would you like to adjust the patch accordingly?
>
> Thanks,
> Ludo’.
>
>

Re: Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Andy Wingo]

On Sun 02 Mar 2014 22:13, ludo@gnu.org (Ludovic Courtès) writes:

> FWIW, I think the approach should rather be to have a special port (a
> fluid) for such things, say, ‘current-notification-port’.  We’d simply
> replace scm_current_error_port by scm_current_notication_port above.

Isn't that the same as current-warning-port?  I thought this was one of
the use cases for current-warning-port :)

Andy
-- 
http://wingolog.org/

Re: Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Ludovic Courtès]

Andy Wingo <wingo@pobox.com> skribis:

> On Sun 02 Mar 2014 22:13, ludo@gnu.org (Ludovic Courtès) writes:
>
>> FWIW, I think the approach should rather be to have a special port (a
>> fluid) for such things, say, ‘current-notification-port’.  We’d simply
>> replace scm_current_error_port by scm_current_notication_port above.
>
> Isn't that the same as current-warning-port?  I thought this was one of
> the use cases for current-warning-port :)

Yeah it’s very similar, but we could want to differentiate between
actual warnings and mere notifications.  Dunno if it’s important.

Ludo’.

Re: Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:

> Andy Wingo <wingo@pobox.com> skribis:
>
>> On Sun 02 Mar 2014 22:13, ludo@gnu.org (Ludovic Courtès) writes:
>>
>>> FWIW, I think the approach should rather be to have a special port (a
>>> fluid) for such things, say, ‘current-notification-port’.  We’d simply
>>> replace scm_current_error_port by scm_current_notication_port above.
>>
>> Isn't that the same as current-warning-port?  I thought this was one of
>> the use cases for current-warning-port :)
>
> Yeah it’s very similar, but we could want to differentiate between
> actual warnings and mere notifications.  Dunno if it’s important.

One issue worth thinking about is that there are cases where code has to
parameterize all of these port parameters.

For example, I recently discovered that REPL Servers weren't redirecting
the warning port to the socket, so warnings about expressions typed on a
REPL server were being sent to the main program's stderr.  I fixed this
in 5e74217c7cf07ad474cdce1a01e049492e7ef1b7, but if we add another port
parameter I'll have to fix that in at least two places now: server.scm
and coop-server.scm.

I wonder if external code needs to sometimes do this as well.  How will
they cope with a periodically-expanding set of port parameters, while
retaining compatible with older versions of guile that lack some of
them?  Should we add a 'parameterize-all-stdout-like-ports' macro?

     Mark

Re: Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Mark H Weaver]

Mark H Weaver <mhw@netris.org> writes:

> For example, I recently discovered that REPL Servers weren't redirecting
> the warning port to the socket, so warnings about expressions typed on a
> REPL server were being sent to the main program's stderr.  I fixed this
> in 5e74217c7cf07ad474cdce1a01e049492e7ef1b7, but if we add another port
> parameter I'll have to fix that in at least two places now: server.scm
> and coop-server.scm.
>
> I wonder if external code needs to sometimes do this as well.  How will
> they cope with a periodically-expanding set of port parameters, while
> retaining compatible with older versions of guile that lack some of
> them?  Should we add a 'parameterize-all-stdout-like-ports' macro?

It occurs to me that one possibility would be to allow some of these
parameters to effectively mirror the value of some other parameter.

For example, the 'current-notification-port' parameter could mirror the
'current-warning-port' parameter by default, which could in turn mirror
the 'current-error-port' parameter by default.

One way to implement this would be to make the corresponding fluid be a
thunk that returns a port, instead of the port itself.  The converter
could accept either a port or a parameter, and convert each of these to
a suitable thunk.  Finally, we'd make custom 'current-warning-port' and
'current-error-port' procedures that would handling calling the thunk.

Of course, this extra complexity would make these parameters inherently
more expensive, so it would only be appropriate for these special
seldom-used port parameters.  However, for these cases it might be a
worthwhile extension.  In one commit we could fix a number of possible
bugs (similar to the one fixed by 5e74217) that might well exist in
external code.

What do you think?

      Mark

Re: Discussion for %display-auto-compilation-messages (and --no-auto-compilation-messages option)

[Andy Wingo]

On Wed 26 Mar 2014 04:19, Mark H Weaver <mhw@netris.org> writes:

> Mark H Weaver <mhw@netris.org> writes:
>
> It occurs to me that one possibility would be to allow some of these
> parameters to effectively mirror the value of some other parameter.

> One way to implement this would be to make the corresponding fluid be a
> thunk that returns a port, instead of the port itself.  The converter
> could accept either a port or a parameter, and convert each of these to
> a suitable thunk.  Finally, we'd make custom 'current-warning-port' and
> 'current-error-port' procedures that would handling calling the thunk.

This would mean that you couldn't use current-warning-port as a
parameter and a procedure.

I think this complexity argument is sufficient to argue against having
more current-foo-ports :)

Anyd
-- 
http://wingolog.org/