[PATCH 0/3] binman: Make tests work on non-x86 architectures via cross-compilation

[PATCH 0/3] binman: Make tests work on non-x86 architectures via cross-compilation

[Alper Nebi Yasak]

Right now the 'binman test' command fails spectacularly on arm64 since
it cannot even setup the test environments properly due to errors during
setUpClass(). I can get a 100% coverage result with all tests passing if
I cross-compile things to x86 and run the cross-compiling versions of
some tools. This series tries to implement that solution in a general
way.

I think the thoroughly proper thing would be to make the tests and their
files portable. I don't really know how. Another alternative is to split
the test environments into multiple parts and skip the parts that can't
be prepared for an architecture, but that affects the test coverage
results. In any case, this series doesn't really prevent these other
solutions from being implemented on top of it.


Alper Nebi Yasak (3):
  binman: Support cross-compiling test files to x86
  binman: Use target-specific tools when cross-compiling
  binman: Allow resolving host-specific tools from env vars

 tools/binman/elf.py        |  6 ++-
 tools/binman/elf_test.py   |  4 +-
 tools/binman/test/Makefile | 28 +++++++++++-
 tools/dtoc/fdt_util.py     |  9 ++--
 tools/patman/tools.py      | 88 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 127 insertions(+), 8 deletions(-)

-- 
2.28.0

[PATCH 1/3] binman: Support cross-compiling test files to x86

[Alper Nebi Yasak]

These test files are currently "intended for use on x86 hosts", but most
of the tests using them can still pass when cross-compiled to x86 on an
arm64 host.

This patch enables non-x86 hosts to run the tests by specifying a
cross-compiler via CROSS_COMPILE. The list of variables it sets is taken
from the top-level Makefile. It would be possible to automatically set
an x86 cross-compiler with a few blocks like:

    ifneq ($(shell i386-linux-gnu-gcc --version 2> /dev/null),)
    CROSS_COMPILE = i386-linux-gnu-
    endif

But it wouldn't propagate to the binman process calling this Makefile,
so it's better just raise an error and expect 'binman test' to be run
with a correct CROSS_COMPILE.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/test/Makefile | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/binman/test/Makefile b/tools/binman/test/Makefile
index e4fd97bb2e..e10a8625db 100644
--- a/tools/binman/test/Makefile
+++ b/tools/binman/test/Makefile
@@ -7,6 +7,32 @@
 # SPDX-License-Identifier:      GPL-2.0+
 #
 
+HOST_ARCH := $(shell uname -m | sed -e s/i.86/x86/ )
+ifeq ($(findstring $(HOSTARCH),"x86" "x86_64"),)
+ifeq ($(findstring $(MAKECMDGOALS),"help" "clean"),)
+ifndef CROSS_COMPILE
+$(error Binman tests need to compile to x86, but the CPU arch of your \
+	machine is $(HOST_ARCH). Set CROSS_COMPILE to a suitable cross compiler)
+endif
+endif
+endif
+
+AS		= $(CROSS_COMPILE)as
+# Always use GNU ld
+ifneq ($(shell $(CROSS_COMPILE)ld.bfd -v 2> /dev/null),)
+LD		= $(CROSS_COMPILE)ld.bfd
+else
+LD		= $(CROSS_COMPILE)ld
+endif
+CC		= $(CROSS_COMPILE)gcc
+CPP		= $(CC) -E
+AR		= $(CROSS_COMPILE)ar
+NM		= $(CROSS_COMPILE)nm
+LDR		= $(CROSS_COMPILE)ldr
+STRIP		= $(CROSS_COMPILE)strip
+OBJCOPY		= $(CROSS_COMPILE)objcopy
+OBJDUMP		= $(CROSS_COMPILE)objdump
+
 VPATH := $(SRC)
 CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include \
 	-Wl,--no-dynamic-linker
@@ -32,7 +58,7 @@ bss_data: CFLAGS += $(SRC)bss_data.lds
 bss_data: bss_data.c
 
 u_boot_binman_syms.bin: u_boot_binman_syms
-	objcopy -O binary $< -R .note.gnu.build-id $@
+	$(OBJCOPY) -O binary $< -R .note.gnu.build-id $@
 
 u_boot_binman_syms: CFLAGS += $(LDS_BINMAN)
 u_boot_binman_syms: u_boot_binman_syms.c
-- 
2.28.0

[PATCH 2/3] binman: Use target-specific tools when cross-compiling

[Alper Nebi Yasak]

Currently, binman always runs the compile tools like cc, objcopy, strip,
etc. using their literal name. Instead, this patch makes it use the
target-specific versions by default, derived from the tool-specific
environment variables (CC, OBJCOPY, STRIP, etc.) or from the
CROSS_COMPILE environment variable.

For example, the u-boot-elf etype directly uses 'strip'. Trying to run
the tests with 'CROSS_COMPILE=i686-linux-gnu- binman test' on an arm64
host results in the '097_elf_strip.dts' test to fail as the arm64
version of 'strip' can't understand the format of the x86 ELF file.

This also adjusts some command.Output() calls that caused test errors or
failures to use the target versions of the tools they call. After this,
patch, an arm64 host can run all tests with no errors or failures using
a correct CROSS_COMPILE value.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/elf.py      |  6 +++--
 tools/binman/elf_test.py |  4 ++-
 tools/dtoc/fdt_util.py   |  9 ++++---
 tools/patman/tools.py    | 58 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/tools/binman/elf.py b/tools/binman/elf.py
index f88031c2bf..5e566e56cb 100644
--- a/tools/binman/elf.py
+++ b/tools/binman/elf.py
@@ -234,8 +234,10 @@ SECTIONS
     #   text section at the start
     # -m32: Build for 32-bit x86
     # -T...: Specifies the link script, which sets the start address
-    stdout = command.Output('cc', '-static', '-nostdlib', '-Wl,--build-id=none',
-                            '-m32','-T', lds_file, '-o', elf_fname, s_file)
+    cc, args = tools.GetTargetCompileTool('cc')
+    args += ['-static', '-nostdlib', '-Wl,--build-id=none', '-m32', '-T',
+            lds_file, '-o', elf_fname, s_file]
+    stdout = command.Output(cc, *args)
     shutil.rmtree(outdir)
 
 def DecodeElf(data, location):
diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
index 37e1b423cf..e3d218a89e 100644
--- a/tools/binman/elf_test.py
+++ b/tools/binman/elf_test.py
@@ -186,7 +186,9 @@ class TestElf(unittest.TestCase):
         # Make an Elf file and then convert it to a fkat binary file. This
         # should produce the original data.
         elf.MakeElf(elf_fname, expected_text, expected_data)
-        stdout = command.Output('objcopy', '-O', 'binary', elf_fname, bin_fname)
+        objcopy, args = tools.GetTargetCompileTool('objcopy')
+        args += ['-O', 'binary', elf_fname, bin_fname]
+        stdout = command.Output(objcopy, *args)
         with open(bin_fname, 'rb') as fd:
             data = fd.read()
         self.assertEqual(expected_text + expected_data, data)
diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
index b040793772..37e96b9864 100644
--- a/tools/dtoc/fdt_util.py
+++ b/tools/dtoc/fdt_util.py
@@ -68,22 +68,23 @@ def EnsureCompiled(fname, tmpdir=None, capture_stderr=False):
 
     search_paths = [os.path.join(os.getcwd(), 'include')]
     root, _ = os.path.splitext(fname)
-    args = ['-E', '-P', '-x', 'assembler-with-cpp', '-D__ASSEMBLY__']
+    cc, args = tools.GetTargetCompileTool('cc')
+    args += ['-E', '-P', '-x', 'assembler-with-cpp', '-D__ASSEMBLY__']
     args += ['-Ulinux']
     for path in search_paths:
         args.extend(['-I', path])
     args += ['-o', dts_input, fname]
-    command.Run('cc', *args)
+    command.Run(cc, *args)
 
     # If we don't have a directory, put it in the tools tempdir
     search_list = []
     for path in search_paths:
         search_list.extend(['-i', path])
-    args = ['-I', 'dts', '-o', dtb_output, '-O', 'dtb',
+    dtc, args = tools.GetTargetCompileTool('dtc')
+    args += ['-I', 'dts', '-o', dtb_output, '-O', 'dtb',
             '-W', 'no-unit_address_vs_reg']
     args.extend(search_list)
     args.append(dts_input)
-    dtc = os.environ.get('DTC') or 'dtc'
     command.Run(dtc, *args, capture_stderr=capture_stderr)
     return dtb_output
 
diff --git a/tools/patman/tools.py b/tools/patman/tools.py
index d41115a22c..ee8b70d0cc 100644
--- a/tools/patman/tools.py
+++ b/tools/patman/tools.py
@@ -188,6 +188,58 @@ def PathHasFile(path_spec, fname):
             return True
     return False
 
+def GetTargetCompileTool(name, cross_compile=None):
+    """Get the target-specific version for a compile tool
+
+    This first checks the environment variables that specify which
+    version of the tool should be used (e,g, ${CC}). If those aren't
+    specified, it checks the CROSS_COMPILE variable as a prefix for the
+    tool with some substitutions (e.g. "${CROSS_COMPILE}gcc" for cc).
+
+    Args:
+        name: Command name to run
+
+    Returns:
+        target_name: Exact command name to run instead
+        extra_args: List of extra arguments to pass
+    """
+    env = dict(os.environ)
+
+    target_name = None
+    extra_args = []
+    if name in ('as', 'ld', 'cc', 'cpp', 'ar', 'nm', 'ldr', 'strip',
+                'objcopy', 'objdump', 'dtc'):
+        target_name, *extra_args = env.get(name.upper(), '').split(' ')
+    elif name == 'c++':
+        target_name, *extra_args = env.get('CXX', '').split(' ')
+
+    if target_name:
+        return target_name, extra_args
+
+    if cross_compile is None:
+        cross_compile = env.get('CROSS_COMPILE', '')
+    if not cross_compile:
+        return name, []
+
+    if name in ('as', 'ar', 'nm', 'ldr', 'strip', 'objcopy', 'objdump'):
+        target_name = cross_compile + name
+    elif name == 'ld':
+        try:
+            if Run(cross_compile + 'ld.bfd', '-v'):
+                target_name = cross_compile + 'ld.bfd'
+        except:
+            target_name = cross_compile + 'ld'
+    elif name == 'cc':
+        target_name = cross_compile + 'gcc'
+    elif name == 'cpp':
+        target_name = cross_compile + 'gcc'
+        extra_args = ['-E']
+    elif name == 'c++':
+        target_name = cross_compile + 'g++'
+    else:
+        target_name = name
+    return target_name, extra_args
+
 def Run(name, *args, **kwargs):
     """Run a tool with some arguments
 
@@ -198,16 +250,22 @@ def Run(name, *args, **kwargs):
     Args:
         name: Command name to run
         args: Arguments to the tool
+        for_target: False to run the command as-is, without resolving it
+                   to the version for the compile target
 
     Returns:
         CommandResult object
     """
     try:
         binary = kwargs.get('binary')
+        for_target = kwargs.get('for_target', True)
         env = None
         if tool_search_paths:
             env = dict(os.environ)
             env['PATH'] = ':'.join(tool_search_paths) + ':' + env['PATH']
+        if for_target:
+            name, extra_args = GetTargetCompileTool(name)
+            args = tuple(extra_args) + args
         all_args = (name,) + args
         result = command.RunPipe([all_args], capture=True, capture_stderr=True,
                                  env=env, raise_on_error=False, binary=binary)
-- 
2.28.0

[PATCH 3/3] binman: Allow resolving host-specific tools from env vars

[Alper Nebi Yasak]

This patch lets tools.Run() use host-specific versions with the
for_host keyword argument, based on the host-specific environment
variables (HOSTCC, HOSTOBJCOPY, HOSTSTRIP, etc.).

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---
Not sure if this patch will ever be useful, but it complements the
previous patch very well.

 tools/patman/tools.py | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/patman/tools.py b/tools/patman/tools.py
index ee8b70d0cc..6d539fe594 100644
--- a/tools/patman/tools.py
+++ b/tools/patman/tools.py
@@ -188,6 +188,31 @@ def PathHasFile(path_spec, fname):
             return True
     return False
 
+def GetHostCompileTool(name):
+    """Get the host-specific version for a compile tool
+
+    This checks the environment variables that specify which version of
+    the tool should be used.
+
+    Args:
+        name: Command name to run
+
+    Returns:
+        host_name: Exact command name to run instead
+        extra_args: List of extra arguments to pass
+    """
+    host_name = None
+    extra_args = []
+    if name in ('as', 'ld', 'cc', 'cpp', 'ar', 'nm', 'ldr', 'strip',
+                'objcopy', 'objdump', 'dtc'):
+        host_name, *host_args = env.get('HOST' + name.upper(), '').split(' ')
+    elif name == 'c++':
+        host_name, *host_args = env.get('HOSTCXX', '').split(' ')
+
+    if host_name:
+        return host_name, extra_args
+    return name, []
+
 def GetTargetCompileTool(name, cross_compile=None):
     """Get the target-specific version for a compile tool
 
@@ -250,6 +275,7 @@ def Run(name, *args, **kwargs):
     Args:
         name: Command name to run
         args: Arguments to the tool
+        for_host: True to resolve the command to the version for the host
         for_target: False to run the command as-is, without resolving it
                    to the version for the compile target
 
@@ -258,7 +284,8 @@ def Run(name, *args, **kwargs):
     """
     try:
         binary = kwargs.get('binary')
-        for_target = kwargs.get('for_target', True)
+        for_host = kwargs.get('for_host', False)
+        for_target = kwargs.get('for_target', not for_host)
         env = None
         if tool_search_paths:
             env = dict(os.environ)
@@ -266,6 +293,9 @@ def Run(name, *args, **kwargs):
         if for_target:
             name, extra_args = GetTargetCompileTool(name)
             args = tuple(extra_args) + args
+        elif for_host:
+            name, extra_args = GetHostCompileTool(name)
+            args = tuple(extra_args) + args
         all_args = (name,) + args
         result = command.RunPipe([all_args], capture=True, capture_stderr=True,
                                  env=env, raise_on_error=False, binary=binary)
-- 
2.28.0

[PATCH 1/3] binman: Support cross-compiling test files to x86

[Simon Glass]

Hi Alper,

On Sat, 5 Sep 2020 at 08:44, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> These test files are currently "intended for use on x86 hosts", but most
> of the tests using them can still pass when cross-compiled to x86 on an
> arm64 host.
>
> This patch enables non-x86 hosts to run the tests by specifying a
> cross-compiler via CROSS_COMPILE. The list of variables it sets is taken
> from the top-level Makefile. It would be possible to automatically set
> an x86 cross-compiler with a few blocks like:
>
>     ifneq ($(shell i386-linux-gnu-gcc --version 2> /dev/null),)
>     CROSS_COMPILE = i386-linux-gnu-
>     endif
>
> But it wouldn't propagate to the binman process calling this Makefile,
> so it's better just raise an error and expect 'binman test' to be run
> with a correct CROSS_COMPILE.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/test/Makefile | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)

For me this fails on x86_64, complaining for example:

Exception: Error 2 running 'make -C /tmp/binmant.d17vfu3j/elftest -f
/scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/Makefile
SRC=/scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/':
/scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/Makefile:14:
*** Binman tests need to compile to x86, but the CPU arch of your
machine is x86_64. Set CROSS_COMPILE to a suitable cross compiler.
Stop.

Can you make it work on both i386 and x86_64 without complaining? It
looks like that is the intent.

Also I'm not sure we need to define vars for all the tools, so you
could perhaps drop those that are not needed.

Regards,
Simon

[PATCH 2/3] binman: Use target-specific tools when cross-compiling

[Simon Glass]

Hi Alper,

On Sat, 5 Sep 2020 at 08:44, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Currently, binman always runs the compile tools like cc, objcopy, strip,
> etc. using their literal name. Instead, this patch makes it use the
> target-specific versions by default, derived from the tool-specific
> environment variables (CC, OBJCOPY, STRIP, etc.) or from the
> CROSS_COMPILE environment variable.
>
> For example, the u-boot-elf etype directly uses 'strip'. Trying to run
> the tests with 'CROSS_COMPILE=i686-linux-gnu- binman test' on an arm64
> host results in the '097_elf_strip.dts' test to fail as the arm64
> version of 'strip' can't understand the format of the x86 ELF file.
>
> This also adjusts some command.Output() calls that caused test errors or
> failures to use the target versions of the tools they call. After this,
> patch, an arm64 host can run all tests with no errors or failures using
> a correct CROSS_COMPILE value.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/elf.py      |  6 +++--
>  tools/binman/elf_test.py |  4 ++-
>  tools/dtoc/fdt_util.py   |  9 ++++---
>  tools/patman/tools.py    | 58 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 70 insertions(+), 7 deletions(-)

This looks good, but it drops the use of DTC to specify the
device-tree compiler. Can you add it back?

Regards,
Simon

[PATCH 3/3] binman: Allow resolving host-specific tools from env vars

[Simon Glass]

On Sat, 5 Sep 2020 at 08:44, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> This patch lets tools.Run() use host-specific versions with the
> for_host keyword argument, based on the host-specific environment
> variables (HOSTCC, HOSTOBJCOPY, HOSTSTRIP, etc.).
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
> Not sure if this patch will ever be useful, but it complements the
> previous patch very well.

Yes, agreed.

>
>  tools/patman/tools.py | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below.

Also please add a mention of the CROSS_COMPILE thing in binman's README.

>
> diff --git a/tools/patman/tools.py b/tools/patman/tools.py
> index ee8b70d0cc..6d539fe594 100644
> --- a/tools/patman/tools.py
> +++ b/tools/patman/tools.py
> @@ -188,6 +188,31 @@ def PathHasFile(path_spec, fname):
>              return True
>      return False
>
> +def GetHostCompileTool(name):
> +    """Get the host-specific version for a compile tool
> +
> +    This checks the environment variables that specify which version of
> +    the tool should be used.

Can you please expand the comment to mention the environment variables
it checks?


> +
> +    Args:
> +        name: Command name to run
> +
> +    Returns:
> +        host_name: Exact command name to run instead
> +        extra_args: List of extra arguments to pass
> +    """
> +    host_name = None
> +    extra_args = []
> +    if name in ('as', 'ld', 'cc', 'cpp', 'ar', 'nm', 'ldr', 'strip',
> +                'objcopy', 'objdump', 'dtc'):
> +        host_name, *host_args = env.get('HOST' + name.upper(), '').split(' ')
> +    elif name == 'c++':
> +        host_name, *host_args = env.get('HOSTCXX', '').split(' ')
> +
> +    if host_name:
> +        return host_name, extra_args
> +    return name, []
> +
>  def GetTargetCompileTool(name, cross_compile=None):
>      """Get the target-specific version for a compile tool
>
> @@ -250,6 +275,7 @@ def Run(name, *args, **kwargs):
>      Args:
>          name: Command name to run
>          args: Arguments to the tool
> +        for_host: True to resolve the command to the version for the host
>          for_target: False to run the command as-is, without resolving it
>                     to the version for the compile target
>
> @@ -258,7 +284,8 @@ def Run(name, *args, **kwargs):
>      """
>      try:
>          binary = kwargs.get('binary')
> -        for_target = kwargs.get('for_target', True)
> +        for_host = kwargs.get('for_host', False)
> +        for_target = kwargs.get('for_target', not for_host)
>          env = None
>          if tool_search_paths:
>              env = dict(os.environ)
> @@ -266,6 +293,9 @@ def Run(name, *args, **kwargs):
>          if for_target:
>              name, extra_args = GetTargetCompileTool(name)
>              args = tuple(extra_args) + args
> +        elif for_host:
> +            name, extra_args = GetHostCompileTool(name)
> +            args = tuple(extra_args) + args
>          all_args = (name,) + args
>          result = command.RunPipe([all_args], capture=True, capture_stderr=True,
>                                   env=env, raise_on_error=False, binary=binary)
> --
> 2.28.0
>

[PATCH 1/3] binman: Support cross-compiling test files to x86

[Alper Nebi Yasak]

On 05/09/2020 19:36, Simon Glass wrote:
> For me this fails on x86_64, complaining for example:
> 
> Exception: Error 2 running 'make -C /tmp/binmant.d17vfu3j/elftest -f
> /scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/Makefile
> SRC=/scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/':
> /scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/Makefile:14:
> *** Binman tests need to compile to x86, but the CPU arch of your
> machine is x86_64. Set CROSS_COMPILE to a suitable cross compiler.
> Stop.
> 
> Can you make it work on both i386 and x86_64 without complaining? It
> looks like that is the intent.

I messed up the variable names: defined HOST_ARCH, but used HOSTARCH in
the check. Fixing that makes it work like it should. (I'll go with
HOSTARCH since that's where I took the "uname -m | sed" call from).

> Also I'm not sure we need to define vars for all the tools, so you
> could perhaps drop those that are not needed.

Looks like we don't need anything except CC and OBJCOPY, I'll drop the
rest.

[PATCH 2/3] binman: Use target-specific tools when cross-compiling

[Alper Nebi Yasak]

On 05/09/2020 19:37, Simon Glass wrote:
> This looks good, but it drops the use of DTC to specify the
> device-tree compiler. Can you add it back?

I think you're referring to this hunk:

>      # If we don't have a directory, put it in the tools tempdir
>      search_list = []
>      for path in search_paths:
>          search_list.extend(['-i', path])
> -    args = ['-I', 'dts', '-o', dtb_output, '-O', 'dtb',
> +    dtc, args = tools.GetTargetCompileTool('dtc')
> +    args += ['-I', 'dts', '-o', dtb_output, '-O', 'dtb',
>              '-W', 'no-unit_address_vs_reg']
>      args.extend(search_list)
>      args.append(dts_input)
> -    dtc = os.environ.get('DTC') or 'dtc'
>      command.Run(dtc, *args, capture_stderr=capture_stderr)
>      return dtb_output

where I removed the os.environ.get('DTC'). Instead of that, I get the
command for dtc from GetTargetCompileTool('dtc'), which does check the
'DTC' environment variable:

> +def GetTargetCompileTool(name, cross_compile=None):
>      [...]
> +    env = dict(os.environ)
> +
> +    target_name = None
> +    extra_args = []
> +    if name in ('as', 'ld', 'cc', 'cpp', 'ar', 'nm', 'ldr', 'strip',
> +                'objcopy', 'objdump', 'dtc'):
> +        target_name, *extra_args = env.get(name.upper(), '').split(' ')
> +
> +    if target_name:
> +        return target_name, extra_args

If that's not convincing enough: running 'DTC=false binman test' gets me
a lot of test errors that I don't get without the 'DTC=false'.

[PATCH 3/3] binman: Allow resolving host-specific tools from env vars

[Alper Nebi Yasak]

On 05/09/2020 19:37, Simon Glass wrote:
> Please see below.
> 
> Also please add a mention of the CROSS_COMPILE thing in binman's README.
> 
>> +def GetHostCompileTool(name):
>> +    """Get the host-specific version for a compile tool
>> +
>> +    This checks the environment variables that specify which version of
>> +    the tool should be used.
> 
> Can you please expand the comment to mention the environment variables
> it checks?

OK, will send a v2 after doing these with the fixes for the first patch
(unless you point out until then something new that I should fix).

[PATCH 2/3] binman: Use target-specific tools when cross-compiling

[Simon Glass]

Hi Alper,

On Sat, 5 Sep 2020 at 17:04, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 05/09/2020 19:37, Simon Glass wrote:
> > This looks good, but it drops the use of DTC to specify the
> > device-tree compiler. Can you add it back?
>
> I think you're referring to this hunk:
>
> >      # If we don't have a directory, put it in the tools tempdir
> >      search_list = []
> >      for path in search_paths:
> >          search_list.extend(['-i', path])
> > -    args = ['-I', 'dts', '-o', dtb_output, '-O', 'dtb',
> > +    dtc, args = tools.GetTargetCompileTool('dtc')
> > +    args += ['-I', 'dts', '-o', dtb_output, '-O', 'dtb',
> >              '-W', 'no-unit_address_vs_reg']
> >      args.extend(search_list)
> >      args.append(dts_input)
> > -    dtc = os.environ.get('DTC') or 'dtc'
> >      command.Run(dtc, *args, capture_stderr=capture_stderr)
> >      return dtb_output
>
> where I removed the os.environ.get('DTC'). Instead of that, I get the
> command for dtc from GetTargetCompileTool('dtc'), which does check the
> 'DTC' environment variable:
>
> > +def GetTargetCompileTool(name, cross_compile=None):
> >      [...]
> > +    env = dict(os.environ)
> > +
> > +    target_name = None
> > +    extra_args = []
> > +    if name in ('as', 'ld', 'cc', 'cpp', 'ar', 'nm', 'ldr', 'strip',
> > +                'objcopy', 'objdump', 'dtc'):
> > +        target_name, *extra_args = env.get(name.upper(), '').split(' ')
> > +
> > +    if target_name:
> > +        return target_name, extra_args
>
> If that's not convincing enough: running 'DTC=false binman test' gets me
> a lot of test errors that I don't get without the 'DTC=false'.

OK I did look at that function thinking you might have done that, but
was expecting the same code...so this seems OK to me.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[PATCH 1/3] binman: Support cross-compiling test files to x86

[Simon Glass]

Hi Alper,

On Sat, 5 Sep 2020 at 16:51, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 05/09/2020 19:36, Simon Glass wrote:
> > For me this fails on x86_64, complaining for example:
> >
> > Exception: Error 2 running 'make -C /tmp/binmant.d17vfu3j/elftest -f
> > /scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/Makefile
> > SRC=/scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/':
> > /scratch/sglass/cosarm/src/third_party/u-boot/files/tools/binman/test/Makefile:14:
> > *** Binman tests need to compile to x86, but the CPU arch of your
> > machine is x86_64. Set CROSS_COMPILE to a suitable cross compiler.
> > Stop.
> >
> > Can you make it work on both i386 and x86_64 without complaining? It
> > looks like that is the intent.
>
> I messed up the variable names: defined HOST_ARCH, but used HOSTARCH in
> the check. Fixing that makes it work like it should. (I'll go with
> HOSTARCH since that's where I took the "uname -m | sed" call from).
>
> > Also I'm not sure we need to define vars for all the tools, so you
> > could perhaps drop those that are not needed.
>
> Looks like we don't need anything except CC and OBJCOPY, I'll drop the
> rest.

OK great.

Regards,
Simon