Re: [PATCH] Tidy up 'make' output
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 I know my rights; I want my phone call! What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
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 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
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 I know my rights; I want my phone call! What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
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 I know my rights; I want my phone call! What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
On Tue, 17 Jun 2008 22:37:10 +0200 Robert Millan <[EMAIL PROTECTED]> 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. ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
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: [EMAIL PROTECTED] • http://www.coresystems.de/ Registergericht: Amtsgericht Freiburg • HRB 7656 Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
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 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
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 I know my rights; I want my phone call! What good is a phone call… if you are unable to speak? (as seen on /.) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
On Tue, 2008-06-17 at 13:07 -0700, Colin D Bennett wrote: > On Tue, 17 Jun 2008 15:57:47 -0400 > Pavel Roskin <[EMAIL PROTECTED]> 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 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
On Tue, 17 Jun 2008 15:57:47 -0400 Pavel Roskin <[EMAIL PROTECTED]> 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 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Tidy up 'make' output
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 ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Tidy up 'make' output
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=== modified file 'Makefile.in'
--- Makefile.in 2008-04-25 19:41:48 +
+++ Makefile.in 2008-06-11 05:50:59 +
@@ -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 +
+++ genmk.rb 2008-06-11 05:50:59 +
@@ -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 ($(#{pr
