unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
* add command line option to quiet compiler messages
@ 2016-07-13  4:08 Tobin Harding
  2016-07-14 10:01 ` Andy Wingo
  0 siblings, 1 reply; 6+ messages in thread
From: Tobin Harding @ 2016-07-13  4:08 UTC (permalink / raw)
  To: Andy Wingo; +Cc: guile-devel

Patch adds command line option '--quiet' to inhibit compile and load messages.

---

This is version 2. Submitting this patch on a new email thread.

While working on this I discovered that compile messages are output from two
separate places (load.c and boot-9.scm). Each file contains identical strings
for the messages. This goes against the rule of SPOT.

This patch adds scheme procedures for outputting the messages. However load.c
does not call these.

I do not know how to call custom scheme procedures from the C file.

If someone could please point me in the right direction I would like to add this
before the patch is applied.

thanks,
Tobin.

 libguile/load.c               | 33 ++++++++++++++++++++++-----------
 module/ice-9/boot-9.scm       | 24 +++++++++++++++++++-----
 module/ice-9/command-line.scm |  5 +++++
 3 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/libguile/load.c b/libguile/load.c
index 7ad9a75..6b13090 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -226,6 +226,9 @@ static SCM *scm_loc_fresh_auto_compile;
 /* The fallback path for auto-compilation */
 static SCM *scm_loc_compile_fallback_path;
 
+/* Whether we should load [and compile] quietly */
+static SCM *scm_loc_quiet;
+
 /* Ellipsis: "..." */
 static SCM scm_ellipsis;
 
@@ -561,11 +564,13 @@ compiled_is_fresh (SCM full_filename, SCM compiled_filename,
   else
     {
       compiled_is_newer = 0;
-      scm_puts (";;; note: source file ", scm_current_warning_port ());
-      scm_display (full_filename, scm_current_warning_port ());
-      scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
-      scm_display (compiled_filename, scm_current_warning_port ());
-      scm_puts ("\n", scm_current_warning_port ());
+      if (scm_is_false (*scm_loc_quiet)) {
+	scm_puts (";;; note: source file ", scm_current_warning_port ());
+	scm_display (full_filename, scm_current_warning_port ());
+	scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
+	scm_display (compiled_filename, scm_current_warning_port ());
+	scm_puts ("\n", scm_current_warning_port ());
+      }
     }
 
   return compiled_is_newer;
@@ -1007,9 +1012,11 @@ do_try_auto_compile (void *data)
   SCM source = SCM_PACK_POINTER (data);
   SCM comp_mod, compile_file;
 
-  scm_puts (";;; compiling ", scm_current_warning_port ());
-  scm_display (source, scm_current_warning_port ());
-  scm_newline (scm_current_warning_port ());
+  if (scm_is_false (*scm_loc_quiet)) {
+    scm_puts (";;; compiling ", scm_current_warning_port ());
+    scm_display (source, scm_current_warning_port ());
+    scm_newline (scm_current_warning_port ());
+  }
 
   comp_mod = scm_c_resolve_module ("system base compile");
   compile_file = scm_module_variable (comp_mod, sym_compile_file);
@@ -1036,9 +1043,11 @@ 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 (";;; compiled ", scm_current_warning_port ());
-      scm_display (res, scm_current_warning_port ());
-      scm_newline (scm_current_warning_port ());
+      if (scm_is_false (*scm_loc_quiet)) {
+	scm_puts (";;; compiled ", scm_current_warning_port ());
+	scm_display (res, scm_current_warning_port ());
+	scm_newline (scm_current_warning_port ());
+      }
       return res;
     }
   else
@@ -1349,6 +1358,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_quiet
+    = SCM_VARIABLE_LOC (scm_c_define ("%quiet", SCM_BOOL_F));
 
   scm_ellipsis = scm_from_latin1_string ("...");
 
diff --git a/module/ice-9/boot-9.scm b/module/ice-9/boot-9.scm
index 99543e7..98a9a4e 100644
--- a/module/ice-9/boot-9.scm
+++ b/module/ice-9/boot-9.scm
@@ -32,6 +32,22 @@
 
 \f
 
+;;; Don't know where to put these:
+(define (cond_warn_compiling file)
+  (unless %quiet
+    (format (current-warning-port) ";;; compiling ~a\n" file)))
+
+(define (cond_warn_compiled file)
+  (unless %quiet
+    (format (current-warning-port) ";;; compiled ~a\n" file)))
+
+(define (cond_warn_newer new old)
+  (unless %quiet
+    (format (current-warning-port)
+            ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
+            name go-file-name)))
+
+
 ;; Before compiling, make sure any symbols are resolved in the (guile)
 ;; module, the primary location of those symbols, rather than in
 ;; (guile-user), the default module that we compile in.
@@ -3769,15 +3785,13 @@ when none is available, reading FILE-NAME with READER."
            (load-thunk-from-file go-file-name)
            (begin
              (when gostat
-               (format (current-warning-port)
-                       ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
-                       name go-file-name))
+               (cond_warn_newer name go-file-name)
              (cond
               (%load-should-auto-compile
                (%warn-auto-compilation-enabled)
-               (format (current-warning-port) ";;; compiling ~a\n" name)
+               (cond_warn_compiling name)
                (let ((cfn (compile name)))
-                 (format (current-warning-port) ";;; compiled ~a\n" cfn)
+                 (cond_warn_compiled cfn)
                  (load-thunk-from-file cfn)))
               (else #f)))))
      #:warning "WARNING: compilation of ~a failed:\n" name))
diff --git a/module/ice-9/command-line.scm b/module/ice-9/command-line.scm
index 98d3855..9a3f7e1 100644
--- a/module/ice-9/command-line.scm
+++ b/module/ice-9/command-line.scm
@@ -136,6 +136,7 @@ If FILE begins with `-' the -s switch is mandatory.
   --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
+  --quiet        inhibit compile and load messages
   --use-srfi=LS  load SRFI modules for the SRFIs in LS,
                  which is a list of numbers like \"2,13,14\"
   -h, --help     display this help and exit
@@ -358,6 +359,10 @@ If FILE begins with `-' the -s switch is mandatory.
             (set! inhibit-user-init? #t)
             (parse args out))
 
+           ((string=? arg "--quiet")
+            (set! %quiet #t)
+            (parse args out))
+
            ((string-prefix? "--use-srfi=" arg)
             (let ((srfis (map (lambda (x)
                                 (let ((n (string->number x)))
-- 
2.8.3





^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: add command line option to quiet compiler messages
  2016-07-13  4:08 add command line option to quiet compiler messages Tobin Harding
@ 2016-07-14 10:01 ` Andy Wingo
  2016-07-20  6:41   ` Tobin Harding
  2016-07-20  7:05   ` Tobin Harding
  0 siblings, 2 replies; 6+ messages in thread
From: Andy Wingo @ 2016-07-14 10:01 UTC (permalink / raw)
  To: Tobin Harding; +Cc: guile-devel

Hi :)

Thanks for the patch!

On Wed 13 Jul 2016 06:08, Tobin Harding <me@tobin.cc> writes:

> While working on this I discovered that compile messages are output from two
> separate places (load.c and boot-9.scm). Each file contains identical strings
> for the messages. This goes against the rule of SPOT.

Sadly it has to be like this for now.  I do not have suggestions to fix
it; it's gnarly.

> -      scm_puts (";;; note: source file ", scm_current_warning_port ());
> -      scm_display (full_filename, scm_current_warning_port ());
> -      scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
> -      scm_display (compiled_filename, scm_current_warning_port ());
> -      scm_puts ("\n", scm_current_warning_port ());
> +      if (scm_is_false (*scm_loc_quiet)) {
> +	scm_puts (";;; note: source file ", scm_current_warning_port ());
> +	scm_display (full_filename, scm_current_warning_port ());
> +	scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
> +	scm_display (compiled_filename, scm_current_warning_port ());
> +	scm_puts ("\n", scm_current_warning_port ());
> +      }
>      }

Please resend without tabs, please.  Thanks :)

>  \f
>  
> +;;; Don't know where to put these:
> +(define (cond_warn_compiling file)
> +  (unless %quiet
> +    (format (current-warning-port) ";;; compiling ~a\n" file)))
> +
> +(define (cond_warn_compiled file)
> +  (unless %quiet
> +    (format (current-warning-port) ";;; compiled ~a\n" file)))
> +
> +(define (cond_warn_newer new old)
> +  (unless %quiet
> +    (format (current-warning-port)
> +            ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
> +            name go-file-name)))

These definitions are inappropriate for being at the top-level of
boot-9.scm -- they would become automatically a part of all
environments.  I would leave out this change.

Note also that the Guile convention is hyphen-case not snake_case.

> diff --git a/module/ice-9/command-line.scm b/module/ice-9/command-line.scm
> index 98d3855..9a3f7e1 100644
> --- a/module/ice-9/command-line.scm
> +++ b/module/ice-9/command-line.scm
> @@ -136,6 +136,7 @@ If FILE begins with `-' the -s switch is mandatory.
>    --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
> +  --quiet        inhibit compile and load messages
>    --use-srfi=LS  load SRFI modules for the SRFIs in LS,
>                   which is a list of numbers like \"2,13,14\"
>    -h, --help     display this help and exit

What do people think about the name of this command line option?

To me "inhibit compile and load messages" does not quite capture what it
does.  But maybe it is good enough.

Incidentally this change will need a documentation update as well.  I
often find that adding documentation makes it more clear how things
should be named -- that the process of explaining things makes it
apparent what things should be changed :)

> +           ((string=? arg "--quiet")
> +            (set! %quiet #t)
> +            (parse args out))
> +

Probably %quiet should be a parameter, and unless we plan on using it
for other purposes it should have a more specific name
(%verbose-auto-compilation-messages or so; no idea).

Or, you could always just (current-warning-port (%make-void-port "w"));
then the patch is super simple, no conditions needed.  That is what I
was originally thinking, but it has the disadvantage that you lose other
uses of the current warning port, but maybe that's an advantage too.

WDYT?

Andy



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: add command line option to quiet compiler messages
  2016-07-14 10:01 ` Andy Wingo
@ 2016-07-20  6:41   ` Tobin Harding
  2016-07-23 10:29     ` Andy Wingo
  2016-07-20  7:05   ` Tobin Harding
  1 sibling, 1 reply; 6+ messages in thread
From: Tobin Harding @ 2016-07-20  6:41 UTC (permalink / raw)
  To: Andy Wingo; +Cc: guile-devel

On Thu, Jul 14, 2016 at 12:01:10PM +0200, Andy Wingo wrote:
> On Wed 13 Jul 2016 06:08, Tobin Harding <me@tobin.cc> writes:
> 
> > While working on this I discovered that compile messages are output from two
> > separate places (load.c and boot-9.scm). Each file contains identical strings
> > for the messages. This goes against the rule of SPOT.
> 
> Sadly it has to be like this for now.  I do not have suggestions to fix
> it; it's gnarly.

Ok, Will forget about this. I would love to know why though, solely out of interest.

> 
> > -      scm_puts (";;; note: source file ", scm_current_warning_port ());
> > -      scm_display (full_filename, scm_current_warning_port ());
> > -      scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
> > -      scm_display (compiled_filename, scm_current_warning_port ());
> > -      scm_puts ("\n", scm_current_warning_port ());
> > +      if (scm_is_false (*scm_loc_quiet)) {
> > +	scm_puts (";;; note: source file ", scm_current_warning_port ());
> > +	scm_display (full_filename, scm_current_warning_port ());
> > +	scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
> > +	scm_display (compiled_filename, scm_current_warning_port ());
> > +	scm_puts ("\n", scm_current_warning_port ());
> > +      }
> >      }
> 
> Please resend without tabs, please.  Thanks :)

I'm not sure about this one. I manually put spaces in front of each line as
directed but it does not show up as a change in 'git diff'. Is the rest of the
patch correct in regards to Guile project tab/space guidelines. (I couldn't find
any such guideline, is there some specific Emacs setup I should be using for
Guile development?)

> 
> >  \f
> >  
> > +;;; Don't know where to put these:
> > +(define (cond_warn_compiling file)
> > +  (unless %quiet
> > +    (format (current-warning-port) ";;; compiling ~a\n" file)))
> > +
> > +(define (cond_warn_compiled file)
> > +  (unless %quiet
> > +    (format (current-warning-port) ";;; compiled ~a\n" file)))
> > +
> > +(define (cond_warn_newer new old)
> > +  (unless %quiet
> > +    (format (current-warning-port)
> > +            ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
> > +            name go-file-name)))
> 
> These definitions are inappropriate for being at the top-level of
> boot-9.scm -- they would become automatically a part of all
> environments.  I would leave out this change.

Fixed. I have put these definitions at the top of the procedure that calls them,
does that fit in with the rest of the code ok?

> Note also that the Guile convention is hyphen-case not snake_case.

My bad, amateur mistake.

> 
> > diff --git a/module/ice-9/command-line.scm b/module/ice-9/command-line.scm
> > index 98d3855..9a3f7e1 100644
> > --- a/module/ice-9/command-line.scm
> > +++ b/module/ice-9/command-line.scm
> > @@ -136,6 +136,7 @@ If FILE begins with `-' the -s switch is mandatory.
> >    --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
> > +  --quiet        inhibit compile and load messages
> >    --use-srfi=LS  load SRFI modules for the SRFIs in LS,
> >                   which is a list of numbers like \"2,13,14\"
> >    -h, --help     display this help and exit
> 
> What do people think about the name of this command line option?
> 
> To me "inhibit compile and load messages" does not quite capture what it
> does.  But maybe it is good enough.

I've changed it to;

  --quiet        inhibit informational compiler output, e.g name of
                 files being compiled

Open to suggestions still. Unlike most programmers I find documenting my work harder
than writing the code :)

> Incidentally this change will need a documentation update as well.  I
> often find that adding documentation makes it more clear how things
> should be named -- that the process of explaining things makes it
> apparent what things should be changed :)
> 
> > +           ((string=? arg "--quiet")
> > +            (set! %quiet #t)
> > +            (parse args out))
> > +
> 
> Probably %quiet should be a parameter, and unless we plan on using it
> for other purposes it should have a more specific name
> (%verbose-auto-compilation-messages or so; no idea).

Could you clarify please, a parameter to what?

> 
> Or, you could always just (current-warning-port (%make-void-port "w"));
> then the patch is super simple, no conditions needed.  That is what I
> was originally thinking, but it has the disadvantage that you lose other
> uses of the current warning port, but maybe that's an advantage too.

Won't we loose compiler warnings then? I did not want to inhibit those.

thanks,
Tobin.

reworked patch follows :)

---
 libguile/load.c               | 34 ++++++++++++++++++++++------------
 module/ice-9/boot-9.scm       | 22 +++++++++++++++++-----
 module/ice-9/command-line.scm |  6 ++++++
 3 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/libguile/load.c b/libguile/load.c
index 7ad9a75..e2d6c4a 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -226,6 +226,9 @@ static SCM *scm_loc_fresh_auto_compile;
 /* The fallback path for auto-compilation */
 static SCM *scm_loc_compile_fallback_path;
 
+/* Whether we should output compiler informational messages */
+static SCM *scm_loc_verbose_compiler_messages;
+
 /* Ellipsis: "..." */
 static SCM scm_ellipsis;
 
@@ -561,11 +564,13 @@ compiled_is_fresh (SCM full_filename, SCM compiled_filename,
   else
     {
       compiled_is_newer = 0;
-      scm_puts (";;; note: source file ", scm_current_warning_port ());
-      scm_display (full_filename, scm_current_warning_port ());
-      scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
-      scm_display (compiled_filename, scm_current_warning_port ());
-      scm_puts ("\n", scm_current_warning_port ());
+      if (scm_is_true (*scm_loc_verbose_compiler_messages)) {
+        scm_puts (";;; note: source file ", scm_current_warning_port ());
+        scm_display (full_filename, scm_current_warning_port ());
+        scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
+        scm_display (compiled_filename, scm_current_warning_port ());
+        scm_puts ("\n", scm_current_warning_port ());
+      }
     }
 
   return compiled_is_newer;
@@ -1006,10 +1011,11 @@ do_try_auto_compile (void *data)
 {
   SCM source = SCM_PACK_POINTER (data);
   SCM comp_mod, compile_file;
-
-  scm_puts (";;; compiling ", scm_current_warning_port ());
-  scm_display (source, scm_current_warning_port ());
-  scm_newline (scm_current_warning_port ());
+  if (scm_is_true (*scm_loc_verbose_compiler_messages)) {
+    scm_puts (";;; compiling ", scm_current_warning_port ());
+    scm_display (source, scm_current_warning_port ());
+    scm_newline (scm_current_warning_port ());
+  }
 
   comp_mod = scm_c_resolve_module ("system base compile");
   compile_file = scm_module_variable (comp_mod, sym_compile_file);
@@ -1036,9 +1042,11 @@ 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 (";;; compiled ", scm_current_warning_port ());
-      scm_display (res, scm_current_warning_port ());
-      scm_newline (scm_current_warning_port ());
+      if (scm_is_true (*scm_loc_verbose_compiler_messages)) {
+	scm_puts (";;; compiled ", scm_current_warning_port ());
+	scm_display (res, scm_current_warning_port ());
+	scm_newline (scm_current_warning_port ());
+      }
       return res;
     }
   else
@@ -1349,6 +1357,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_verbose_compiler_messages
+    = SCM_VARIABLE_LOC (scm_c_define ("%verbose-compiler-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 99543e7..018ca6a 100644
--- a/module/ice-9/boot-9.scm
+++ b/module/ice-9/boot-9.scm
@@ -3762,6 +3762,20 @@ when none is available, reading FILE-NAME with READER."
     ;; Return GO-FILE-NAME after making sure that it contains a freshly
     ;; compiled version of source file NAME with stat SCMSTAT; return #f
     ;; on failure.
+    (define (cond-warn-compiling file)
+      (if %verbose-compiler-messages
+        (format (current-warning-port) ";;; compiling ~a\n" file)))
+
+    (define (cond-warn-compiled file)
+      (if %verbose-compiler-messages
+        (format (current-warning-port) ";;; compiled ~a\n" file)))
+
+    (define (cond-warn-newer new old)
+      (if %verbose-compiler-messages
+        (format (current-warning-port)
+                ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
+                name go-file-name)))
+
     (false-if-exception
      (let ((gostat (and (not %fresh-auto-compile)
                         (stat go-file-name #f))))
@@ -3769,15 +3783,13 @@ when none is available, reading FILE-NAME with READER."
            (load-thunk-from-file go-file-name)
            (begin
              (when gostat
-               (format (current-warning-port)
-                       ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
-                       name go-file-name))
+               (cond-warn-newer name go-file-name)
              (cond
               (%load-should-auto-compile
                (%warn-auto-compilation-enabled)
-               (format (current-warning-port) ";;; compiling ~a\n" name)
+               (cond-warn-compiling name)
                (let ((cfn (compile name)))
-                 (format (current-warning-port) ";;; compiled ~a\n" cfn)
+                 (cond-warn-compiled cfn)
                  (load-thunk-from-file cfn)))
               (else #f)))))
      #:warning "WARNING: compilation of ~a failed:\n" name))
diff --git a/module/ice-9/command-line.scm b/module/ice-9/command-line.scm
index 98d3855..fe639bb 100644
--- a/module/ice-9/command-line.scm
+++ b/module/ice-9/command-line.scm
@@ -136,6 +136,8 @@ If FILE begins with `-' the -s switch is mandatory.
   --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
+  --quiet        inhibit informational compiler output, e.g name of
+                 files being compiled
   --use-srfi=LS  load SRFI modules for the SRFIs in LS,
                  which is a list of numbers like \"2,13,14\"
   -h, --help     display this help and exit
@@ -358,6 +360,10 @@ If FILE begins with `-' the -s switch is mandatory.
             (set! inhibit-user-init? #t)
             (parse args out))
 
+           ((string=? arg "--quiet")
+            (set! %verbose-compiler-messages #f)
+            (parse args out))
+
            ((string-prefix? "--use-srfi=" arg)
             (let ((srfis (map (lambda (x)
                                 (let ((n (string->number x)))
-- 
2.8.3




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: add command line option to quiet compiler messages
  2016-07-14 10:01 ` Andy Wingo
  2016-07-20  6:41   ` Tobin Harding
@ 2016-07-20  7:05   ` Tobin Harding
  1 sibling, 0 replies; 6+ messages in thread
From: Tobin Harding @ 2016-07-20  7:05 UTC (permalink / raw)
  To: Andy Wingo; +Cc: guile-devel

On Thu, Jul 14, 2016 at 12:01:10PM +0200, Andy Wingo wrote:
> Incidentally this change will need a documentation update as well. 

I forgot the documentation in the last email.

Excuse my git noob'ness but git is telling me that doc/ref is excluded by
.gitignore, however the only two entries (referencing doc/) in .gitignore are

/doc/ref/standard-library.texi
/doc/ref/standard-libraryscmfiles

The docs came down with git clone so obviously they are known to git somehow?

I roughed up a document addition, is this the sort of thing you would like

 -- Scheme Variable: %verbose-compiler-messages
     This variable turns on/off informational messages from the
     compiler. Currently these messages output information about the
     source file and compiled file (fully qualified path names).

I thought to add it to the end of guile.info-4 (that's the section on
compiling). Are the variable index entries auto-generated or do I need to make
an addition there also?

Sorry to take up so much of you time with such a small patch :)

thanks,
Tobin.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: add command line option to quiet compiler messages
  2016-07-20  6:41   ` Tobin Harding
@ 2016-07-23 10:29     ` Andy Wingo
  2016-07-27  0:36       ` Tobin Harding
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Wingo @ 2016-07-23 10:29 UTC (permalink / raw)
  To: Tobin Harding; +Cc: guile-devel

Hi :)

On Wed 20 Jul 2016 08:41, Tobin Harding <me@tobin.cc> writes:

> On Thu, Jul 14, 2016 at 12:01:10PM +0200, Andy Wingo wrote:
>> On Wed 13 Jul 2016 06:08, Tobin Harding <me@tobin.cc> writes:
>> 
>> > While working on this I discovered that compile messages are output from two
>> > separate places (load.c and boot-9.scm). Each file contains identical strings
>> > for the messages. This goes against the rule of SPOT.
>> 
>> Sadly it has to be like this for now.  I do not have suggestions to fix
>> it; it's gnarly.
>
> Ok, Will forget about this. I would love to know why though, solely out of interest.

Guile needs the ability to detect when files are out of date very early
in C, before any other Scheme file is loaded (compiled or not).  We need
to do it from Scheme too, as you see in this code.  The natural thing
would be to define primitives to emit the warnings in C and export them
to Scheme (or vice versa) but that means that the primitive would then
be in the default environment for everyone -- and we really don't want
to add things to the default environment at this point, it creates
maintenance hazards.  So copying strings is the lesser evil in a way.

>> Please resend without tabs, please.  Thanks :)
>
> I'm not sure about this one. I manually put spaces in front of each line as
> directed but it does not show up as a change in 'git diff'. Is the rest of the
> patch correct in regards to Guile project tab/space guidelines. (I couldn't find
> any such guideline, is there some specific Emacs setup I should be using for
> Guile development?)

Hum, I see that our .dir-locals.el is currently:

  ((nil             . ((fill-column . 72)
                       (tab-width   .  8)))
   (c-mode          . ((c-file-style . "gnu")))
   (scheme-mode
    . ((indent-tabs-mode . nil)
       (eval . (put 'pass-if 'scheme-indent-function 1))
       (eval . (put 'pass-if-exception 'scheme-indent-function 2))
       (eval . (put 'pass-if-equal 'scheme-indent-function 2))
       (eval . (put 'with-test-prefix 'scheme-indent-function 1))
       (eval . (put 'with-code-coverage 'scheme-indent-function 1))
       (eval . (put 'with-statprof 'scheme-indent-function 1))))
   (emacs-lisp-mode . ((indent-tabs-mode . nil)))
   (texinfo-mode    . ((indent-tabs-mode . nil)
                       (fill-column . 72))))

But we are missing an (indent-tabs-mode . nil) in the c-mode part I
think.  That is the proper fix ;)  In my emacs I customized the default
indent-tabs-mode to nil globally; but that's a personal preference.

> I've changed it to;
>
>   --quiet        inhibit informational compiler output, e.g name of
>                  files being compiled
>
> Open to suggestions still. Unlike most programmers I find documenting
> my work harder than writing the code :)

Oh I entirely agree :)  FWIW I think this is a fine description of the
option.

>> > +           ((string=? arg "--quiet")
>> > +            (set! %quiet #t)
>> > +            (parse args out))
>> > +
>> 
>> Probably %quiet should be a parameter, and unless we plan on using it
>> for other purposes it should have a more specific name
>> (%verbose-auto-compilation-messages or so; no idea).
>
> Could you clarify please, a parameter to what?

See "Parameters" in the manual.  They are dynamically scoped
thread-local variables.

>> Or, you could always just (current-warning-port (%make-void-port "w"));
>> then the patch is super simple, no conditions needed.  That is what I
>> was originally thinking, but it has the disadvantage that you lose other
>> uses of the current warning port, but maybe that's an advantage too.
>
> Won't we loose compiler warnings then? I did not want to inhibit those.

Humm!  Good point.  But maybe some people want to silence those too?

As far as documentation goes, the right place is the doc/ref/ directory,
specifically doc/ref/guile-invoke.texi.

I will apply your patch as-is once we have docs, unless we can figure
out some more general "chattiness" or verbosity knob that makes sense
for other messages that Guile issues.  I suspect that the "real"
solution for user code is to use a logging module that suits the user's
needs, but for Guile we also have some needs that we should solve in a
simple way.  Could be that this patch is the one :)

Thanks for your patience in contributing!

Andy



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: add command line option to quiet compiler messages
  2016-07-23 10:29     ` Andy Wingo
@ 2016-07-27  0:36       ` Tobin Harding
  0 siblings, 0 replies; 6+ messages in thread
From: Tobin Harding @ 2016-07-27  0:36 UTC (permalink / raw)
  To: Andy Wingo; +Cc: guile-devel

Patch Version 3

On Sat, Jul 23, 2016 at 12:29:41PM +0200, Andy Wingo wrote:
> On Wed 20 Jul 2016 08:41, Tobin Harding <me@tobin.cc> writes:
> 
> > On Thu, Jul 14, 2016 at 12:01:10PM +0200, Andy Wingo wrote:
> >> On Wed 13 Jul 2016 06:08, Tobin Harding <me@tobin.cc> writes:

...

> >> Please resend without tabs, please.  Thanks :)

> But we are missing an (indent-tabs-mode . nil) in the c-mode part I
> think.  That is the proper fix ;)  In my emacs I customized the default
> indent-tabs-mode to nil globally; but that's a personal preference.

All changes were re-indented with '(setq-default indent-tabs-mode nil)'. Please
advise if there is still a problem with the indentation. Being my first patch to
the project I am more than happy to back and forth untill it is perfect so I
know for next time :)

> >> > +           ((string=? arg "--quiet")
> >> > +            (set! %quiet #t)
> >> > +            (parse args out))
> >> > +
> >> 
> >> Probably %quiet should be a parameter, and unless we plan on using it
> >> for other purposes it should have a more specific name
> >> (%verbose-auto-compilation-messages or so; no idea).
> >
> > Could you clarify please, a parameter to what?
> 
> See "Parameters" in the manual.  They are dynamically scoped
> thread-local variables.

I read up on parameters, since this variable is declared in the C file
(load.c) I'm not sure how you mean to use a parameter. Am I missing
something? So, I have kept it as is but renamed it as suggested.

> 
> As far as documentation goes, the right place is the doc/ref/ directory,
> specifically doc/ref/guile-invoke.texi.

This version adds documentation. This is my first time editing Texinfo
format. Entry did not seem to need a @cindex line, if required please
say so, again I am happy to add it and re-submit.

> I will apply your patch as-is once we have docs, unless we can figure
> out some more general "chattiness" or verbosity knob that makes sense
> for other messages that Guile issues.  I suspect that the "real"
> solution for user code is to use a logging module that suits the user's
> needs, but for Guile we also have some needs that we should solve in a
> simple way.  Could be that this patch is the one :)
> 
> Thanks for your patience in contributing!

Thanks for taking the time to look over my patch.

thanks,
Tobin.


---
 doc/ref/guile-invoke.texi     |  7 +++++++
 libguile/load.c               | 33 ++++++++++++++++++++++-----------
 module/ice-9/boot-9.scm       | 22 +++++++++++++++++-----
 module/ice-9/command-line.scm |  6 ++++++
 4 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/doc/ref/guile-invoke.texi b/doc/ref/guile-invoke.texi
index bc33ce0..add96da 100644
--- a/doc/ref/guile-invoke.texi
+++ b/doc/ref/guile-invoke.texi
@@ -172,6 +172,13 @@ Do not load the initialization file, @file{.guile}.  This option only
 has an effect when running interactively; running scripts does not load
 the @file{.guile} file.  @xref{Init File}.
 
+@item --quiet
+Inhibit the output of compiler informational messages. Default behaviour
+is for the compiler to output the file name (fully qualified path name)
+of each file before it is loaded if it is newer than an available
+compiled version, also once compiled, the compiled file name is
+output. The @option{--quiet} flag inhibits these messages.
+
 @item --listen[=@var{p}]
 While this program runs, listen on a local port or a path for REPL
 clients.  If @var{p} starts with a number, it is assumed to be a local
diff --git a/libguile/load.c b/libguile/load.c
index 7ad9a75..b5d80c5 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -226,6 +226,9 @@ static SCM *scm_loc_fresh_auto_compile;
 /* The fallback path for auto-compilation */
 static SCM *scm_loc_compile_fallback_path;
 
+/* Whether we should output compiler informational messages */
+static SCM *scm_loc_verbose_compiler_messages;
+
 /* Ellipsis: "..." */
 static SCM scm_ellipsis;
 
@@ -561,11 +564,13 @@ compiled_is_fresh (SCM full_filename, SCM compiled_filename,
   else
     {
       compiled_is_newer = 0;
-      scm_puts (";;; note: source file ", scm_current_warning_port ());
-      scm_display (full_filename, scm_current_warning_port ());
-      scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
-      scm_display (compiled_filename, scm_current_warning_port ());
-      scm_puts ("\n", scm_current_warning_port ());
+      if (scm_is_true (*scm_loc_verbose_compiler_messages)) {
+        scm_puts (";;; note: source file ", scm_current_warning_port ());
+        scm_display (full_filename, scm_current_warning_port ());
+        scm_puts ("\n;;;       newer than compiled ", scm_current_warning_port ());
+        scm_display (compiled_filename, scm_current_warning_port ());
+        scm_puts ("\n", scm_current_warning_port ());
+      }
     }
 
   return compiled_is_newer;
@@ -1007,9 +1012,11 @@ do_try_auto_compile (void *data)
   SCM source = SCM_PACK_POINTER (data);
   SCM comp_mod, compile_file;
 
-  scm_puts (";;; compiling ", scm_current_warning_port ());
-  scm_display (source, scm_current_warning_port ());
-  scm_newline (scm_current_warning_port ());
+  if (scm_is_true (*scm_loc_verbose_compiler_messages)) {
+    scm_puts (";;; compiling ", scm_current_warning_port ());
+    scm_display (source, scm_current_warning_port ());
+    scm_newline (scm_current_warning_port ());
+  }
 
   comp_mod = scm_c_resolve_module ("system base compile");
   compile_file = scm_module_variable (comp_mod, sym_compile_file);
@@ -1036,9 +1043,11 @@ 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 (";;; compiled ", scm_current_warning_port ());
-      scm_display (res, scm_current_warning_port ());
-      scm_newline (scm_current_warning_port ());
+      if (scm_is_true (*scm_loc_verbose_compiler_messages)) {
+	scm_puts (";;; compiled ", scm_current_warning_port ());
+	scm_display (res, scm_current_warning_port ());
+	scm_newline (scm_current_warning_port ());
+      }
       return res;
     }
   else
@@ -1349,6 +1358,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_verbose_compiler_messages
+    = SCM_VARIABLE_LOC (scm_c_define ("%verbose-compiler-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 99543e7..a0e463c 100644
--- a/module/ice-9/boot-9.scm
+++ b/module/ice-9/boot-9.scm
@@ -3762,6 +3762,20 @@ when none is available, reading FILE-NAME with READER."
     ;; Return GO-FILE-NAME after making sure that it contains a freshly
     ;; compiled version of source file NAME with stat SCMSTAT; return #f
     ;; on failure.
+    (define (cond-warn-compiling file)
+      (if %verbose-compiler-messages
+          (format (current-warning-port) ";;; compiling ~a\n" file)))
+
+    (define (cond-warn-compiled file)
+      (if %verbose-compiler-messages
+          (format (current-warning-port) ";;; compiled ~a\n" file)))
+
+    (define (cond-warn-newer new old)
+      (if %verbose-compiler-messages
+          (format (current-warning-port)
+                  ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
+                  name go-file-name)))
+
     (false-if-exception
      (let ((gostat (and (not %fresh-auto-compile)
                         (stat go-file-name #f))))
@@ -3769,15 +3783,13 @@ when none is available, reading FILE-NAME with READER."
            (load-thunk-from-file go-file-name)
            (begin
              (when gostat
-               (format (current-warning-port)
-                       ";;; note: source file ~a\n;;;       newer than compiled ~a\n"
-                       name go-file-name))
+               (cond-warn-newer name go-file-name))
              (cond
               (%load-should-auto-compile
                (%warn-auto-compilation-enabled)
-               (format (current-warning-port) ";;; compiling ~a\n" name)
+               (cond-warn-compiling name)
                (let ((cfn (compile name)))
-                 (format (current-warning-port) ";;; compiled ~a\n" cfn)
+                 (cond-warn-compiled cfn)
                  (load-thunk-from-file cfn)))
               (else #f)))))
      #:warning "WARNING: compilation of ~a failed:\n" name))
diff --git a/module/ice-9/command-line.scm b/module/ice-9/command-line.scm
index 98d3855..fe639bb 100644
--- a/module/ice-9/command-line.scm
+++ b/module/ice-9/command-line.scm
@@ -136,6 +136,8 @@ If FILE begins with `-' the -s switch is mandatory.
   --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
+  --quiet        inhibit informational compiler output, e.g name of
+                 files being compiled
   --use-srfi=LS  load SRFI modules for the SRFIs in LS,
                  which is a list of numbers like \"2,13,14\"
   -h, --help     display this help and exit
@@ -358,6 +360,10 @@ If FILE begins with `-' the -s switch is mandatory.
             (set! inhibit-user-init? #t)
             (parse args out))
 
+           ((string=? arg "--quiet")
+            (set! %verbose-compiler-messages #f)
+            (parse args out))
+
            ((string-prefix? "--use-srfi=" arg)
             (let ((srfis (map (lambda (x)
                                 (let ((n (string->number x)))
-- 
2.9.0




^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-07-27  0:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-13  4:08 add command line option to quiet compiler messages Tobin Harding
2016-07-14 10:01 ` Andy Wingo
2016-07-20  6:41   ` Tobin Harding
2016-07-23 10:29     ` Andy Wingo
2016-07-27  0:36       ` Tobin Harding
2016-07-20  7:05   ` Tobin Harding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).