[PATCH] Tidy up 'make' output

[PATCH] Tidy up 'make' output

[Colin D Bennett]

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

When thousands of long, wrapped lines full of command line options and
file names are scrolling by on your terminal, it is very hard to pick
out the irregularities in the build process, such as error and warnings.

To make the output of ``make`` easier to parse by eye as it scrolls by,
I added support for "pretty" output for make. 

For example, it replaces the full gcc commands that look like

  gcc -Iutil -I../util -I. -Iinclude -I../include -Wall -W
  -DGRUB_LIBDIR=\"/usr/local/lib/`echo grub/i386-pc | sed 's,x,x,'`\" -g
  -O2 -DGRUB_UTIL=1  -MD -c -o
  grub_setup-util_getroot.o ../util/getroot.c gcc -Ikern -I../kern -I.
  -Iinclude -I../include -Wall -W -DGRUB_LIBDIR=\"/usr/local/lib/`echo
  grub/i386-pc | sed 's,x,x,'`\" -g -O2 -DGRUB_UTIL=1  -MD -c -o
  grub_setup-kern_device.o ../kern/device.c ../kern/device.c: In
  function 'grub_device_iterate': ../kern/device.c:84: warning:
  generating trampoline in object (requires executable
  stack) ../kern/device.c:84: warning: generating trampoline in object
  (requires executable stack) gcc -Ikern -I../kern -I. -Iinclude
  -I../include -Wall -W -DGRUB_LIBDIR=\"/usr/local/lib/`echo
  grub/i386-pc | sed 's,x,x,'`\" -g -O2 -DGRUB_UTIL=1  -MD -c -o
  grub_setup-kern_disk.o ../kern/disk.c gcc -Ikern -I../kern -I.
  -Iinclude -I../include -Wall -W -DGRUB_LIBDIR=\"/usr/local/lib/`echo
  grub/i386-pc | sed 's,x,x,'`\" -g -O2 -DGRUB_UTIL=1  -MD -c -o
  grub_setup-kern_err.o ../kern/err.c gcc -Ikern -I../kern -I. -Iinclude
  -I../include -Wall -W -DGRUB_LIBDIR=\"/usr/local/lib/`echo
  grub/i386-pc | sed 's,x,x,'`\" -g -O2 -DGRUB_UTIL=1  -MD -c -o
  grub_setup-kern_misc.o ../kern/misc.c

with output that, in my opinion, makes it easier to see warnings and
errors:

  COMPILE         ../util/getroot.c
  COMPILE         ../kern/device.c
  ../kern/device.c: In function 'grub_device_iterate':
  ../kern/device.c:84: warning: generating trampoline in object
  (requires executable stack)
  ../kern/device.c:84: warning: generating trampoline in object
  (requires executable stack)
  COMPILE         ../kern/disk.c 
  COMPILE         ../kern/err.c
  COMPILE         ../kern/misc.c

If the variable ``V`` (for verbose) is set to 1, then the traditional
full output is displayed.  This is useful when you need to see which
compiler flags are being used or which source files are being linked
into an object file. Just run

  make V=1

to get the full output.

This patch can be refined further by adding 'pretty' rules for other
build operations to Makefile.in.

Regards,
Colin

[-- Attachment #2: grub_prettymake_2008-06-17.patch --]
[-- Type: text/x-patch, Size: 3805 bytes --]

=== modified file 'Makefile.in'
--- Makefile.in	2008-04-25 19:41:48 +0000
+++ Makefile.in	2008-06-11 05:50:59 +0000
@@ -84,6 +84,39 @@
 YACC = @YACC@
 UNIFONT_HEX = @UNIFONT_HEX@
 
+### Pretty output control ###
+# Set up compiler and linker commands that either is quiet (does not print
+# the command line being executed) or verbose (print the command line).
+_CC := $(CC)
+_TARGET_CC := $(TARGET_CC)
+_STRIP := $(STRIP)
+_GENMODSRC := sh $(srcdir)/genmodsrc.sh
+ifeq ($(V),1)
+ override V_PREFIX :=
+ override CC = $(_CC)
+ override TARGET_CC = $(_CC)
+ override STRIP = $(_STRIP)
+ override GENMODSRC = $(_GENMODSRC)
+ override INFO_GENCMDLIST =
+ override INFO_GENFSLIST =
+ override INFO_GENPARTMAPLIST =
+ override INFO_GEN_FINAL_COMMAND_LIST =
+ override INFO_GEN_FINAL_FS_LIST =
+ override INFO_GEN_FINAL_PARTMAP_LIST =
+else
+ override V_PREFIX := @
+ override CC = @echo "COMPILE         $<"; $(_CC)
+ override TARGET_CC = @echo "COMPILE(TARGET) $<"; $(_TARGET_CC)
+ override STRIP = @echo "STRIP           $@"; $(_STRIP)
+ override GENMODSRC = @echo "GENMODSRC       $@"; $(_GENMODSRC)
+ override INFO_GENCMDLIST = @echo "GENCMDLIST      $@"
+ override INFO_GENFSLIST = @echo "GENFSLIST       $@"
+ override INFO_GENPARTMAPLIST = @echo "GENPARTMAPLIST  $@"
+ override INFO_GEN_FINAL_COMMAND_LIST = @echo "GENCMDLIST[final]  $@"
+ override INFO_GEN_FINAL_FS_LIST = @echo "GENFSLIST[final]   $@"
+ override INFO_GEN_FINAL_PARTMAP_LIST = @echo "GENPARTMAPLIST[final]  $@"
+endif
+
 # Options.
 enable_grub_emu = @enable_grub_emu@
 enable_grub_fstest = @enable_grub_fstest@
@@ -130,13 +163,16 @@
 	  || (rm -f $@; exit 1)
 
 command.lst: $(COMMANDFILES)
-	cat $^ /dev/null | sort > $@
+	$(INFO_GEN_FINAL_COMMAND_LIST)
+	$(V_PREFIX)cat $^ /dev/null | sort > $@
 
 fs.lst: $(FSFILES)
-	cat $^ /dev/null | sort > $@
+	$(INFO_GEN_FINAL_FS_LIST)
+	$(V_PREFIX)cat $^ /dev/null | sort > $@
 
 partmap.lst: $(PARTMAPFILES)
-	cat $^ /dev/null | sort > $@
+	$(INFO_GEN_FINAL_PARTMAP_LIST)
+	$(V_PREFIX)cat $^ /dev/null | sort > $@
 
 ifeq (, $(UNIFONT_HEX))
 else

=== modified file 'genmk.rb'
--- genmk.rb	2008-04-17 14:14:09 +0000
+++ genmk.rb	2008-06-11 05:50:59 +0000
@@ -125,7 +125,7 @@
 	$(TARGET_CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -c -o $@ $<
 
 #{mod_src}: moddep.lst genmodsrc.sh
-	sh $(srcdir)/genmodsrc.sh '#{mod_name}' $< > $@ || (rm -f $@; exit 1)
+	$(GENMODSRC) '#{mod_name}' $< > $@ || (rm -f $@; exit 1)
 
 ifneq ($(#{prefix}_EXPORTS),no)
 #{defsym}: #{pre_obj}
@@ -157,18 +157,21 @@
 PARTMAPFILES += #{partmap}
 
 #{command}: #{src} $(#{src}_DEPENDENCIES) gencmdlist.sh
-	set -e; \
-	  $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -E $< \
+	$(INFO_GENCMDLIST)
+	$(V_PREFIX)set -e; \
+	  $(_TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -E $< \
 	  | sh $(srcdir)/gencmdlist.sh #{symbolic_name} > $@ || (rm -f $@; exit 1)
 
 #{fs}: #{src} $(#{src}_DEPENDENCIES) genfslist.sh
-	set -e; \
-	  $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -E $< \
+	$(INFO_GENFSLIST)
+	$(V_PREFIX)set -e; \
+	  $(_TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -E $< \
 	  | sh $(srcdir)/genfslist.sh #{symbolic_name} > $@ || (rm -f $@; exit 1)
 
 #{partmap}: #{src} $(#{src}_DEPENDENCIES) genpartmaplist.sh
-	set -e; \
-	  $(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -E $< \
+	$(INFO_GENPARTMAPLIST)
+	$(V_PREFIX)set -e; \
+	  $(_TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_#{flag}) $(#{prefix}_#{flag}) -E $< \
 	  | sh $(srcdir)/genpartmaplist.sh #{symbolic_name} > $@ || (rm -f $@; exit 1)
 
 

Re: [PATCH] Tidy up 'make' output

[Pavel Roskin]

On Tue, 2008-06-17 at 10:44 -0700, Colin D Bennett wrote:
> When thousands of long, wrapped lines full of command line options and
> file names are scrolling by on your terminal, it is very hard to pick
> out the irregularities in the build process, such as error and warnings.

I like the idea, but the massive use of "override" doesn't looks right.
I'd rather see variables with different names used throughout the
makefiles.  Linux makefiles don't use "override" at all.  "override"
should be the last resort if everything else fails.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Tidy up 'make' output

[Colin D Bennett]

On Tue, 17 Jun 2008 15:57:47 -0400
Pavel Roskin <proski@gnu.org> wrote:

> On Tue, 2008-06-17 at 10:44 -0700, Colin D Bennett wrote:
> > When thousands of long, wrapped lines full of command line options
> > and file names are scrolling by on your terminal, it is very hard
> > to pick out the irregularities in the build process, such as error
> > and warnings.
> 
> I like the idea, but the massive use of "override" doesn't looks
> right. I'd rather see variables with different names used throughout
> the makefiles.  Linux makefiles don't use "override" at all.
> "override" should be the last resort if everything else fails.

Ok.  Well, are implicit make rules ever used in the makefiles?  If so,
we would have to make them explicit in order to use a different
compiler variable.  I tried to touch the least number of things
possible with my patch.

If you wish, I can take a crack at eliminating the use of 'override'
and using a different set of make variables for the instrumented calls
to commands.

Regards,
Colin

Re: [PATCH] Tidy up 'make' output

[Pavel Roskin]

On Tue, 2008-06-17 at 13:07 -0700, Colin D Bennett wrote:
> On Tue, 17 Jun 2008 15:57:47 -0400
> Pavel Roskin <proski@gnu.org> wrote:
> 
> > On Tue, 2008-06-17 at 10:44 -0700, Colin D Bennett wrote:
> > > When thousands of long, wrapped lines full of command line options
> > > and file names are scrolling by on your terminal, it is very hard
> > > to pick out the irregularities in the build process, such as error
> > > and warnings.
> > 
> > I like the idea, but the massive use of "override" doesn't looks
> > right. I'd rather see variables with different names used throughout
> > the makefiles.  Linux makefiles don't use "override" at all.
> > "override" should be the last resort if everything else fails.
> 
> Ok.  Well, are implicit make rules ever used in the makefiles?

I don't think so.  "make -r" is working fine for me.

>   If so,
> we would have to make them explicit in order to use a different
> compiler variable.  I tried to touch the least number of things
> possible with my patch.

That's a good idea, but doing things right should take priority.  If you
want, you can split your work along different lines - silence CC first,
then LD and so on.

> If you wish, I can take a crack at eliminating the use of 'override'
> and using a different set of make variables for the instrumented calls
> to commands.

That would be great.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Tidy up 'make' output

[Robert Millan]

On Tue, Jun 17, 2008 at 10:44:48AM -0700, Colin D Bennett wrote:
> with output that, in my opinion, makes it easier to see warnings and
> errors:
> 
>   COMPILE         ../util/getroot.c
>   COMPILE         ../kern/device.c
>   ../kern/device.c: In function 'grub_device_iterate':
>   ../kern/device.c:84: warning: generating trampoline in object
>   (requires executable stack)
>   ../kern/device.c:84: warning: generating trampoline in object
>   (requires executable stack)
>   COMPILE         ../kern/disk.c 
>   COMPILE         ../kern/err.c
>   COMPILE         ../kern/misc.c

I don't like the idea of hiding information this way.  If the goal is to
catch warnings, I think -Werror can do a much better job (and catching
errors shouldn't be a problem unless you're using make -j or -k).

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] Tidy up 'make' output

[Pavel Roskin]

On Tue, 2008-06-17 at 22:37 +0200, Robert Millan wrote:

> I don't like the idea of hiding information this way.

If something fails, "make V=1" can be used to find the command.

> If the goal is to
> catch warnings, I think -Werror can do a much better job (and catching
> errors shouldn't be a problem unless you're using make -j or -k).

Even with -j and -k, rerunning make would show the error at the end.

But -Werror would not help for linker warnings and whatever else some
utility may want to tell users.  Also, projects that use -Werror often
find the build fail because a new, stricter compiler finds warnings in
headers of older libraries.

Also, using -Werror would put pressure on developers to fix warnings in
a provisional way rather than address the real issue. 

The GRUB build is especially noisy.  It's hard to say what's being
generated at the given time without looking very closely.  Shorter
output could possibly help improve the build system, because it would be
clear where the build is spending most of the time.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Tidy up 'make' output

[Stefan Reinauer]

Robert Millan wrote:
> On Tue, Jun 17, 2008 at 10:44:48AM -0700, Colin D Bennett wrote:
>   
>> with output that, in my opinion, makes it easier to see warnings and
>> errors:
>>
>>   COMPILE         ../util/getroot.c
>>   COMPILE         ../kern/device.c
>>   ../kern/device.c: In function 'grub_device_iterate':
>>   ../kern/device.c:84: warning: generating trampoline in object
>>   (requires executable stack)
>>   ../kern/device.c:84: warning: generating trampoline in object
>>   (requires executable stack)
>>   COMPILE         ../kern/disk.c 
>>   COMPILE         ../kern/err.c
>>   COMPILE         ../kern/misc.c
>>     
>
> I don't like the idea of hiding information this way.  If the goal is to
> catch warnings, I think -Werror can do a much better job (and catching
> errors shouldn't be a problem unless you're using make -j or -k).
>   
I would not consider this "hiding information". The information you see
(CFLAGS for example) don't really change across the lines and there's
always the chance to say V=1 to see all the compiler lines. The
opposite: The current forest of duplicate information is really what is
hiding the relevant information between a lot of uninteresting fuzz.
Maybe, you guys would prefer to set V=1 as the default, so one would
have to say V=0 to get above output? I am currently only compiling grub
with make -s, because that is the only way to get any decently parsable
output for finding issues in the code.

Please, please, don't use -Werror.  GRUB2 is currently hard enough to
build and the build system is less than optimal and elegant. While I
agree that clean code never throughs warnings, the amazing number of
different gccs and build environments out there would make developing
for grub2 and compiling it very hard. There are quite a number of
warnings that do not matter because the developers simply know better
than the compiler. -Werror will lead to ugly workarounds to suppress
these warnings and make adoption of new tool chain versions a task from
hell.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info@coresystems.de  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

Re: [PATCH] Tidy up 'make' output

[Robert Millan]

On Wed, Jun 18, 2008 at 12:22:43AM +0200, Stefan Reinauer wrote:
> I would not consider this "hiding information". The information you see
> (CFLAGS for example) don't really change across the lines and there's
> always the chance to say V=1 to see all the compiler lines. The
> opposite: The current forest of duplicate information is really what is
> hiding the relevant information between a lot of uninteresting fuzz.
> Maybe, you guys would prefer to set V=1 as the default, so one would
> have to say V=0 to get above output? I am currently only compiling grub
> with make -s, because that is the only way to get any decently parsable
> output for finding issues in the code.

Yeah you have a point here.

> Please, please, don't use -Werror.  GRUB2 is currently hard enough to
> build and the build system is less than optimal and elegant. While I
> agree that clean code never throughs warnings, the amazing number of
> different gccs and build environments out there would make developing
> for grub2 and compiling it very hard. There are quite a number of
> warnings that do not matter because the developers simply know better
> than the compiler. -Werror will lead to ugly workarounds to suppress
> these warnings and make adoption of new tool chain versions a task from
> hell.

The problem with GRUB is that even a minor error can easily become critical
if it prevents you from booting.  Often -Werror can mean extra work just to
shut up a warning (although I wouldn't consider this a workaround, unless
there's some example I'm missing), but sometimes it can catch bugs that turn
out to be really hard to debug, like those involving memory corruption.

I think in the long run it would pay off.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] Tidy up 'make' output

[Colin D Bennett]

On Tue, 17 Jun 2008 22:37:10 +0200
Robert Millan <rmh@aybabtu.com> wrote:

> On Tue, Jun 17, 2008 at 10:44:48AM -0700, Colin D Bennett wrote:
> > with output that, in my opinion, makes it easier to see warnings and
> > errors:
> > 
> >   COMPILE         ../util/getroot.c
> >   COMPILE         ../kern/device.c
> >   ../kern/device.c: In function 'grub_device_iterate':
> >   ../kern/device.c:84: warning: generating trampoline in object
> >   (requires executable stack)
> >   ../kern/device.c:84: warning: generating trampoline in object
> >   (requires executable stack)
> >   COMPILE         ../kern/disk.c 
> >   COMPILE         ../kern/err.c
> >   COMPILE         ../kern/misc.c
> 
> I don't like the idea of hiding information this way.  If the goal is
> to catch warnings, I think -Werror can do a much better job (and
> catching errors shouldn't be a problem unless you're using make -j or
> -k).

What about all the "trampoline" and strict-aliasing warnings (there must
be hundreds of them)?  I'm all for warning-free code, but if we try to
use -Werror, the code won't even begin to compile in the current state.

Re: [PATCH] Tidy up 'make' output

[Robert Millan]

On Tue, Jun 17, 2008 at 03:31:54PM -0700, Colin D Bennett wrote:
> I'm all for warning-free code, but if we try to
> use -Werror, the code won't even begin to compile in the current state.

Of course, I wasn't proposing to add -Werror in the current state and just
throw the hot potato into everyone ;-)

Ideally, someone (or all of us ;-)) could do the work to eliminate those
warnings, then add -Werror, and at that point it's the responsibility of
every contributor that new code is warning-free.

So is the proposed situation you don't like, or the path that would be
needed to archieve it?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] Tidy up 'make' output

[Pavel Roskin]

On Wed, 2008-06-18 at 19:46 +0200, Robert Millan wrote:
> On Tue, Jun 17, 2008 at 03:31:54PM -0700, Colin D Bennett wrote:
> > I'm all for warning-free code, but if we try to
> > use -Werror, the code won't even begin to compile in the current state.
> 
> Of course, I wasn't proposing to add -Werror in the current state and just
> throw the hot potato into everyone ;-)
> 
> Ideally, someone (or all of us ;-)) could do the work to eliminate those
> warnings, then add -Werror, and at that point it's the responsibility of
> every contributor that new code is warning-free.

There will be some combinations of gcc and libraries that will produce
warnings.  It should be easy to turn off -Werror on the make command
line if necessary.

> So is the proposed situation you don't like, or the path that would be
> needed to archieve it?

That's OK, but it's doesn't make build system changes unnecessary.  The
less noisy build system will help find other messages that -Werror won't
catch, such as linker warnings.  It will help understand what is
happening and what is potentially wrong or suboptimal.

For example, I'm seeing warnings from xfs.c that nobody is fixing.  I
can fix the warning by changing the code so that it does exactly what
it's doing now but doesn't cause a warning.  The problem is, I don't
see corresponding structures in the Linux xfs code.  I don't have time
to investigate xfs implementation to see if I'm possibly hiding a bug.

I can imagine that somebody knows more about xfs but doesn't see the
warning.  Once we make the warning visible, maybe that person will have
a look and make a better patch.

It's also possible that somebody who want to install GRUB in xfs will be
extra cautious when seeing the warning.  It's actually a good thing.
Sure, not having the warning will be even better.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Tidy up 'make' output

[Robert Millan]

On Wed, Jun 18, 2008 at 02:02:17PM -0400, Pavel Roskin wrote:
> On Wed, 2008-06-18 at 19:46 +0200, Robert Millan wrote:
> > On Tue, Jun 17, 2008 at 03:31:54PM -0700, Colin D Bennett wrote:
> > > I'm all for warning-free code, but if we try to
> > > use -Werror, the code won't even begin to compile in the current state.
> > 
> > Of course, I wasn't proposing to add -Werror in the current state and just
> > throw the hot potato into everyone ;-)
> > 
> > Ideally, someone (or all of us ;-)) could do the work to eliminate those
> > warnings, then add -Werror, and at that point it's the responsibility of
> > every contributor that new code is warning-free.
> 
> There will be some combinations of gcc and libraries that will produce
> warnings.  It should be easy to turn off -Werror on the make command
> line if necessary.
> 
> > So is the proposed situation you don't like, or the path that would be
> > needed to archieve it?
> 
> That's OK, but it's doesn't make build system changes unnecessary.  The
> less noisy build system will help find other messages that -Werror won't
> catch, such as linker warnings.  It will help understand what is
> happening and what is potentially wrong or suboptimal.
> 
> For example, I'm seeing warnings from xfs.c that nobody is fixing.  I
> can fix the warning by changing the code so that it does exactly what
> it's doing now but doesn't cause a warning.  The problem is, I don't
> see corresponding structures in the Linux xfs code.  I don't have time
> to investigate xfs implementation to see if I'm possibly hiding a bug.

Ok.  I got no time to review all the warnings and make -Werror possible atm,
but I agree that making them more visible can help archieve that in the long
term.

> It's also possible that somebody who want to install GRUB in xfs will be
> extra cautious when seeing the warning.  It's actually a good thing.
> Sure, not having the warning will be even better.

Based on my daily experience with people installing from packages, I assure
you they don't check the code for warnings ;-)

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)