Re: [Buildroot] [RFC v2 08/14] package/hexagonMVM: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello,

On Wed, 18 Dec 2024 21:30:47 -0800
Brian Cain via buildroot <buildroot@buildroot.org> wrote:

> MiniVM is a virtual machine used by the Linux kernel for
> the hexagon architecture.

Is it "MiniVM" or hexagonMVM ? Also, with your definition, I'm really
unclear about what hexagonMVM really is. Is it a VM that runs under
Linux? Or the VM under which Linux runs? Or?

> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  package/Config.in                  |  1 +
>  package/hexagonMVM/Config.in       |  8 ++++++++
>  package/hexagonMVM/hexagonMVM.hash |  5 +++++
>  package/hexagonMVM/hexagonMVM.mk   | 27 +++++++++++++++++++++++++++
>  4 files changed, 41 insertions(+)

Package name should be lower-case. Maybe hexagon-mvm.

Please update the DEVELOPERS file in this file.

> diff --git a/package/hexagonMVM/Config.in b/package/hexagonMVM/Config.in
> new file mode 100644
> index 0000000000..b00666188b
> --- /dev/null
> +++ b/package/hexagonMVM/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_HEXAGONMVM
> +	bool "Hexagon MiniVM"
> +	depends on BR2_hexagon && BR2_TOOLCHAIN_CLANG_AT_LEAST_19_0

BR2_TOOLCHAIN_CLANG_AT_LEAST_19_0 doesn't exist as of this commit, so
you have a patch ordering issue.

> +	help
> +	  Hexagon VM implementation.  This virtual machine can be used
> +	  to boot linux or other operating systems / programs.

Ah so its the VM under which Linux runs?

> +
> +	  https://github.com/quic/hexagonMVM
> diff --git a/package/hexagonMVM/hexagonMVM.hash b/package/hexagonMVM/hexagonMVM.hash
> new file mode 100644
> index 0000000000..3e1ae8e2b4
> --- /dev/null
> +++ b/package/hexagonMVM/hexagonMVM.hash
> @@ -0,0 +1,5 @@
> +
> +#

Minor: drop those two lines.

> +# Locally computed
> +sha256  e5e3ba941af491cab37ece5db85eb7422db9dd0d13464ea528dbf2c01c24b5f0  LICENSE
> +sha256  5345cd83b18b6b8f44d06a54bd1a518d447c311dc1c3ba7b45ffa16785e66e1b  0.2.0.tar.gz

Tarball first, license file afterwards. The tarball name is not good,
but I'll comment on that later.

> diff --git a/package/hexagonMVM/hexagonMVM.mk b/package/hexagonMVM/hexagonMVM.mk
> new file mode 100644
> index 0000000000..7b3529c14c
> --- /dev/null
> +++ b/package/hexagonMVM/hexagonMVM.mk
> @@ -0,0 +1,27 @@
> +################################################################################
> +#
> +# Hexagon MiniVM
> +#
> +################################################################################
> +
> +HEXAGONMVM_VERSION = 0.2.0
> +HEXAGONMVM_SOURCE = $(HEXAGONMVM_VERSION).tar.gz

Drop this line, it will give the tarball a proper name.

> +HEXAGONMVM_SITE = $(call github,quic,hexagonMVM,v$(HEXAGONMVM_VERSION))
> +
> +HEXAGONMVM_LICENSE = BSD-3-Clause
> +HEXAGONMVM_LICENSE_FILES = LICENSE
> +
> +HEXAGONMVM_DEPENDENCIES =

If there are no dependencies, just keep it empty.

> +HEXAGONMVM_INSTALL_IMAGES = YES
> +
> +HEXAGONMVM_CFLAGS=-fno-pie -fno-pic

Spaces around = sign. Why aren't those flags in the package
makefile/build-system already?

> +define HEXAGONMVM_BUILD_CMDS
> +	$(MAKE) $(TARGET_CONFIGURE_OPTS) CFLAGS="$(HEXAGONMVM_CFLAGS)" -C $(@D) minivm
> +endef
> +
> +define HEXAGONMVM_INSTALL_IMAGES_CMDS
> +	$(MAKE) $(TARGET_CONFIGURE_OPTS) CFLAGS="$(HEXAGONMVM_CFLAGS)" bindir=$(BINARIES_DIR) -C $(@D) install
> +endef
> +
> +$(eval $(generic-package))

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