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

2020-09-05 Thread Simon Glass 
Hi Alper,

On Sat, 5 Sep 2020 at 08:44, Alper Nebi Yasak  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 
> ---
>
>  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

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

2020-09-05 Thread 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'.

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

2020-09-05 Thread Simon Glass 
Hi Alper,

On Sat, 5 Sep 2020 at 17:04, Alper Nebi Yasak  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 

Regards,
Simon