[PATCH] Build system improvement

[PATCH] Build system improvement

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 1327 bytes --]

This patch modifies several files in the build system (mainly common.rmk
and genmk.rb) to reduce the general verbosity of the build process to a
manageable, semi-informative level. Thus, what currently appears as
"gcc" calls, several lines long each is turned into lines like:

[M xfs.mod] COMPILE ../src/fs/xfs.c ->
xfs_mod-fs_xfs.o                                                                                                                                  
[M xfs.mod] LINK xfs_mod-fs_xfs.o ->
pre-xfs.o                                                                                                                                           
[M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o

And so on. The change also makes warning-hunting marginally easier,
though not by much since the patch intentionally shows a line for nearly
every process that did so previously. This behavior could be simplified
further if needed - this post is more of an RFC than anything else.
Also, it is by no means thorough or complete - only the most common
processes have been addressed - as I'm a bit busy with exams.

The patch makes the new behavior the default one, so a new make-time
option is added: V (for "verbose"), which must have the value 1 in order
to get the behavior, as in "make V=1"

[-- Attachment #1.2: build_system.diff --]
[-- Type: text/x-patch, Size: 9871 bytes --]

Index: Makefile.in
===================================================================
--- Makefile.in	(revisión: 1954)
+++ Makefile.in	(copia de trabajo)
@@ -138,18 +138,30 @@
 CLEANFILES += $(pkglib_DATA) $(pkgdata_DATA)
 pkglib_DATA += moddep.lst command.lst fs.lst partmap.lst
 moddep.lst: $(DEFSYMFILES) $(UNDSYMFILES) genmoddep.awk
-	cat $(DEFSYMFILES) /dev/null \
+ifneq ($(V),1)
+	@echo [L moddep.lst] Building MODULE DEPENDENCIES list
+endif
+	$(SILENT)cat $(DEFSYMFILES) /dev/null \
 	  | $(AWK) -f $(srcdir)/genmoddep.awk $(UNDSYMFILES) > $@ \
 	  || (rm -f $@; exit 1)
 
 command.lst: $(COMMANDFILES)
-	cat $^ /dev/null | sort > $@
+ifneq ($(V),1)
+	@echo [L command.lst] Building COMMAND list
+endif
+	$(SILENT)cat $^ /dev/null | sort > $@
 
 fs.lst: $(FSFILES)
-	cat $^ /dev/null | sort > $@
+ifneq ($(V),1)
+	@echo [L fs.lst] Building FILESYSTEM list
+endif
+	$(SILENT)cat $^ /dev/null | sort > $@
 
 partmap.lst: $(PARTMAPFILES)
-	cat $^ /dev/null | sort > $@
+ifneq ($(V),1)
+	@echo [L partmap.lst] Building PARTMAP list
+endif
+	$(SILENT)cat $^ /dev/null | sort > $@
 
 ifeq (, $(UNIFONT_BDF))
 else
Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(revisión: 1954)
+++ conf/common.rmk	(copia de trabajo)
@@ -1,5 +1,9 @@
 # -*- makefile -*-
 
+ifneq ($(V),1)
+SILENT = @
+endif
+
 # For grub-mkelfimage.
 bin_UTILITIES += grub-mkelfimage
 grub_mkelfimage_SOURCES = util/elf/grub-mkimage.c util/misc.c \
@@ -125,26 +129,27 @@
 
 # For grub-mkconfig
 grub-mkconfig: util/grub-mkconfig.in config.status
-	./config.status --file=$@:$<
-	chmod +x $@
+	$(SILENT)./config.status --file=$@:$<
+	$(SILENT)chmod +x $@
 sbin_SCRIPTS += grub-mkconfig
 CLEANFILES += grub-mkconfig
 
 grub-mkconfig_lib: util/grub-mkconfig_lib.in config.status
-	./config.status --file=$@:$<
-	chmod +x $@
+	$(SILENT)./config.status --file=$@:$<
+	$(SILENT)chmod +x $@
 lib_DATA += grub-mkconfig_lib
 CLEANFILES += grub-mkconfig_lib
 
 update-grub_lib: util/update-grub_lib.in config.status
-	./config.status --file=$@:$<
-	chmod +x $@
+	$(SILENT)./config.status --file=$@:$<
+	$(SILENT)chmod +x $@
 lib_DATA += update-grub_lib
 CLEANFILES += update-grub_lib
 
 %: util/grub.d/%.in config.status
-	./config.status --file=$@:$<
-	chmod +x $@
+#no need to echo nothing here: config.status already does so
+	$(SILENT)./config.status --file=$@:$<
+	$(SILENT)chmod +x $@
 grub-mkconfig_SCRIPTS = 00_header 10_linux 10_hurd 10_freebsd 30_os-prober 40_custom
 ifeq ($(target_os), cygwin)
 grub-mkconfig_SCRIPTS += 10_windows
Index: genmk.rb
===================================================================
--- genmk.rb	(revisión: 1954)
+++ genmk.rb	(copia de trabajo)
@@ -57,10 +57,16 @@
 MOSTLYCLEANFILES += #{deps_str}
 
 #{@name}: #{exe}
+ifneq ($(V),1)
+	@echo [I #{@name}] OBJCOPY $< '->' $@
+endif
 	$(OBJCOPY) -O binary -R .note -R .comment -R .note.gnu.build-id $< $@
 
 #{exe}: #{objs_str}
-	$(TARGET_CC) -o $@ $^ $(TARGET_LDFLAGS) $(#{prefix}_LDFLAGS)
+ifneq ($(V),1)
+	@echo [I #{@name}] LINK $< '->' $@
+endif
+	$(SILENT)$(TARGET_CC) -o $@ $^ $(TARGET_LDFLAGS) $(#{prefix}_LDFLAGS)
 
 " + objs.collect_with_index do |obj, i|
       src = sources[i]
@@ -71,7 +77,10 @@
       dir = File.dirname(src)
       
       "#{obj}: #{src} $(#{src}_DEPENDENCIES)
-	$(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) #{extra_flags} $(TARGET_#{flag}) $(#{prefix}_#{flag}) -MD -c -o $@ $<
+ifneq ($(V),1)
+	@echo [I #{@name}] COMPILE $< '->' $@
+endif
+	$(SILENT)$(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) #{extra_flags} $(TARGET_#{flag}) $(#{prefix}_#{flag}) -MD -c -o $@ $<
 -include #{dep}
 
 "
@@ -113,29 +122,47 @@
 UNDSYMFILES += #{undsym}
 
 #{@name}: #{pre_obj} #{mod_obj} $(TARGET_OBJ2ELF)
-	-rm -f $@
-	$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) $(MODULE_LDFLAGS) -Wl,-r,-d -o $@ #{pre_obj} #{mod_obj}
-	if test ! -z $(TARGET_OBJ2ELF); then ./$(TARGET_OBJ2ELF) $@ || (rm -f $@; exit 1); fi
-	$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -K _grub_mod_init -K _grub_mod_fini -R .note -R .comment $@
+	$(SILENT)-rm -f $@
+ifneq ($(V),1)
+	@echo [M #{@name}] LINK #{pre_obj} #{mod_obj} '->' $@
+endif
+	$(SILENT)$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) $(MODULE_LDFLAGS) -Wl,-r,-d -o $@ #{pre_obj} #{mod_obj}
+	$(SILENT)if test ! -z $(TARGET_OBJ2ELF); then ./$(TARGET_OBJ2ELF) $@ || (rm -f $@; exit 1); fi
+ifneq ($(V),1)
+	@echo [M #{@name}] STRIP $@
+endif
+	$(SILENT)$(STRIP) --strip-unneeded -K grub_mod_init -K grub_mod_fini -K _grub_mod_init -K _grub_mod_fini -R .note -R .comment $@
 
 #{pre_obj}: $(#{prefix}_DEPENDENCIES) #{objs_str}
-	-rm -f $@
-	$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) -Wl,-r,-d -o $@ #{objs_str}
+	$(SILENT)-rm -f $@
+ifneq ($(V),1)
+	@echo [M #{@name}] LINK #{objs_str} '->' $@
+endif
+	$(SILENT)$(TARGET_CC) $(#{prefix}_LDFLAGS) $(TARGET_LDFLAGS) -Wl,-r,-d -o $@ #{objs_str}
 
 #{mod_obj}: #{mod_src}
-	$(TARGET_CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -c -o $@ $<
+ifneq ($(V),1)
+	@echo [M #{@name}] COMPILE $< '->' $@
+endif
+	$(SILENT)$(TARGET_CC) $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -c -o $@ $<
 
 #{mod_src}: $(builddir)/moddep.lst $(srcdir)/genmodsrc.sh
-	sh $(srcdir)/genmodsrc.sh '#{mod_name}' $< > $@ || (rm -f $@; exit 1)
+	$(SILENT)sh $(srcdir)/genmodsrc.sh '#{mod_name}' $< > $@ || (rm -f $@; exit 1)
 
 ifneq ($(#{prefix}_EXPORTS),no)
 #{defsym}: #{pre_obj}
-	$(NM) -g --defined-only -P -p $< | sed 's/^\\([^ ]*\\).*/\\1 #{mod_name}/' > $@
+ifneq ($(V),1)
+	@echo [M #{@name}] Looking for EXPORTED SYMBOL definitions: $<
 endif
+	$(SILENT)$(NM) -g --defined-only -P -p $< | sed 's/^\\([^ ]*\\).*/\\1 #{mod_name}/' > $@
+endif
 
 #{undsym}: #{pre_obj}
-	echo '#{mod_name}' > $@
-	$(NM) -u -P -p $< | cut -f1 -d' ' >> $@
+ifneq ($(V),1)
+	@echo [M #{@name}] Looking for REQUESTED SYMBOL definitions: $<
+endif
+	$(SILENT)echo '#{mod_name}' > $@
+	$(SILENT)$(NM) -u -P -p $< | cut -f1 -d' ' >> $@
 
 " + objs.collect_with_index do |obj, i|
       src = sources[i]
@@ -149,7 +176,10 @@
       dir = File.dirname(src)
 
       "#{obj}: #{src} $(#{src}_DEPENDENCIES)
-	$(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) #{extra_flags} $(TARGET_#{flag}) $(#{prefix}_#{flag}) -MD -c -o $@ $<
+ifneq ($(V),1)
+	@echo [M #{@name}] COMPILE $< '->' $@
+endif
+	$(SILENT)$(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) #{extra_flags} $(TARGET_#{flag}) $(#{prefix}_#{flag}) -MD -c -o $@ $<
 -include #{dep}
 
 CLEANFILES += #{command} #{fs} #{partmap}
@@ -158,17 +188,26 @@
 PARTMAPFILES += #{partmap}
 
 #{command}: #{src} $(#{src}_DEPENDENCIES) gencmdlist.sh
-	set -e; \
+ifneq ($(V),1)
+	@echo [M #{@name}] Looking for COMMAND definitions: $<
+endif
+	$(SILENT)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; \
+ifneq ($(V),1)
+	@echo [M #{@name}] Looking for FILESYSTEM definitions: $<
+endif
+	$(SILENT)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; \
+ifneq ($(V),1)
+	@echo [M #{@name}] Looking for PARTMAP definitions: $<
+endif
+	$(SILENT)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)
 
@@ -199,7 +238,10 @@
 MOSTLYCLEANFILES += #{deps_str}
 
 #{@name}: $(#{prefix}_DEPENDENCIES) #{objs_str}
-	$(CC) -o $@ #{objs_str} $(LDFLAGS) $(#{prefix}_LDFLAGS)
+ifneq ($(V),1)
+	@echo [U #{@name}] LINK $< '->' $@
+endif
+	$(SILENT)$(CC) -o $@ #{objs_str} $(LDFLAGS) $(#{prefix}_LDFLAGS)
 
 " + objs.collect_with_index do |obj, i|
       src = sources[i]
@@ -208,7 +250,10 @@
       dir = File.dirname(src)
 
       "#{obj}: #{src} $(#{src}_DEPENDENCIES)
-	$(CC) -I#{dir} -I$(srcdir)/#{dir} $(CPPFLAGS) $(CFLAGS) -DGRUB_UTIL=1 $(#{prefix}_CFLAGS) -MD -c -o $@ $<
+ifneq ($(V),1)
+	@echo [U #{@name}] COMPILE $< '->' $@
+endif
+	$(SILENT)$(CC) -I#{dir} -I$(srcdir)/#{dir} $(CPPFLAGS) $(CFLAGS) -DGRUB_UTIL=1 $(#{prefix}_CFLAGS) -MD -c -o $@ $<
 -include #{dep}
 
 "
@@ -237,7 +282,10 @@
 MOSTLYCLEANFILES += #{deps_str}
 
 #{@name}: $(#{prefix}_DEPENDENCIES) #{objs_str}
-	$(TARGET_CC) -o $@ #{objs_str} $(TARGET_LDFLAGS) $(#{prefix}_LDFLAGS)
+ifneq ($(V),1)
+	@echo [P #{@name}] LINK $< '->' $@
+endif
+	$(SILENT)$(TARGET_CC) -o $@ #{objs_str} $(TARGET_LDFLAGS) $(#{prefix}_LDFLAGS)
 
 " + objs.collect_with_index do |obj, i|
       src = sources[i]
@@ -246,7 +294,10 @@
       dir = File.dirname(src)
 
       "#{obj}: #{src} $(#{src}_DEPENDENCIES)
-	$(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -MD -c -o $@ $<
+ifneq ($(V),1)
+	@echo [P #{@name}] COMPILE $< '->' $@
+endif
+	$(SILENT)$(TARGET_CC) -I#{dir} -I$(srcdir)/#{dir} $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) $(#{prefix}_CFLAGS) -MD -c -o $@ $<
 -include #{dep}
 
 "
@@ -273,8 +324,9 @@
     "CLEANFILES += #{@name}
 
 #{@name}: #{src} $(#{src}_DEPENDENCIES) config.status
-	./config.status --file=#{name}:#{src}
-	chmod +x $@
+#no need to echo nothing here: config.status already does so
+	$(SILENT)./config.status --file=#{name}:#{src}
+	$(SILENT)chmod +x $@
 
 "
   end

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] Build system improvement

[Vesa Jääskeläinen]

Javier Martín wrote:
> This patch modifies several files in the build system (mainly common.rmk
> and genmk.rb) to reduce the general verbosity of the build process to a
> manageable, semi-informative level. Thus, what currently appears as
> "gcc" calls, several lines long each is turned into lines like:
> 
> [M xfs.mod] COMPILE ../src/fs/xfs.c ->
> xfs_mod-fs_xfs.o                                                                                                                                  
> [M xfs.mod] LINK xfs_mod-fs_xfs.o ->
> pre-xfs.o                                                                                                                                           
> [M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o
> 
> And so on. The change also makes warning-hunting marginally easier,
> though not by much since the patch intentionally shows a line for nearly
> every process that did so previously. This behavior could be simplified
> further if needed - this post is more of an RFC than anything else.
> Also, it is by no means thorough or complete - only the most common
> processes have been addressed - as I'm a bit busy with exams.
> 
> The patch makes the new behavior the default one, so a new make-time
> option is added: V (for "verbose"), which must have the value 1 in order
> to get the behavior, as in "make V=1"

First of all I would like compiling process to be more tidier.

However, what is the difference between Colin's similar patch?

Re: [PATCH] Build system improvement

[Javier Martín]

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

El dom, 25-01-2009 a las 12:34 +0200, Vesa Jääskeläinen escribió:
> Javier Martín wrote:
> > This patch modifies several files in the build system (mainly common.rmk
> > and genmk.rb) to reduce the general verbosity of the build process to a
> > manageable, semi-informative level. Thus, what currently appears as
> > "gcc" calls, several lines long each is turned into lines like:
> > 
> > [M xfs.mod] COMPILE ../src/fs/xfs.c ->
> > xfs_mod-fs_xfs.o                                                                                                                                  
> > [M xfs.mod] LINK xfs_mod-fs_xfs.o ->
> > pre-xfs.o                                                                                                                                           
> > [M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o
> > 
> > And so on. The change also makes warning-hunting marginally easier,
> > though not by much since the patch intentionally shows a line for nearly
> > every process that did so previously. This behavior could be simplified
> > further if needed - this post is more of an RFC than anything else.
> > Also, it is by no means thorough or complete - only the most common
> > processes have been addressed - as I'm a bit busy with exams.
> > 
> > The patch makes the new behavior the default one, so a new make-time
> > option is added: V (for "verbose"), which must have the value 1 in order
> > to get the behavior, as in "make V=1"
> 
> First of all I would like compiling process to be more tidier.
> 
> However, what is the difference between Colin's similar patch?
Oops... I did not remember Colin's patch, it's been so long since it was
last discussed that my mind considered it deceased for all intents and
purposes. I'll look into the post history and see what it did compared
to mine - if I get it to compile against a current SVN grub, that is.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] Build system improvement

[Colin D Bennett]

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

On Sun, 25 Jan 2009 14:23:29 +0100
Javier Martín <lordhabbit@gmail.com> wrote:

> El dom, 25-01-2009 a las 12:34 +0200, Vesa Jääskeläinen escribió:
> > Javier Martín wrote:
> > > This patch modifies several files in the build system (mainly common.rmk
> > > and genmk.rb) to reduce the general verbosity of the build process to a
> > > manageable, semi-informative level. Thus, what currently appears as
> > > "gcc" calls, several lines long each is turned into lines like:
> > > 
> > > [M xfs.mod] COMPILE ../src/fs/xfs.c ->
> > > xfs_mod-fs_xfs.o                                                                                                                                  
> > > [M xfs.mod] LINK xfs_mod-fs_xfs.o ->
> > > pre-xfs.o                                                                                                                                           
> > > [M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o
> > > 
> > > And so on. The change also makes warning-hunting marginally easier,
> > > though not by much since the patch intentionally shows a line for nearly
> > > every process that did so previously. This behavior could be simplified
> > > further if needed - this post is more of an RFC than anything else.
> > > Also, it is by no means thorough or complete - only the most common
> > > processes have been addressed - as I'm a bit busy with exams.
> > > 
> > > The patch makes the new behavior the default one, so a new make-time
> > > option is added: V (for "verbose"), which must have the value 1 in order
> > > to get the behavior, as in "make V=1"
> > 
> > First of all I would like compiling process to be more tidier.
> > 
> > However, what is the difference between Colin's similar patch?
> Oops... I did not remember Colin's patch, it's been so long since it was
> last discussed that my mind considered it deceased for all intents and
> purposes. I'll look into the post history and see what it did compared
> to mine - if I get it to compile against a current SVN grub, that is.

Basically the difference is that my 'prettymake' patch redefines
the program variables like TARGET_CC, etc. so that they cause the echo
of a nice like when they are executed.

This is the main part of the patch (from Makefile.in)


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


I implemented it this way because it was easiest, since I didn't have
to hunt down all the usages of the tools to add the output control.

This method has downsides, and I think everyone will agree Javier's
method is much more robust and more transparent as to what it is
doing.  Also, my patch no longer works, which shows its fragility:  For
instance, in current GRUB SVN the value of TARGET_CC is being
referenced outside the context of actually executing it (its value is
being assigned to another variable).  This breaks with my patch because
of the way TARGET_CC is redefined.

If you want to see my whole patch (against older GRUB SVN -- it
doesn't quite work against current GRUB for the reason mentioned
above), you can see it at
<http://grub.gibibit.com/patches/prettymake_2008-07-04.patch>

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Build system improvement

[phcoder]

I don't see any stoppers to merge this patch
Javier Martín wrote:
> This patch modifies several files in the build system (mainly common.rmk
> and genmk.rb) to reduce the general verbosity of the build process to a
> manageable, semi-informative level. Thus, what currently appears as
> "gcc" calls, several lines long each is turned into lines like:
> 
> [M xfs.mod] COMPILE ../src/fs/xfs.c ->
> xfs_mod-fs_xfs.o                                                                                                                                  
> [M xfs.mod] LINK xfs_mod-fs_xfs.o ->
> pre-xfs.o                                                                                                                                           
> [M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o
> 
> And so on. The change also makes warning-hunting marginally easier,
> though not by much since the patch intentionally shows a line for nearly
> every process that did so previously. This behavior could be simplified
> further if needed - this post is more of an RFC than anything else.
> Also, it is by no means thorough or complete - only the most common
> processes have been addressed - as I'm a bit busy with exams.
> 
> The patch makes the new behavior the default one, so a new make-time
> option is added: V (for "verbose"), which must have the value 1 in order
> to get the behavior, as in "make V=1"
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Build system improvement

[Yoshinori K. Okuji]

On Saturday 11 April 2009 08:25:50 phcoder wrote:
> I don't see any stoppers to merge this patch

Personally, I don't like this so much. Is it so useful?

Regards,
Okuji

>
> Javier Martín wrote:
> > This patch modifies several files in the build system (mainly common.rmk
> > and genmk.rb) to reduce the general verbosity of the build process to a
> > manageable, semi-informative level. Thus, what currently appears as
> > "gcc" calls, several lines long each is turned into lines like:
> >
> > [M xfs.mod] COMPILE ../src/fs/xfs.c ->
> > xfs_mod-fs_xfs.o
> > [M xfs.mod] LINK xfs_mod-fs_xfs.o ->
> > pre-xfs.o
> > [M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o
> >
> > And so on. The change also makes warning-hunting marginally easier,
> > though not by much since the patch intentionally shows a line for nearly
> > every process that did so previously. This behavior could be simplified
> > further if needed - this post is more of an RFC than anything else.
> > Also, it is by no means thorough or complete - only the most common
> > processes have been addressed - as I'm a bit busy with exams.
> >
> > The patch makes the new behavior the default one, so a new make-time
> > option is added: V (for "verbose"), which must have the value 1 in order
> > to get the behavior, as in "make V=1"
> >
> >
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Build system improvement

[phcoder]

The problem is that build system creates a lot of output on progress and 
so the warnings are often not very visible
Yoshinori K. Okuji wrote:
> On Saturday 11 April 2009 08:25:50 phcoder wrote:
>> I don't see any stoppers to merge this patch
> 
> Personally, I don't like this so much. Is it so useful?
> 
> Regards,
> Okuji
> 
>> Javier Martín wrote:
>>> This patch modifies several files in the build system (mainly common.rmk
>>> and genmk.rb) to reduce the general verbosity of the build process to a
>>> manageable, semi-informative level. Thus, what currently appears as
>>> "gcc" calls, several lines long each is turned into lines like:
>>>
>>> [M xfs.mod] COMPILE ../src/fs/xfs.c ->
>>> xfs_mod-fs_xfs.o
>>> [M xfs.mod] LINK xfs_mod-fs_xfs.o ->
>>> pre-xfs.o
>>> [M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o
>>>
>>> And so on. The change also makes warning-hunting marginally easier,
>>> though not by much since the patch intentionally shows a line for nearly
>>> every process that did so previously. This behavior could be simplified
>>> further if needed - this post is more of an RFC than anything else.
>>> Also, it is by no means thorough or complete - only the most common
>>> processes have been addressed - as I'm a bit busy with exams.
>>>
>>> The patch makes the new behavior the default one, so a new make-time
>>> option is added: V (for "verbose"), which must have the value 1 in order
>>> to get the behavior, as in "make V=1"
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Build system improvement

[Colin D Bennett]

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

Yoshinori K. Okuji wrote on Saturday 11 April 2009:
> On Saturday 11 April 2009 08:25:50 phcoder wrote:
> > I don't see any stoppers to merge this patch
>
> Personally, I don't like this so much. Is it so useful?

Absolutely.  I find it tedious to scroll back through thousands of lines of 
text full of gcc command lines with lots of options etc. to find the actual 
warnings that could indicate (and usually do!) that I made an error in my 
code.  When all the lines generally have a concise and uniform appearance, it 
is visually easy to scan for warnings, compared to now where the lines printed 
are very long and often wrap onto multiple lines, obscuring warnings.

If we could build with -Werror, then it wouldn't be so hard to find the 
warnings since the build would abort...

I think there is only one warning (the “trampoline” warning) that we want to 
ignore.  All others can and should be fixed (and they may be already).

Regards,
Colin


> > Javier Martín wrote:
> > > This patch modifies several files in the build system (mainly
> > > common.rmk and genmk.rb) to reduce the general verbosity of the build
> > > process to a manageable, semi-informative level. Thus, what currently
> > > appears as "gcc" calls, several lines long each is turned into lines
> > > like:
> > >
> > > [M xfs.mod] COMPILE ../src/fs/xfs.c ->
> > > xfs_mod-fs_xfs.o
> > > [M xfs.mod] LINK xfs_mod-fs_xfs.o ->
> > > pre-xfs.o
> > > [M xfs.mod] Looking for EXPORTED SYMBOL definitions: pre-xfs.o
> > >
> > > And so on. The change also makes warning-hunting marginally easier,
> > > though not by much since the patch intentionally shows a line for
> > > nearly every process that did so previously. This behavior could be
> > > simplified further if needed - this post is more of an RFC than
> > > anything else. Also, it is by no means thorough or complete - only the
> > > most common processes have been addressed - as I'm a bit busy with
> > > exams.
> > >
> > > The patch makes the new behavior the default one, so a new make-time
> > > option is added: V (for "verbose"), which must have the value 1 in
> > > order to get the behavior, as in "make V=1"


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Build system improvement

[Pavel Roskin]

On Sat, 2009-04-11 at 08:29 -0700, Colin D Bennett wrote:

> If we could build with -Werror, then it wouldn't be so hard to find the 
> warnings since the build would abort...

It's also possible to redirect stderr to a file so that the build
doesn't stumble on the first warning.

I my opinion, making the build less verbose will increase the
probability that the warnings will be caught and fixed by their makers
rather than by dedicated warning hunters who may not know the code so
well as those who wrote it.  And that's a good thing.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Build system improvement

[David Miller]

From: Pavel Roskin <proski@gnu.org>
Date: Sun, 12 Apr 2009 17:24:49 -0400

> On Sat, 2009-04-11 at 08:29 -0700, Colin D Bennett wrote:
> 
>> If we could build with -Werror, then it wouldn't be so hard to find the 
>> warnings since the build would abort...
> 
> It's also possible to redirect stderr to a file so that the build
> doesn't stumble on the first warning.

I'm iffy about this.

There are some hard warnings to get rid of.

For example when building certain grub-* tools there is no way
to get around the current redefinitions we get of LONG_MAX and
friends.  (one comes in via grub headers, then the stdio.h include
gets us the system definition, we can't use ifdef guards because
the grub headers come in and define things first)

Re: [PATCH] Build system improvement

[Pavel Roskin]

On Sun, 2009-04-12 at 18:07 -0700, David Miller wrote:
> From: Pavel Roskin <proski@gnu.org>
> Date: Sun, 12 Apr 2009 17:24:49 -0400
> 
> > On Sat, 2009-04-11 at 08:29 -0700, Colin D Bennett wrote:
> > 
> >> If we could build with -Werror, then it wouldn't be so hard to find the 
> >> warnings since the build would abort...
> > 
> > It's also possible to redirect stderr to a file so that the build
> > doesn't stumble on the first warning.
> 
> I'm iffy about this.

I meant that "warning hunters" can use it and have a choice what
warnings to fix.  I didn't suggest stderr redirection to be part of the
build system.

> There are some hard warnings to get rid of.
> 
> For example when building certain grub-* tools there is no way
> to get around the current redefinitions we get of LONG_MAX and
> friends.  (one comes in via grub headers, then the stdio.h include
> gets us the system definition, we can't use ifdef guards because
> the grub headers come in and define things first)

I would explore the possibility of introducing GRUB_LONG_MAX.  GRUB
already duplicates a lot of libc definitions.

Sure enough, forcing developers to keep the code warning-free could lead
to ugly fixes that are worse that the original warnings.  However,
keeping the build system quiet will help more people notice the
warnings.  Somebody will come with a nice fix eventually.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Build system improvement

[Yoshinori K. Okuji]

On Monday 13 April 2009 14:03:01 Pavel Roskin wrote:
> On Sun, 2009-04-12 at 18:07 -0700, David Miller wrote:
> > From: Pavel Roskin <proski@gnu.org>
> > Date: Sun, 12 Apr 2009 17:24:49 -0400
> >
> > > On Sat, 2009-04-11 at 08:29 -0700, Colin D Bennett wrote:
> > >> If we could build with -Werror, then it wouldn't be so hard to find
> > >> the warnings since the build would abort...
> > >
> > > It's also possible to redirect stderr to a file so that the build
> > > doesn't stumble on the first warning.
> >
> > I'm iffy about this.
>
> I meant that "warning hunters" can use it and have a choice what
> warnings to fix.  I didn't suggest stderr redirection to be part of the
> build system.
>
> > There are some hard warnings to get rid of.
> >
> > For example when building certain grub-* tools there is no way
> > to get around the current redefinitions we get of LONG_MAX and
> > friends.  (one comes in via grub headers, then the stdio.h include
> > gets us the system definition, we can't use ifdef guards because
> > the grub headers come in and define things first)
>
> I would explore the possibility of introducing GRUB_LONG_MAX.  GRUB
> already duplicates a lot of libc definitions.

Yes. It is bad and dangerous to use the same symbols as libc. I think I have 
written this in the wiki:

http://grub.enbug.org/CodingStyle

Regards,
Okuji

Re: [PATCH] Build system improvement

[Felix Zielcke]

Am Samstag, den 11.04.2009, 19:09 +0900 schrieb Yoshinori K. Okuji:
> On Saturday 11 April 2009 08:25:50 phcoder wrote:
> > I don't see any stoppers to merge this patch
> 
> Personally, I don't like this so much. Is it so useful?
> 

Okuji can we please have this?
Robert brought up again to use -Werror in the `Code quality' topic [0]
and as a start it would be very useful to make the warnings more visible
by default.

[0] http://lists.gnu.org/archive/html/grub-devel/2009-05/msg00009.html
-- 
Felix Zielcke

Re: [PATCH] Build system improvement

[Felix Zielcke]

Am Sonntag, den 03.05.2009, 10:29 +0200 schrieb Felix Zielcke:
> Am Samstag, den 11.04.2009, 19:09 +0900 schrieb Yoshinori K. Okuji:
> > On Saturday 11 April 2009 08:25:50 phcoder wrote:
> > > I don't see any stoppers to merge this patch
> > 
> > Personally, I don't like this so much. Is it so useful?
> > 
> 
> Okuji can we please have this?
> Robert brought up again to use -Werror in the `Code quality' topic [0]
> and as a start it would be very useful to make the warnings more visible
> by default.
> 
> [0] http://lists.gnu.org/archive/html/grub-devel/2009-05/msg00009.html

So what's now with it?
Marco can you at least say something about it?
It would be very nice if the warnings would be more visible by default.
Probable we can't ever use -Werror because different compilers produce
different warnings etc.
For example gentoo's gcc with his trampoline patch.

-- 
Felix Zielcke

Re: [PATCH] Build system improvement

[Pavel Roskin]

On Wed, 2009-07-22 at 14:41 +0200, Felix Zielcke wrote:

> > [0] http://lists.gnu.org/archive/html/grub-devel/2009-05/msg00009.html
> 
> So what's now with it?
> Marco can you at least say something about it?
> It would be very nice if the warnings would be more visible by default.
> Probable we can't ever use -Werror because different compilers produce
> different warnings etc.
> For example gentoo's gcc with his trampoline patch.

I don't see any warnings in any configuration, unless more warning flags
are added (e.g. -Wconversion, which is very noisy).

We can use -Werror by default, but make it very easy to disable.  For
instance, we could have a configure option --disable-strict-warnings.

GRUB is a bootloader; we don't want anyone's system to become unbootable
because something was miscompiled.  Requiring an extra step to disable
warnings would tell users to be more careful.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Build system improvement

[Robert Millan]

On Wed, Jul 22, 2009 at 12:14:54PM -0400, Pavel Roskin wrote:
> On Wed, 2009-07-22 at 14:41 +0200, Felix Zielcke wrote:
> 
> > > [0] http://lists.gnu.org/archive/html/grub-devel/2009-05/msg00009.html
> > 
> > So what's now with it?
> > Marco can you at least say something about it?
> > It would be very nice if the warnings would be more visible by default.
> > Probable we can't ever use -Werror because different compilers produce
> > different warnings etc.
> > For example gentoo's gcc with his trampoline patch.
> 
> I don't see any warnings in any configuration, unless more warning flags
> are added (e.g. -Wconversion, which is very noisy).
> 
> We can use -Werror by default, but make it very easy to disable.  For
> instance, we could have a configure option --disable-strict-warnings.
> 
> GRUB is a bootloader; we don't want anyone's system to become unbootable
> because something was miscompiled.  Requiring an extra step to disable
> warnings would tell users to be more careful.

I completely agree with this (both things).

Except, I'd call it --disable-werror, I think it's clearer (and I vaguely
recall other projects using this flag).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."