[PATCH v2 0/4] Various build and doc fixes

[PATCH v2 0/4] Various build and doc fixes

[Daniel Kiper]

Hey,

As in subject... Please review...

Daniel

 INSTALL      | 51 +++++++++++++++++++++++++++------------------------
 autogen.sh   |  2 +-
 configure.ac | 16 +++++++++++-----
 3 files changed, 39 insertions(+), 30 deletions(-)

Daniel Kiper (4):
      configure: Drop unneeded TARGET_CFLAGS expansion
      configure: Enforce gnu99 C language standard
      INSTALL/configure: Update install doc and configure comment
      autogen: Replace -iname with -ipath in find command

[PATCH v2 1/4] configure: Drop unneeded TARGET_CFLAGS expansion

[Daniel Kiper]

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 88c0adbae..b2576b013 100644
--- a/configure.ac
+++ b/configure.ac
@@ -77,7 +77,7 @@ grub_TRANSFORM([grub-file])
 
 # Optimization flag.  Allow user to override.
 if test "x$TARGET_CFLAGS" = x; then
-  TARGET_CFLAGS="$TARGET_CFLAGS -Os"
+  TARGET_CFLAGS=-Os
 fi
 
 # Default HOST_CPPFLAGS
-- 
2.11.0

[PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Daniel Kiper]

Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
const qualifiers) introduced "restrict" keyword into some functions
definitions. This keyword was introduced in C99 standard. However, some
compilers by default may use C89 or something different. This behavior
leads to the breakage during builds when c89 or gnu89 is in force. So,
let's enforce gnu99 C language standard for all compilers. This way
a bit random build issue will be fixed and the GRUB source will be
build consistently regardless of type and version of the compiler.

It was decided to use gnu99 C language standard because it fixes the
issue mentioned above and also provides some useful extensions which are
used here and there in the GRUB source. Potentially we can use gnu11 too.
However, this may reduce pool of older compilers which can be used to
build the GRUB. So, let's live with gnu99 until we do not discover that
we strongly require a feature from newer C standard.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v2 - suggestions/fixes:
   - unconditionally enforce gnu99 C language standard
     (suggested by Leif Lindholm).
---
 configure.ac | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure.ac b/configure.ac
index b2576b013..fc74ee800 100644
--- a/configure.ac
+++ b/configure.ac
@@ -80,6 +80,10 @@ if test "x$TARGET_CFLAGS" = x; then
   TARGET_CFLAGS=-Os
 fi
 
+BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
+HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
+TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
+
 # Default HOST_CPPFLAGS
 HOST_CPPFLAGS="$HOST_CPPFLAGS -Wall -W"
 HOST_CPPFLAGS="$HOST_CPPFLAGS -DGRUB_UTIL=1"
-- 
2.11.0

[PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Daniel Kiper]

..to reflect the GRUB build reality in them.

Additionally, fix ./configure command example formatting in INSTALL file.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 INSTALL      | 51 +++++++++++++++++++++++++++------------------------
 configure.ac | 10 ++++++----
 2 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/INSTALL b/INSTALL
index 8acb40902..d1b3bb60e 100644
--- a/INSTALL
+++ b/INSTALL
@@ -160,12 +160,12 @@ For this example the configure line might look like (more details below)
 (some options are optional and included here for completeness but some rarely
 used options are omitted):
 
-./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu
-CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config
---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
-TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
-TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
-TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
+./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu \
+  CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config \
+  --target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc \
+  TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6" \
+  TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip" \
+  TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
 
 You need to use following options to specify tools and platforms. For minimum
 version look at prerequisites. All tools not mentioned in this section under
@@ -182,28 +182,31 @@ corresponding platform are not needed for the platform in question.
 
   - For host
     1. --host= to autoconf name of host.
-    2. CC= for gcc able to compile for host
-    3. HOST_CFLAGS= for C options for host.
-    4. HOST_CPPFLAGS= for C preprocessor options for host.
-    5. HOST_LDFLAGS= for linker options for host.
-    6. PKG_CONFIG= for pkg-config for host (optional).
-    7. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
-    8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
-    9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
-    10. Liblzma if any must be in standard linker folders (-llzma) (optional).
+    2. CC= for gcc able to compile for host and target.
+    3. CFLAGS= for C options for host and target.
+    4. HOST_CFLAGS= for C options for host.
+    5. HOST_CPPFLAGS= for C preprocessor options for host.
+    6. HOST_LDFLAGS= for linker options for host.
+    7. PKG_CONFIG= for pkg-config for host (optional).
+    8. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
+    9. Libfuse if any must be in standard linker folders (-lfuse) (optional).
+    10. Libzfs if any must be in standard linker folders (-lzfs) (optional).
+    11. Liblzma if any must be in standard linker folders (-llzma) (optional).
 
   - For target
     1. --target= to autoconf cpu name of target.
     2. --with-platform to choose firmware.
-    3. TARGET_CC= for gcc able to compile for target
-    4. TARGET_CFLAGS= for C options for target.
-    5. TARGET_CPPFLAGS= for C preprocessor options for target.
-    6. TARGET_CCASFLAGS= for assembler options for target.
-    7. TARGET_LDFLAGS= for linker options for target.
-    8. TARGET_OBJCOPY= for objcopy for target.
-    9. TARGET_STRIP= for strip for target.
-    10. TARGET_NM= for nm for target.
-    11. TARGET_RANLIB= for ranlib for target.
+    3. CC= for gcc able to compile for host and target.
+    4. CFLAGS= for C options for host and target.
+    5. TARGET_CC= for gcc able to compile for target.
+    6. TARGET_CFLAGS= for C options for target.
+    7. TARGET_CPPFLAGS= for C preprocessor options for target.
+    8. TARGET_CCASFLAGS= for assembler options for target.
+    9. TARGET_LDFLAGS= for linker options for target.
+    10. TARGET_OBJCOPY= for objcopy for target.
+    11. TARGET_STRIP= for strip for target.
+    12. TARGET_NM= for nm for target.
+    13. TARGET_RANLIB= for ranlib for target.
 
   - Additionally for emu, for host and target.
     1. SDL is looked for in standard linker directories (-lSDL) (optional)
diff --git a/configure.ac b/configure.ac
index fc74ee800..06bc4fb0c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -26,10 +26,12 @@ dnl This is necessary because the target type in autoconf does not
 dnl describe such a system very well.
 dnl
 dnl The current strategy is to use variables with no prefix (such as
-dnl CC, CFLAGS, etc.) for the host type, variables with prefix "BUILD_"
-dnl (such as BUILD_CC, BUILD_CFLAGS, etc.) for the build type and variables
-dnl with the prefix "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are
-dnl used for the target type. See INSTALL for full list of variables.
+dnl CC, CFLAGS, etc.) for the host and target type, variables with
+dnl prefix "BUILD_" (such as BUILD_CC, BUILD_CFLAGS, etc.) for the
+dnl build type, variables with prefix "HOST_" (such as HOST_CC,
+dnl HOST_CFLAGS, etc.) for the host type and variables with the prefix
+dnl "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are used for
+dnl the target type. See INSTALL for full list of variables.
 
 AC_INIT([GRUB],[2.05],[bug-grub@gnu.org])
 
-- 
2.11.0

[PATCH v2 4/4] autogen: Replace -iname with -ipath in find command

[Daniel Kiper]

..because -iname cannot be used to match paths.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 autogen.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/autogen.sh b/autogen.sh
index ef43270fc..31b0ced7e 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -13,7 +13,7 @@ fi
 export LC_COLLATE=C
 unset LC_ALL
 
-find . -iname '*.[ch]' ! -ipath './grub-core/lib/libgcrypt-grub/*' ! -ipath './build-aux/*' ! -ipath './grub-core/lib/libgcrypt/src/misc.c' ! -ipath './grub-core/lib/libgcrypt/src/global.c' ! -ipath './grub-core/lib/libgcrypt/src/secmem.c'  ! -ipath './util/grub-gen-widthspec.c' ! -ipath './util/grub-gen-asciih.c' ! -ipath './gnulib/*' ! -iname './grub-core/lib/gnulib/*' |sort > po/POTFILES.in
+find . -iname '*.[ch]' ! -ipath './grub-core/lib/libgcrypt-grub/*' ! -ipath './build-aux/*' ! -ipath './grub-core/lib/libgcrypt/src/misc.c' ! -ipath './grub-core/lib/libgcrypt/src/global.c' ! -ipath './grub-core/lib/libgcrypt/src/secmem.c'  ! -ipath './util/grub-gen-widthspec.c' ! -ipath './util/grub-gen-asciih.c' ! -ipath './gnulib/*' ! -ipath './grub-core/lib/gnulib/*' |sort > po/POTFILES.in
 find util -iname '*.in' ! -name Makefile.in  |sort > po/POTFILES-shell.in
 
 echo "Importing unicode..."
-- 
2.11.0

Re: [PATCH v2 1/4] configure: Drop unneeded TARGET_CFLAGS expansion

[John Paul Adrian Glaubitz]

On 4/3/20 2:45 PM, Daniel Kiper wrote:
> diff --git a/configure.ac b/configure.ac
> index 88c0adbae..b2576b013 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -77,7 +77,7 @@ grub_TRANSFORM([grub-file])
>  
>  # Optimization flag.  Allow user to override.
>  if test "x$TARGET_CFLAGS" = x; then
> -  TARGET_CFLAGS="$TARGET_CFLAGS -Os"
> +  TARGET_CFLAGS=-Os
>  fi

Good catch. It took me a few seconds to realize that the conditional
is only true and "-Os" added when TARGET_CFLAGS is not set by the
user :).

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH v2 1/4] configure: Drop unneeded TARGET_CFLAGS expansion

[Javier Martinez Canillas]

On 4/3/20 2:45 PM, Daniel Kiper wrote:
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 88c0adbae..b2576b013 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -77,7 +77,7 @@ grub_TRANSFORM([grub-file])
>  
>  # Optimization flag.  Allow user to override.
>  if test "x$TARGET_CFLAGS" = x; then
> -  TARGET_CFLAGS="$TARGET_CFLAGS -Os"
> +  TARGET_CFLAGS=-Os
>  fi
>

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Javier Martinez Canillas]

On 4/3/20 2:45 PM, Daniel Kiper wrote:
> Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> const qualifiers) introduced "restrict" keyword into some functions
> definitions. This keyword was introduced in C99 standard. However, some
> compilers by default may use C89 or something different. This behavior
> leads to the breakage during builds when c89 or gnu89 is in force. So,
> let's enforce gnu99 C language standard for all compilers. This way
> a bit random build issue will be fixed and the GRUB source will be
> build consistently regardless of type and version of the compiler.
> 
> It was decided to use gnu99 C language standard because it fixes the
> issue mentioned above and also provides some useful extensions which are
> used here and there in the GRUB source. Potentially we can use gnu11 too.
> However, this may reduce pool of older compilers which can be used to
> build the GRUB. So, let's live with gnu99 until we do not discover that
> we strongly require a feature from newer C standard.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> v2 - suggestions/fixes:
>    - unconditionally enforce gnu99 C language standard
>      (suggested by Leif Lindholm).
> ---
>  configure.ac | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index b2576b013..fc74ee800 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -80,6 +80,10 @@ if test "x$TARGET_CFLAGS" = x; then
>    TARGET_CFLAGS=-Os
>  fi
> 

I would add a comment here explaining why gnu99 is enforced.
 
> +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
> +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
> +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
> +

Do you want to allow distros to override the -std option or do you want to
always force to use gnu99? If the latter then I think instead it should be:

BUILD_CFLAGS="$BUILD_CFLAGS -std=gnu99"
HOST_CFLAGS="$HOST_CFLAGS -std=gnu99"
TARGET_CFLAGS="$TARGET_CFLAGS -std=gnu99"

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Javier Martinez Canillas]

On 4/3/20 2:45 PM, Daniel Kiper wrote:
> ..to reflect the GRUB build reality in them.
> 
> Additionally, fix ./configure command example formatting in INSTALL file.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  INSTALL      | 51 +++++++++++++++++++++++++++------------------------
>  configure.ac | 10 ++++++----
>  2 files changed, 33 insertions(+), 28 deletions(-)
> 

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2 4/4] autogen: Replace -iname with -ipath in find command

[Javier Martinez Canillas]

On 4/3/20 2:45 PM, Daniel Kiper wrote:
> ..because -iname cannot be used to match paths.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Daniel Kiper]

On Tue, Apr 07, 2020 at 11:38:16AM +0200, Javier Martinez Canillas wrote:
> On 4/3/20 2:45 PM, Daniel Kiper wrote:
> > Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> > const qualifiers) introduced "restrict" keyword into some functions
> > definitions. This keyword was introduced in C99 standard. However, some
> > compilers by default may use C89 or something different. This behavior
> > leads to the breakage during builds when c89 or gnu89 is in force. So,
> > let's enforce gnu99 C language standard for all compilers. This way
> > a bit random build issue will be fixed and the GRUB source will be
> > build consistently regardless of type and version of the compiler.
> >
> > It was decided to use gnu99 C language standard because it fixes the
> > issue mentioned above and also provides some useful extensions which are
> > used here and there in the GRUB source. Potentially we can use gnu11 too.
> > However, this may reduce pool of older compilers which can be used to
> > build the GRUB. So, let's live with gnu99 until we do not discover that
> > we strongly require a feature from newer C standard.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> > v2 - suggestions/fixes:
> >    - unconditionally enforce gnu99 C language standard
> >      (suggested by Leif Lindholm).
> > ---
> >  configure.ac | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/configure.ac b/configure.ac
> > index b2576b013..fc74ee800 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -80,6 +80,10 @@ if test "x$TARGET_CFLAGS" = x; then
> >    TARGET_CFLAGS=-Os
> >  fi
> >
>
> I would add a comment here explaining why gnu99 is enforced.

I am not convinced that we should repeat here what is said in the commit
message...

> > +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
> > +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
> > +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
> > +
>
> Do you want to allow distros to override the -std option or do you want to
> always force to use gnu99? If the latter then I think instead it should be:
>
> BUILD_CFLAGS="$BUILD_CFLAGS -std=gnu99"
> HOST_CFLAGS="$HOST_CFLAGS -std=gnu99"
> TARGET_CFLAGS="$TARGET_CFLAGS -std=gnu99"

I want to allow everybody to override the defaults. In general I think
that we should not impose any artificial limits. If user wants to shoot in
his/her foot he/she should be allowed to do that... In most cases...

Daniel

Re: [PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Leif Lindholm]

On Fri, Apr 03, 2020 at 14:45:01 +0200, Daniel Kiper wrote:
> Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> const qualifiers) introduced "restrict" keyword into some functions
> definitions. This keyword was introduced in C99 standard. However, some
> compilers by default may use C89 or something different. This behavior
> leads to the breakage during builds when c89 or gnu89 is in force. So,
> let's enforce gnu99 C language standard for all compilers. This way
> a bit random build issue will be fixed and the GRUB source will be
> build consistently regardless of type and version of the compiler.
> 
> It was decided to use gnu99 C language standard because it fixes the
> issue mentioned above and also provides some useful extensions which are
> used here and there in the GRUB source. Potentially we can use gnu11 too.
> However, this may reduce pool of older compilers which can be used to
> build the GRUB. So, let's live with gnu99 until we do not discover that

Drop "do not"?

> we strongly require a feature from newer C standard.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> v2 - suggestions/fixes:
>    - unconditionally enforce gnu99 C language standard
>      (suggested by Leif Lindholm).
> ---
>  configure.ac | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index b2576b013..fc74ee800 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -80,6 +80,10 @@ if test "x$TARGET_CFLAGS" = x; then
>    TARGET_CFLAGS=-Os
>  fi
>  
> +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
> +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
> +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"

Agree with Javier that it would be helpful to explain reason for
enforcing gnu99 in a comment.

"// Enable support for 'restrict' keyword"
Would be enough.

/
    Leif

> +
>  # Default HOST_CPPFLAGS
>  HOST_CPPFLAGS="$HOST_CPPFLAGS -Wall -W"
>  HOST_CPPFLAGS="$HOST_CPPFLAGS -DGRUB_UTIL=1"
> -- 
> 2.11.0
> 

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Leif Lindholm]

On Fri, Apr 03, 2020 at 14:45:02 +0200, Daniel Kiper wrote:
> ..to reflect the GRUB build reality in them.
> 
> Additionally, fix ./configure command example formatting in INSTALL file.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  INSTALL      | 51 +++++++++++++++++++++++++++------------------------
>  configure.ac | 10 ++++++----
>  2 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/INSTALL b/INSTALL
> index 8acb40902..d1b3bb60e 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -160,12 +160,12 @@ For this example the configure line might look like (more details below)
>  (some options are optional and included here for completeness but some rarely
>  used options are omitted):
>  
> -./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu
> -CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config
> ---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
> -TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
> -TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
> -TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> +./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu \
> +  CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config \
> +  --target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc \
> +  TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6" \
> +  TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip" \
> +  TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex

So ... probably should have looked more properly at this round 1.

If we are updating this guidance, should we bring it up to date as
well?

Now we have uefi support in u-boot, the "uboot" platform is the
exception rather than the norm. It'd be better to specify
--with-platform=efi.

Secondly, these appear to be flags used when
building GRUB for Raspberry Pi 1; -march-armv6 is quite outdated and
could be dropped. (Although I guess it becomes more relevant when seen
in combination with TARGET_CCASFLAGS.)

Could we add a sentence here going (if switching to efi for the platform):
"Normally, for building a grub on amd64 with tools to run on amd64 to
generate images to run on arm, using your Linux distribution's
packaged cross compiler, the following would suffice:

./configure --target=arm-linux-gnueabihf --with-platform=efi"

>  
>  You need to use following options to specify tools and platforms. For minimum
>  version look at prerequisites. All tools not mentioned in this section under
> @@ -182,28 +182,31 @@ corresponding platform are not needed for the platform in question.
>  
>    - For host
>      1. --host= to autoconf name of host.
> -    2. CC= for gcc able to compile for host
> -    3. HOST_CFLAGS= for C options for host.
> -    4. HOST_CPPFLAGS= for C preprocessor options for host.
> -    5. HOST_LDFLAGS= for linker options for host.
> -    6. PKG_CONFIG= for pkg-config for host (optional).
> -    7. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> -    8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> -    9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> -    10. Liblzma if any must be in standard linker folders (-llzma) (optional).
> +    2. CC= for gcc able to compile for host and target.

But this is incorrect? Apart from where HOST == TARGET? And goes
against the example updated above.
Am I missing something?

> +    3. CFLAGS= for C options for host and target.
> +    4. HOST_CFLAGS= for C options for host.
> +    5. HOST_CPPFLAGS= for C preprocessor options for host.
> +    6. HOST_LDFLAGS= for linker options for host.
> +    7. PKG_CONFIG= for pkg-config for host (optional).
> +    8. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> +    9. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> +    10. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> +    11. Liblzma if any must be in standard linker folders (-llzma) (optional).
>  
>    - For target
>      1. --target= to autoconf cpu name of target.
>      2. --with-platform to choose firmware.
> -    3. TARGET_CC= for gcc able to compile for target
> -    4. TARGET_CFLAGS= for C options for target.
> -    5. TARGET_CPPFLAGS= for C preprocessor options for target.
> -    6. TARGET_CCASFLAGS= for assembler options for target.
> -    7. TARGET_LDFLAGS= for linker options for target.
> -    8. TARGET_OBJCOPY= for objcopy for target.
> -    9. TARGET_STRIP= for strip for target.
> -    10. TARGET_NM= for nm for target.
> -    11. TARGET_RANLIB= for ranlib for target.
> +    3. CC= for gcc able to compile for host and target.

Same again.

> +    4. CFLAGS= for C options for host and target.
> +    5. TARGET_CC= for gcc able to compile for target.
> +    6. TARGET_CFLAGS= for C options for target.
> +    7. TARGET_CPPFLAGS= for C preprocessor options for target.
> +    8. TARGET_CCASFLAGS= for assembler options for target.
> +    9. TARGET_LDFLAGS= for linker options for target.
> +    10. TARGET_OBJCOPY= for objcopy for target.
> +    11. TARGET_STRIP= for strip for target.
> +    12. TARGET_NM= for nm for target.
> +    13. TARGET_RANLIB= for ranlib for target.
>  
>    - Additionally for emu, for host and target.
>      1. SDL is looked for in standard linker directories (-lSDL) (optional)
> diff --git a/configure.ac b/configure.ac
> index fc74ee800..06bc4fb0c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -26,10 +26,12 @@ dnl This is necessary because the target type in autoconf does not
>  dnl describe such a system very well.
>  dnl
>  dnl The current strategy is to use variables with no prefix (such as
> -dnl CC, CFLAGS, etc.) for the host type, variables with prefix "BUILD_"
> -dnl (such as BUILD_CC, BUILD_CFLAGS, etc.) for the build type and variables
> -dnl with the prefix "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are
> -dnl used for the target type. See INSTALL for full list of variables.
> +dnl CC, CFLAGS, etc.) for the host and target type, variables with

Same again.

/
    Leif

> +dnl prefix "BUILD_" (such as BUILD_CC, BUILD_CFLAGS, etc.) for the
> +dnl build type, variables with prefix "HOST_" (such as HOST_CC,
> +dnl HOST_CFLAGS, etc.) for the host type and variables with the prefix
> +dnl "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are used for
> +dnl the target type. See INSTALL for full list of variables.
>  
>  AC_INIT([GRUB],[2.05],[bug-grub@gnu.org])
>  
> -- 
> 2.11.0
> 

Re: [PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Leif Lindholm]

On Tue, Apr 07, 2020 at 12:41:20 +0200, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 11:38:16AM +0200, Javier Martinez Canillas wrote:
> > On 4/3/20 2:45 PM, Daniel Kiper wrote:
> > > Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> > > const qualifiers) introduced "restrict" keyword into some functions
> > > definitions. This keyword was introduced in C99 standard. However, some
> > > compilers by default may use C89 or something different. This behavior
> > > leads to the breakage during builds when c89 or gnu89 is in force. So,
> > > let's enforce gnu99 C language standard for all compilers. This way
> > > a bit random build issue will be fixed and the GRUB source will be
> > > build consistently regardless of type and version of the compiler.
> > >
> > > It was decided to use gnu99 C language standard because it fixes the
> > > issue mentioned above and also provides some useful extensions which are
> > > used here and there in the GRUB source. Potentially we can use gnu11 too.
> > > However, this may reduce pool of older compilers which can be used to
> > > build the GRUB. So, let's live with gnu99 until we do not discover that
> > > we strongly require a feature from newer C standard.
> > >
> > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > > ---
> > > v2 - suggestions/fixes:
> > >    - unconditionally enforce gnu99 C language standard
> > >      (suggested by Leif Lindholm).
> > > ---
> > >  configure.ac | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index b2576b013..fc74ee800 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -80,6 +80,10 @@ if test "x$TARGET_CFLAGS" = x; then
> > >    TARGET_CFLAGS=-Os
> > >  fi
> > >
> >
> > I would add a comment here explaining why gnu99 is enforced.
> 
> I am not convinced that we should repeat here what is said in the commit
> message...
> 
> > > +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
> > > +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
> > > +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
> > > +
> >
> > Do you want to allow distros to override the -std option or do you want to
> > always force to use gnu99? If the latter then I think instead it should be:
> >
> > BUILD_CFLAGS="$BUILD_CFLAGS -std=gnu99"
> > HOST_CFLAGS="$HOST_CFLAGS -std=gnu99"
> > TARGET_CFLAGS="$TARGET_CFLAGS -std=gnu99"
> 
> I want to allow everybody to override the defaults. In general I think
> that we should not impose any artificial limits. If user wants to shoot in
> his/her foot he/she should be allowed to do that... In most cases...

If we were to prevent builders from overriding, we would also prevent
them from overriding with a *higher* version. I think that would be
undesirable.

Whereas setting it by default and letting it be overridden means if it
breaks the build, it's because the builder has requested options no
longer supported by the codebase.

/
    Leif

Re: [PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Javier Martinez Canillas]

On 4/7/20 12:41 PM, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 11:38:16AM +0200, Javier Martinez Canillas wrote:
>> On 4/3/20 2:45 PM, Daniel Kiper wrote:
>>> Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
>>> const qualifiers) introduced "restrict" keyword into some functions
>>> definitions. This keyword was introduced in C99 standard. However, some
>>> compilers by default may use C89 or something different. This behavior
>>> leads to the breakage during builds when c89 or gnu89 is in force. So,
>>> let's enforce gnu99 C language standard for all compilers. This way

I got confused by the intention of this patch since you said enforce
here. Probably it should something like set gnu99 by default and allow
users to override.

[snip]

>>>
>>
>> I would add a comment here explaining why gnu99 is enforced.
> 
> I am not convinced that we should repeat here what is said in the commit
> message...
>

I disagree. If you are reading this file is much easier to read the comment
than having to do a git blame and git show to figure out the rationale of it.

>>> +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
>>> +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
>>> +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
>>> +
>>
>> Do you want to allow distros to override the -std option or do you want to
>> always force to use gnu99? If the latter then I think instead it should be:
>>
>> BUILD_CFLAGS="$BUILD_CFLAGS -std=gnu99"
>> HOST_CFLAGS="$HOST_CFLAGS -std=gnu99"
>> TARGET_CFLAGS="$TARGET_CFLAGS -std=gnu99"
> 
> I want to allow everybody to override the defaults. In general I think
> that we should not impose any artificial limits. If user wants to shoot in
> his/her foot he/she should be allowed to do that... In most cases...
>

Yes, that makes sense to me. Specially since as Leif said one could want to use
a higher version. I just asked since it wasn't clear to me.

The change looks good, so if you re-post feel free to add

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Daniel Kiper]

On Tue, Apr 07, 2020 at 12:16:09PM +0100, Leif Lindholm wrote:
> On Fri, Apr 03, 2020 at 14:45:02 +0200, Daniel Kiper wrote:
> > ..to reflect the GRUB build reality in them.
> >
> > Additionally, fix ./configure command example formatting in INSTALL file.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> >  INSTALL      | 51 +++++++++++++++++++++++++++------------------------
> >  configure.ac | 10 ++++++----
> >  2 files changed, 33 insertions(+), 28 deletions(-)
> >
> > diff --git a/INSTALL b/INSTALL
> > index 8acb40902..d1b3bb60e 100644
> > --- a/INSTALL
> > +++ b/INSTALL
> > @@ -160,12 +160,12 @@ For this example the configure line might look like (more details below)
> >  (some options are optional and included here for completeness but some rarely
> >  used options are omitted):
> >
> > -./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu
> > -CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config
> > ---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
> > -TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
> > -TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
> > -TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> > +./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu \
> > +  CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config \
> > +  --target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc \
> > +  TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6" \
> > +  TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip" \
> > +  TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
>
> So ... probably should have looked more properly at this round 1.
>
> If we are updating this guidance, should we bring it up to date as
> well?
>
> Now we have uefi support in u-boot, the "uboot" platform is the
> exception rather than the norm. It'd be better to specify
> --with-platform=efi.
>
> Secondly, these appear to be flags used when
> building GRUB for Raspberry Pi 1; -march-armv6 is quite outdated and
> could be dropped. (Although I guess it becomes more relevant when seen
> in combination with TARGET_CCASFLAGS.)

What do you think about this:

./configure --host=x86_64-linux-gnu --target=aarch64-linux-gnu \
  --with-platform=efi BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config \
  HOST_CC=x86_64-linux-gnu-gcc HOST_CFLAGS='-g -O2' \
  PKG_CONFIG=x86_64-linux-gnu-pkg-config TARGET_CC=aarch64-linux-gnu-gcc \
  TARGET_CFLAGS='-Os -march=armv8-a' TARGET_CCASFLAGS='-march=armv8-a' \
  TARGET_OBJCOPY=aarch64-linux-gnu-objcopy \
  TARGET_STRIP=aarch64-linux-gnu-strip TARGET_NM=aarch64-linux-gnu-nm \
  TARGET_RANLIB=aarch64-linux-gnu-ranlib LEX=flex

> Could we add a sentence here going (if switching to efi for the platform):
> "Normally, for building a grub on amd64 with tools to run on amd64 to
> generate images to run on arm, using your Linux distribution's
> packaged cross compiler, the following would suffice:
>
> ./configure --target=arm-linux-gnueabihf --with-platform=efi"

./configure --target=aarch64-linux-gnu --with-platform=efi

> >  You need to use following options to specify tools and platforms. For minimum
> >  version look at prerequisites. All tools not mentioned in this section under
> > @@ -182,28 +182,31 @@ corresponding platform are not needed for the platform in question.
> >
> >    - For host
> >      1. --host= to autoconf name of host.
> > -    2. CC= for gcc able to compile for host
> > -    3. HOST_CFLAGS= for C options for host.
> > -    4. HOST_CPPFLAGS= for C preprocessor options for host.
> > -    5. HOST_LDFLAGS= for linker options for host.
> > -    6. PKG_CONFIG= for pkg-config for host (optional).
> > -    7. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> > -    8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> > -    9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> > -    10. Liblzma if any must be in standard linker folders (-llzma) (optional).
> > +    2. CC= for gcc able to compile for host and target.
>
> But this is incorrect? Apart from where HOST == TARGET? And goes
> against the example updated above.
> Am I missing something?

It is correct, CC applies for host and target...

> > +    3. CFLAGS= for C options for host and target.
> > +    4. HOST_CFLAGS= for C options for host.
> > +    5. HOST_CPPFLAGS= for C preprocessor options for host.
> > +    6. HOST_LDFLAGS= for linker options for host.
> > +    7. PKG_CONFIG= for pkg-config for host (optional).
> > +    8. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> > +    9. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> > +    10. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> > +    11. Liblzma if any must be in standard linker folders (-llzma) (optional).
> >
> >    - For target
> >      1. --target= to autoconf cpu name of target.
> >      2. --with-platform to choose firmware.
> > -    3. TARGET_CC= for gcc able to compile for target
> > -    4. TARGET_CFLAGS= for C options for target.
> > -    5. TARGET_CPPFLAGS= for C preprocessor options for target.
> > -    6. TARGET_CCASFLAGS= for assembler options for target.
> > -    7. TARGET_LDFLAGS= for linker options for target.
> > -    8. TARGET_OBJCOPY= for objcopy for target.
> > -    9. TARGET_STRIP= for strip for target.
> > -    10. TARGET_NM= for nm for target.
> > -    11. TARGET_RANLIB= for ranlib for target.
> > +    3. CC= for gcc able to compile for host and target.
>
> Same again.

Ditto...

> > +    4. CFLAGS= for C options for host and target.
> > +    5. TARGET_CC= for gcc able to compile for target.
> > +    6. TARGET_CFLAGS= for C options for target.
> > +    7. TARGET_CPPFLAGS= for C preprocessor options for target.
> > +    8. TARGET_CCASFLAGS= for assembler options for target.
> > +    9. TARGET_LDFLAGS= for linker options for target.
> > +    10. TARGET_OBJCOPY= for objcopy for target.
> > +    11. TARGET_STRIP= for strip for target.
> > +    12. TARGET_NM= for nm for target.
> > +    13. TARGET_RANLIB= for ranlib for target.
> >
> >    - Additionally for emu, for host and target.
> >      1. SDL is looked for in standard linker directories (-lSDL) (optional)
> > diff --git a/configure.ac b/configure.ac
> > index fc74ee800..06bc4fb0c 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -26,10 +26,12 @@ dnl This is necessary because the target type in autoconf does not
> >  dnl describe such a system very well.
> >  dnl
> >  dnl The current strategy is to use variables with no prefix (such as
> > -dnl CC, CFLAGS, etc.) for the host type, variables with prefix "BUILD_"
> > -dnl (such as BUILD_CC, BUILD_CFLAGS, etc.) for the build type and variables
> > -dnl with the prefix "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are
> > -dnl used for the target type. See INSTALL for full list of variables.
> > +dnl CC, CFLAGS, etc.) for the host and target type, variables with
>
> Same again.

...and ditto...

Daniel

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Leif Lindholm]

On Fri, Apr 10, 2020 at 05:05:48 -0700, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 12:16:09PM +0100, Leif Lindholm wrote:
> > On Fri, Apr 03, 2020 at 14:45:02 +0200, Daniel Kiper wrote:
> > > ..to reflect the GRUB build reality in them.
> > >
> > > Additionally, fix ./configure command example formatting in INSTALL file.
> > >
> > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > > ---
> > >  INSTALL      | 51 +++++++++++++++++++++++++++------------------------
> > >  configure.ac | 10 ++++++----
> > >  2 files changed, 33 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/INSTALL b/INSTALL
> > > index 8acb40902..d1b3bb60e 100644
> > > --- a/INSTALL
> > > +++ b/INSTALL
> > > @@ -160,12 +160,12 @@ For this example the configure line might look like (more details below)
> > >  (some options are optional and included here for completeness but some rarely
> > >  used options are omitted):
> > >
> > > -./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu
> > > -CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config
> > > ---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
> > > -TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
> > > -TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
> > > -TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> > > +./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu \
> > > +  CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config \
> > > +  --target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc \
> > > +  TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6" \
> > > +  TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip" \
> > > +  TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> >
> > So ... probably should have looked more properly at this round 1.
> >
> > If we are updating this guidance, should we bring it up to date as
> > well?
> >
> > Now we have uefi support in u-boot, the "uboot" platform is the
> > exception rather than the norm. It'd be better to specify
> > --with-platform=efi.
> >
> > Secondly, these appear to be flags used when
> > building GRUB for Raspberry Pi 1; -march-armv6 is quite outdated and
> > could be dropped. (Although I guess it becomes more relevant when seen
> > in combination with TARGET_CCASFLAGS.)
> 
> What do you think about this:
> 
> ./configure --host=x86_64-linux-gnu --target=aarch64-linux-gnu \
>   --with-platform=efi BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config \
>   HOST_CC=x86_64-linux-gnu-gcc HOST_CFLAGS='-g -O2' \
>   PKG_CONFIG=x86_64-linux-gnu-pkg-config TARGET_CC=aarch64-linux-gnu-gcc \
>   TARGET_CFLAGS='-Os -march=armv8-a' TARGET_CCASFLAGS='-march=armv8-a' \
>   TARGET_OBJCOPY=aarch64-linux-gnu-objcopy \
>   TARGET_STRIP=aarch64-linux-gnu-strip TARGET_NM=aarch64-linux-gnu-nm \
>   TARGET_RANLIB=aarch64-linux-gnu-ranlib LEX=flex

I mean, it's less dated, but it also makes less sense than the
original - an aarch64-linux-gnu-gcc toolchain is very highly likely to
default to -march=armv8-a. It would make more sense to specify
something like -march=armv8.4a.

> > Could we add a sentence here going (if switching to efi for the platform):
> > "Normally, for building a grub on amd64 with tools to run on amd64 to
> > generate images to run on arm, using your Linux distribution's
> > packaged cross compiler, the following would suffice:
> >
> > ./configure --target=arm-linux-gnueabihf --with-platform=efi"
> 
> ./configure --target=aarch64-linux-gnu --with-platform=efi

Again, the original actually made more sense - arm64 only supports efi.

> > >  You need to use following options to specify tools and platforms. For minimum
> > >  version look at prerequisites. All tools not mentioned in this section under
> > > @@ -182,28 +182,31 @@ corresponding platform are not needed for the platform in question.
> > >
> > >    - For host
> > >      1. --host= to autoconf name of host.
> > > -    2. CC= for gcc able to compile for host
> > > -    3. HOST_CFLAGS= for C options for host.
> > > -    4. HOST_CPPFLAGS= for C preprocessor options for host.
> > > -    5. HOST_LDFLAGS= for linker options for host.
> > > -    6. PKG_CONFIG= for pkg-config for host (optional).
> > > -    7. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> > > -    8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> > > -    9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> > > -    10. Liblzma if any must be in standard linker folders (-llzma) (optional).
> > > +    2. CC= for gcc able to compile for host and target.
> >
> > But this is incorrect? Apart from where HOST == TARGET? And goes
> > against the example updated above.
> > Am I missing something?
> 
> It is correct, CC applies for host and target...

So why do we have TARGET_CC and HOST_CC?

My understanding is:
- CC sets the default compilet binary, used for BUILD, HOST and TARGET
  unless overridden for each of these.
- The value of BUILD_CC is used as default for HOST_CC unless
  HOST_CC is explicitly specified.
- The value of HOST_CC is used as the default for TARGET_CC, where *it*
  is not explicitly specified.

Either my understanding here is incorrect, or these changes make the
text more rather than less misleading.

/
    Leif

> > > +    3. CFLAGS= for C options for host and target.
> > > +    4. HOST_CFLAGS= for C options for host.
> > > +    5. HOST_CPPFLAGS= for C preprocessor options for host.
> > > +    6. HOST_LDFLAGS= for linker options for host.
> > > +    7. PKG_CONFIG= for pkg-config for host (optional).
> > > +    8. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> > > +    9. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> > > +    10. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> > > +    11. Liblzma if any must be in standard linker folders (-llzma) (optional).
> > >
> > >    - For target
> > >      1. --target= to autoconf cpu name of target.
> > >      2. --with-platform to choose firmware.
> > > -    3. TARGET_CC= for gcc able to compile for target
> > > -    4. TARGET_CFLAGS= for C options for target.
> > > -    5. TARGET_CPPFLAGS= for C preprocessor options for target.
> > > -    6. TARGET_CCASFLAGS= for assembler options for target.
> > > -    7. TARGET_LDFLAGS= for linker options for target.
> > > -    8. TARGET_OBJCOPY= for objcopy for target.
> > > -    9. TARGET_STRIP= for strip for target.
> > > -    10. TARGET_NM= for nm for target.
> > > -    11. TARGET_RANLIB= for ranlib for target.
> > > +    3. CC= for gcc able to compile for host and target.
> >
> > Same again.
> 
> Ditto...
> 
> > > +    4. CFLAGS= for C options for host and target.
> > > +    5. TARGET_CC= for gcc able to compile for target.
> > > +    6. TARGET_CFLAGS= for C options for target.
> > > +    7. TARGET_CPPFLAGS= for C preprocessor options for target.
> > > +    8. TARGET_CCASFLAGS= for assembler options for target.
> > > +    9. TARGET_LDFLAGS= for linker options for target.
> > > +    10. TARGET_OBJCOPY= for objcopy for target.
> > > +    11. TARGET_STRIP= for strip for target.
> > > +    12. TARGET_NM= for nm for target.
> > > +    13. TARGET_RANLIB= for ranlib for target.
> > >
> > >    - Additionally for emu, for host and target.
> > >      1. SDL is looked for in standard linker directories (-lSDL) (optional)
> > > diff --git a/configure.ac b/configure.ac
> > > index fc74ee800..06bc4fb0c 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -26,10 +26,12 @@ dnl This is necessary because the target type in autoconf does not
> > >  dnl describe such a system very well.
> > >  dnl
> > >  dnl The current strategy is to use variables with no prefix (such as
> > > -dnl CC, CFLAGS, etc.) for the host type, variables with prefix "BUILD_"
> > > -dnl (such as BUILD_CC, BUILD_CFLAGS, etc.) for the build type and variables
> > > -dnl with the prefix "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are
> > > -dnl used for the target type. See INSTALL for full list of variables.
> > > +dnl CC, CFLAGS, etc.) for the host and target type, variables with
> >
> > Same again.
> 
> ...and ditto...
> 
> Daniel

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Daniel Kiper]

On Mon, Apr 20, 2020 at 02:04:32PM +0100, Leif Lindholm wrote:
> On Fri, Apr 10, 2020 at 05:05:48 -0700, Daniel Kiper wrote:
> > On Tue, Apr 07, 2020 at 12:16:09PM +0100, Leif Lindholm wrote:
> > > On Fri, Apr 03, 2020 at 14:45:02 +0200, Daniel Kiper wrote:
> > > > ..to reflect the GRUB build reality in them.
> > > >
> > > > Additionally, fix ./configure command example formatting in INSTALL file.
> > > >
> > > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > > > ---
> > > >  INSTALL      | 51 +++++++++++++++++++++++++++------------------------
> > > >  configure.ac | 10 ++++++----
> > > >  2 files changed, 33 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/INSTALL b/INSTALL
> > > > index 8acb40902..d1b3bb60e 100644
> > > > --- a/INSTALL
> > > > +++ b/INSTALL
> > > > @@ -160,12 +160,12 @@ For this example the configure line might look like (more details below)
> > > >  (some options are optional and included here for completeness but some rarely
> > > >  used options are omitted):
> > > >
> > > > -./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu
> > > > -CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config
> > > > ---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
> > > > -TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
> > > > -TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
> > > > -TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> > > > +./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu \
> > > > +  CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config \
> > > > +  --target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc \
> > > > +  TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6" \
> > > > +  TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip" \
> > > > +  TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> > >
> > > So ... probably should have looked more properly at this round 1.
> > >
> > > If we are updating this guidance, should we bring it up to date as
> > > well?
> > >
> > > Now we have uefi support in u-boot, the "uboot" platform is the
> > > exception rather than the norm. It'd be better to specify
> > > --with-platform=efi.
> > >
> > > Secondly, these appear to be flags used when
> > > building GRUB for Raspberry Pi 1; -march-armv6 is quite outdated and
> > > could be dropped. (Although I guess it becomes more relevant when seen
> > > in combination with TARGET_CCASFLAGS.)
> >
> > What do you think about this:
> >
> > ./configure --host=x86_64-linux-gnu --target=aarch64-linux-gnu \
> >   --with-platform=efi BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config \
> >   HOST_CC=x86_64-linux-gnu-gcc HOST_CFLAGS='-g -O2' \
> >   PKG_CONFIG=x86_64-linux-gnu-pkg-config TARGET_CC=aarch64-linux-gnu-gcc \
> >   TARGET_CFLAGS='-Os -march=armv8-a' TARGET_CCASFLAGS='-march=armv8-a' \
> >   TARGET_OBJCOPY=aarch64-linux-gnu-objcopy \
> >   TARGET_STRIP=aarch64-linux-gnu-strip TARGET_NM=aarch64-linux-gnu-nm \
> >   TARGET_RANLIB=aarch64-linux-gnu-ranlib LEX=flex
>
> I mean, it's less dated, but it also makes less sense than the
> original - an aarch64-linux-gnu-gcc toolchain is very highly likely to
> default to -march=armv8-a. It would make more sense to specify
> something like -march=armv8.4a.

OK, so, I will change -march to armv8.4a.

> > > Could we add a sentence here going (if switching to efi for the platform):
> > > "Normally, for building a grub on amd64 with tools to run on amd64 to
> > > generate images to run on arm, using your Linux distribution's
> > > packaged cross compiler, the following would suffice:
> > >
> > > ./configure --target=arm-linux-gnueabihf --with-platform=efi"
> >
> > ./configure --target=aarch64-linux-gnu --with-platform=efi
>
> Again, the original actually made more sense - arm64 only supports efi.

Yeah, so, let's stick with currently existing example.

> > > >  You need to use following options to specify tools and platforms. For minimum
> > > >  version look at prerequisites. All tools not mentioned in this section under
> > > > @@ -182,28 +182,31 @@ corresponding platform are not needed for the platform in question.
> > > >
> > > >    - For host
> > > >      1. --host= to autoconf name of host.
> > > > -    2. CC= for gcc able to compile for host
> > > > -    3. HOST_CFLAGS= for C options for host.
> > > > -    4. HOST_CPPFLAGS= for C preprocessor options for host.
> > > > -    5. HOST_LDFLAGS= for linker options for host.
> > > > -    6. PKG_CONFIG= for pkg-config for host (optional).
> > > > -    7. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> > > > -    8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> > > > -    9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> > > > -    10. Liblzma if any must be in standard linker folders (-llzma) (optional).
> > > > +    2. CC= for gcc able to compile for host and target.
> > >
> > > But this is incorrect? Apart from where HOST == TARGET? And goes
> > > against the example updated above.
> > > Am I missing something?
> >
> > It is correct, CC applies for host and target...
>
> So why do we have TARGET_CC and HOST_CC?
>
> My understanding is:
> - CC sets the default compilet binary, used for BUILD, HOST and TARGET
>   unless overridden for each of these.

Nope, just HOST_CC and TARGET_CC. You can override CC using TARGET_CC and
HOST_CC respectively. Order in configure command line does not matter.

> - The value of BUILD_CC is used as default for HOST_CC unless
>   HOST_CC is explicitly specified.

Nope, it is not.

> - The value of HOST_CC is used as the default for TARGET_CC, where *it*
>   is not explicitly specified.

Nope, it is not.

> Either my understanding here is incorrect, or these changes make the
> text more rather than less misleading.

Sorry but I am not sure what is unclear after my changes.

Daniel

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Leif Lindholm]

On Tue, Apr 21, 2020 at 16:55:25 +0200, Daniel Kiper wrote:
> > > > >  You need to use following options to specify tools and platforms. For minimum
> > > > >  version look at prerequisites. All tools not mentioned in this section under
> > > > > @@ -182,28 +182,31 @@ corresponding platform are not needed for the platform in question.
> > > > >
> > > > >    - For host
> > > > >      1. --host= to autoconf name of host.
> > > > > -    2. CC= for gcc able to compile for host
> > > > > -    3. HOST_CFLAGS= for C options for host.
> > > > > -    4. HOST_CPPFLAGS= for C preprocessor options for host.
> > > > > -    5. HOST_LDFLAGS= for linker options for host.
> > > > > -    6. PKG_CONFIG= for pkg-config for host (optional).
> > > > > -    7. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
> > > > > -    8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> > > > > -    9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> > > > > -    10. Liblzma if any must be in standard linker folders (-llzma) (optional).
> > > > > +    2. CC= for gcc able to compile for host and target.
> > > >
> > > > But this is incorrect? Apart from where HOST == TARGET? And goes
> > > > against the example updated above.
> > > > Am I missing something?
> > >
> > > It is correct, CC applies for host and target...
> >
> > So why do we have TARGET_CC and HOST_CC?
> >
> > My understanding is:
> > - CC sets the default compilet binary, used for BUILD, HOST and TARGET
> >   unless overridden for each of these.
> 
> Nope, just HOST_CC and TARGET_CC. You can override CC using TARGET_CC and
> HOST_CC respectively. Order in configure command line does not matter.

I wasn't suggesting command line order mattered.

> > - The value of BUILD_CC is used as default for HOST_CC unless
> >   HOST_CC is explicitly specified.
> 
> Nope, it is not.
> 
> > - The value of HOST_CC is used as the default for TARGET_CC, where *it*
> >   is not explicitly specified.
> 
> Nope, it is not.
> 
> > Either my understanding here is incorrect, or these changes make the
> > text more rather than less misleading.
> 
> Sorry but I am not sure what is unclear after my changes.

OK, I concede configure.ac completely ignores the value of CC for
setting BUILD_CC. It tests for a hard-wired set of commands (which
seems weird to me, but some googling suggests that is indeed default
behaviour - I guess overriding the BUILD_ toolchain is the *least*
common use case).

But let me rewind:

configure scripts support separate concepts of BUILD, HOST and TARGET
to permit things like cross-compiling a tool on an AMD64 box (BUILD) to
run on an armhf box (HOST) to generate images to run on SPARC
(TARGET).

CC is traditionally used to override just using the first "gcc"
command to find a (apparently HOST_) toolchain.

But if I set both HOST_ and TARGET_ options for my build, I expect the
target to be built using the TARGET_ toolchain. If I specify CC and
TARGET_CC, I expect the target to be built with TARGET_CC. Which is
thankfully also what happens in reality.

However, the proposed change now says that HOST_CC sets the host and
target compiler and that TARGET_CC sets the target compiler. This is
not even self-consistent.

I get what the change is trying to say, but I feel it is only adding
confusion. I think the same thing could be adequately conveyed by
adding something along the lines of:
"If no target options are given, these will default to be the same as
the host options."
immediately below the - "For target" line

I don't think adding description of CC/CFLAGS to target section
improves clarity. If CFLAGS should be separately speficied, I think
that makes more sense next to HOST_CFLAGS.

And I think if we're changing this, we should add an entry for HOST_CC
- or add that to the existing CC option.

Regards,

Leif