Re: [Buildroot] [PATCH v2 1/4] package/binutils-bare-metal: add multiple tuple support

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Neal Frager via buildroot <buildroot@buildroot.org>
Cc: Neal Frager <neal.frager@amd.com>,
	ibai.erkiaga-elorza@amd.com, luca.ceresoli@bootlin.com,
	brandon.maier@collins.com, ju.o@free.fr, romain.naour@smile.fr,
	michal.simek@amd.com
Subject: Re: [Buildroot] [PATCH v2 1/4] package/binutils-bare-metal: add multiple tuple support
Date: Fri, 14 Feb 2025 22:58:31 +0100	[thread overview]
Message-ID: <20250214225831.23731a2d@windsurf> (raw)
In-Reply-To: <20250214165402.1128194-1-neal.frager@amd.com>

Hello Neal,

Thanks for your work on this topic. Overall, the approach you have
taken is exactly what I had in mind, so that's very good. I only have
some implementation details to comment on below.

On Fri, 14 Feb 2025 16:53:59 +0000
Neal Frager via buildroot <buildroot@buildroot.org> wrote:

> +# to build for multiple arch tuples, it is necesssary to build out of the
> +# source tree, so that each arch tuple can have its own build location
> +define HOST_BINUTILS_BARE_METAL_CONFIGURE_SYMLINKS
> +	$(foreach arch_tuple, $(TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH_TUPLE), \
> +		mkdir -p $(@D)/build-$(arch_tuple) && \
> +		ln -sf ../configure $(@D)/build-$(arch_tuple)/configure;)
> +endef

You don't need to create a symlink. We do that in gcc because we're
using <pkg>_SUBDIR, with which the default CONFIGURE_CMDS from
autotools-package assumes the configure script will be in the
<pkg>_SUBDIR. But here, since we are anyway implementing our custom
CONFIGURE_CMDS, creating this symlink is not needed. So I would drop
this HOST_BINUTILS_BARE_METAL_CONFIGURE_SYMLINKS entirely.

See below how things would work.

> +
> +HOST_BINUTILS_BARE_METAL_PRE_CONFIGURE_HOOKS += HOST_BINUTILS_BARE_METAL_CONFIGURE_SYMLINKS
> +
> +define HOST_BINUTILS_BARE_METAL_CONFIGURE_CMDS
> +	$(foreach arch_tuple, $(TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH_TUPLE), \

Add here:

		mkdir -p $(@D)/build-$(arch_tuple) && \

> +		cd $(@D)/build-$(arch_tuple) && \
> +		$(HOST_CONFIGURE_OPTS) \
> +		$(HOST_BINUTILS_BARE_METAL_CONF_ENV) \
> +		./configure \

Replace with:

		../configure

or:

		$(@D)/configure

whichever you prefer.

> +			--prefix=$(HOST_DIR) \
> +			--sysconfdir=$(HOST_DIR)/etc \
> +			--localstatedir=$(HOST_DIR)/var \
> +			--enable-shared --disable-static \
> +			--disable-gtk-doc \
> +			--disable-gtk-doc-html \
> +			--disable-doc \
> +			--disable-docs \
> +			--disable-documentation \
> +			--disable-debug \
> +			--with-xmlto=no \
> +			--with-fop=no \
> +			--disable-nls \
> +			--disable-dependency-tracking \
> +			--target=$(arch_tuple) \
> +			--disable-gprof \
> +			--enable-lto \
> +			--disable-initfini-array \
> +			--disable-multilib \
> +			--disable-werror; \

I think the final semi-colon here is not needed.

> +	)
> +endef
> +
> +define HOST_BINUTILS_BARE_METAL_BUILD_CMDS
> +	$(foreach arch_tuple, $(TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH_TUPLE), \
> +		$(HOST_MAKE_ENV) $(MAKE) \
> +			$(HOST_BINUTILS_BARE_METAL_MAKE_OPTS) \
> +			-C $(@D)/build-$(arch_tuple);)

Instead of this final semi-colon, put the closing parenthesis on the
next line, so that the loop looks like this:

	$(foreach bla,$(list),
		something
	)

> +endef
> +
> +define HOST_BINUTILS_BARE_METAL_INSTALL_CMDS
> +	$(foreach arch_tuple, $(TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH_TUPLE), \
> +		$(HOST_MAKE_ENV) $(MAKE) \
> +			$(HOST_BINUTILS_BARE_METAL_INSTALL_OPTS) \
> +			-C $(@D)/build-$(arch_tuple);)

Ditto here.

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot