[Buildroot] [PATCH 1/1] package/octave: add config support for ulibc-ng and musl

[Buildroot] [PATCH 1/1] package/octave: add config support for ulibc-ng and musl

[Alex Roberts]

From e00afef7a1811db5cd0a26e702ade0af1cdcca23 Mon Sep 17 00:00:00 2001
From: Alex Roberts <alex.roberts109@outlook.com>
Date: Sun, 4 Sep 2022 17:43:18 -0500
Subject: [Buildroot] [PATCH 1/1] package/octave: add config support for ulibc-ng and musl
 toolchains

Modified Config.in to allow Octave package to be built with Buildroot internal toolchains. This adds support
for both uClib-ng and musl based toolchain.

Octave requires locale support, this can be provided by the toolchain (default for musl and glibc). Locale support in
uClibc-ng is optional. If not enabled, locale support can be provided for Octave by libiconv (BR2_PACKAGE_LIBICONV).

octave.mk is modified to check if BR2_PACKAGE_LIBICONV is being used and will include libiconv as a build dependency.

This was tested with on arm32 and arm64 (aarch) targets with qemu_arm_versatile_defconfig and aarch64_virt_defconfig.

octave-cli --eval 'oruntests general'

All tests passed. uClibc-ng has issues on aarch64 and segfaults on program termination.

Signed-off-by: Alex Roberts <alex.roberts109@outlook.com>
---
 package/octave/Config.in | 17 ++++++++++++++---
 package/octave/octave.mk |  4 ++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/package/octave/Config.in b/package/octave/Config.in
index 171bdfa156..c74be51c26 100644
--- a/package/octave/Config.in
+++ b/package/octave/Config.in
@@ -4,6 +4,10 @@ config BR2_PACKAGE_OCTAVE
 	depends on BR2_INSTALL_LIBSTDCPP
 	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 # C++11
 	depends on BR2_TOOLCHAIN_HAS_FORTRAN
+	depends on BR2_TOOLCHAIN_HAS_OPENMP
+	depends on BR2_USE_WCHAR
+	depends on (BR2_ENABLE_LOCALE || BR2_PACKAGE_LIBICONV)
+
 	# Some Bootlin x86_64 toolchains (like version
 	# bleeding-edge-2021.11-1) has a file
 	# "x86_64-buildroot-linux-gnu/lib64/libgfortran.la" including
@@ -11,10 +15,12 @@ config BR2_PACKAGE_OCTAVE
 	# to linquadmath.la on the bootlin build host. This breaks
 	# builds using libtool with libgfortran. Those toolchains are
 	# used by the "utils/test-pkg" script.
-	depends on !BR2_TOOLCHAIN_EXTERNAL_BOOTLIN || !BR2_x86_64
+	depends on !BR2_x86_64
+
 	select BR2_PACKAGE_OPENBLAS
 	select BR2_PACKAGE_PCRE
 	select BR2_PACKAGE_PCRE_UTF
+
 	help
 	  GNU Octave is a high-level language, primarily intended for
 	  numerical computations. It provides a convenient command
@@ -33,7 +39,12 @@ config BR2_PACKAGE_OCTAVE
 
 	  https://www.octave.org/
 
-comment "octave needs a toolchain w/ C++ and fortran, gcc >= 4.8"
+comment "octave needs a toolchain w/ wchar, C++, Fortran, OpenMP, gcc >= 4.8"
 	depends on !BR2_INSTALL_LIBSTDCPP || \
 		!BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || \
-		!BR2_TOOLCHAIN_HAS_FORTRAN
+		!BR2_TOOLCHAIN_HAS_FORTRAN || \
+		!BR2_TOOLCHAIN_HAS_OPENMP || \
+		!BR2_USE_WCHAR 
+
+comment "octave needs locale support via toolchain or libiconv"
+	depends on !(BR2_ENABLE_LOCALE || BR2_PACKAGE_LIBICONV)
diff --git a/package/octave/octave.mk b/package/octave/octave.mk
index b28617438a..65552c1f78 100644
--- a/package/octave/octave.mk
+++ b/package/octave/octave.mk
@@ -25,4 +25,8 @@ else
 OCTAVE_CONF_OPTS += --disable-readline
 endif
 
+ifeq ($(BR2_PACKAGE_LIBICONV),y)
+OCTAVE_DEPENDENCIES += libiconv
+endif
+
 $(eval $(autotools-package))
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/octave: add config support for ulibc-ng and musl

[Yann E. MORIN]

Alex, All,

On 2022-09-05 01:21 +0000, Alex Roberts spake thusly:
> From e00afef7a1811db5cd0a26e702ade0af1cdcca23 Mon Sep 17 00:00:00 2001
> From: Alex Roberts <alex.roberts109@outlook.com>
> Date: Sun, 4 Sep 2022 17:43:18 -0500
> Subject: [Buildroot] [PATCH 1/1] package/octave: add config support for ulibc-ng and musl
>  toolchains

This is weird that your mail contains the git metadata. Normally, this
is not present when sent with "git send-email".

Also, your commit title states 'for ulibc-ng and musl', but all the
explanations seems to indicate that only uClibc-NG is concerned (since
locales are always available with musl).

> Modified Config.in to allow Octave package to be built with Buildroot internal toolchains. This adds support
> for both uClib-ng and musl based toolchain.

Please, wrap your commit log to ~72 chars.

Also, do not describe what you did ("Modified Config.in to ..."), but
explain what you did.

> Octave requires locale support, this can be provided by the toolchain (default for musl and glibc). Locale support in
> uClibc-ng is optional. If not enabled, locale support can be provided for Octave by libiconv (BR2_PACKAGE_LIBICONV).

This is a good intrductory blurb: it explains the context; the commit
log should start with that.

> octave.mk is modified to check if BR2_PACKAGE_LIBICONV is being used and will include libiconv as a build dependency.

Again, do not describe ("octave.mk is modified to..."), but explain.

> This was tested with on arm32 and arm64 (aarch) targets with qemu_arm_versatile_defconfig and aarch64_virt_defconfig.
> 
> octave-cli --eval 'oruntests general'
> 
> All tests passed. uClibc-ng has issues on aarch64 and segfaults on program termination.

Sorry, I don't understand... If "uClibc-ng has issues on aarch64 and
segfaults on program termination", then how can you conclude that "All
tests passed"?

It would also be very interesting to update the existing runtime test,
to include testing with uClibc-NG:

    support/testing/tests/package/test_octave.py

> Signed-off-by: Alex Roberts <alex.roberts109@outlook.com>
> --- a/package/octave/Config.in
> +++ b/package/octave/Config.in
> @@ -4,6 +4,10 @@ config BR2_PACKAGE_OCTAVE
>  	depends on BR2_INSTALL_LIBSTDCPP
>  	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 # C++11
>  	depends on BR2_TOOLCHAIN_HAS_FORTRAN
> +	depends on BR2_TOOLCHAIN_HAS_OPENMP

How is OpenMP related to the locale issue you explained in the commit
log? If OpenMP really is needed unconditionally, then this needs to be
changed in a separate patch.

There is --disable-openmp, so I guess this is actually not re
quired, in fact. But having it explicitly set would be nice (as a
separate patch; of course):

    ifeq ($(BR2_TOOLCHAIN_HAS_OPENMP),y)
    OCTAVE_CONF_OPTS += --enable-openmp
    else
    OCTAVE_CONF_OPTS += --disable-openmp
    endif

> +	depends on BR2_USE_WCHAR
> +	depends on (BR2_ENABLE_LOCALE || BR2_PACKAGE_LIBICONV)

We have quite some packages that do:

    select BR2_PACKAGE_LIBICONV if !BR2_ENABLE_LOCALE

> +

spurious empty line.

>  	# Some Bootlin x86_64 toolchains (like version
>  	# bleeding-edge-2021.11-1) has a file
>  	# "x86_64-buildroot-linux-gnu/lib64/libgfortran.la" including
> @@ -11,10 +15,12 @@ config BR2_PACKAGE_OCTAVE
>  	# to linquadmath.la on the bootlin build host. This breaks
>  	# builds using libtool with libgfortran. Those toolchains are
>  	# used by the "utils/test-pkg" script.
> -	depends on !BR2_TOOLCHAIN_EXTERNAL_BOOTLIN || !BR2_x86_64
> +	depends on !BR2_x86_64

Why did you drop the dependency on the Bootlin toolchains? If the issue
is no longer present, then the comment above would no longer be
applicable, so that would have to be modified.

Also, this mens that octave is no longer available for x86_64 at all,
even when using toolchains other than the Bootlin ones. Previously, it
was possible to use an internal toolchain, or any custom external
toolchain.

In any case, that would have to be a separate patch.

>  	select BR2_PACKAGE_OPENBLAS
>  	select BR2_PACKAGE_PCRE
>  	select BR2_PACKAGE_PCRE_UTF
> +

Spurious empty line.

>  	help
>  	  GNU Octave is a high-level language, primarily intended for
>  	  numerical computations. It provides a convenient command
> @@ -33,7 +39,12 @@ config BR2_PACKAGE_OCTAVE
>  
>  	  https://www.octave.org/
>  
> -comment "octave needs a toolchain w/ C++ and fortran, gcc >= 4.8"
> +comment "octave needs a toolchain w/ wchar, C++, Fortran, OpenMP, gcc >= 4.8"
>  	depends on !BR2_INSTALL_LIBSTDCPP || \
>  		!BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 || \
> -		!BR2_TOOLCHAIN_HAS_FORTRAN
> +		!BR2_TOOLCHAIN_HAS_FORTRAN || \
> +		!BR2_TOOLCHAIN_HAS_OPENMP || \
> +		!BR2_USE_WCHAR 
> +
> +comment "octave needs locale support via toolchain or libiconv"
> +	depends on !(BR2_ENABLE_LOCALE || BR2_PACKAGE_LIBICONV)

If you use the select as I suggested above, this coment is no longer
needed.

> diff --git a/package/octave/octave.mk b/package/octave/octave.mk
> index b28617438a..65552c1f78 100644
> --- a/package/octave/octave.mk
> +++ b/package/octave/octave.mk
> @@ -25,4 +25,8 @@ else
>  OCTAVE_CONF_OPTS += --disable-readline
>  endif
>  
> +ifeq ($(BR2_PACKAGE_LIBICONV),y)
> +OCTAVE_DEPENDENCIES += libiconv

Don't we also need to pass --with-libiconv-prefix?

And since there is --without-libiconv-prefix, can't we use that to
disable use of libiconv?

Regards,
Yann E. MORIN.

> +endif
> +
>  $(eval $(autotools-package))
> -- 
> 2.25.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/octave: add config support for ulibc-ng and musl

[Alex Roberts]

Yann, All,

>This is weird that your mail contains the git metadata. Normally, this is not present when sent with "git send-email".
Sorry about that.. first time trying to use git send-email. IMAP and SMTP settings weren't working with MS365/Outlook so I just copied/pasted the output of git patch-format directly into Outlook. First time trying to contribute as well so forgive my other mistakes. 

> Also, your commit title states 'for ulibc-ng and musl', but all the explanations seems to indicate that only uClibc-NG is concerned (since locales are always available with musl).
I tested configurations with uclibc-ng with locale support as part of the toolchain and without, but with libiconv instead. 

For musl, as you said locales are always available, but BR2_TOOLCHAIN_HAS_FORTRAN, BR2_TOOLCHAIN_HAS_OPENMP and BR2_TOOLCHAIN_BUILDROOT_CXX are not active by default with qemu_arm_versatile_defconfig and aarch64_virt_defconfig.
These configurations must be enabled for both musl and ublic-ng toolchains. 

>Sorry, I don't understand... If "uClibc-ng has issues on aarch64 and segfaults on program termination", then how can you conclude that "All tests passed"?
Calling "octave --eval 'oruntests general' " fails on aarch64/uclibc-ng as it segfaults when octave detects there is no display in the qemu environment, just after "src/main.in.cc:456: std::cerr << "octave: " << display_check_err_msg << std::endl"
# octave --eval 'oruntests general'
octave: Segmentation fault

Calling "octave-cli --eval 'oruntests general' works, and all the test pass - just ran again to confirm and I correct myself this does not segfault at completion.

I've been sitting on this patch for a while because of the segfault with aarch64/uclibc-ng trying to figure it out, it wasn't 
until I saw https://marc.info/?l=buildroot&m=166021556008040&w=2  that it made me think it's a problem with ulibc-ng/aarch64 hence why I also tested uclibc-ng/arm32.
Indeed, a simple helloworld will segfault, https://stackoverflow.com/questions/73393107/segfault-when-writing-to-cerr-iostream-uclibc-ng-unhandled-exception (my post)

>How is OpenMP related to the locale issue you explained in the commit log? If OpenMP really is needed unconditionally, then this needs to be changed in a separate patch.
>There is --disable-openmp, so I guess this is actually not re quired, in fact. But having it explicitly set would be nice (as a separate patch; of course):

I was not aware of this configuration option. I can make this a separate patch.

> We have quite some packages that do:

	>select BR2_PACKAGE_LIBICONV if !BR2_ENABLE_LOCALE

I will change it to follow suit.

>> -	depends on !BR2_TOOLCHAIN_EXTERNAL_BOOTLIN || !BR2_x86_64
>> +	depends on !BR2_x86_64
>Why did you drop the dependency on the Bootlin toolchains? If the issue is no longer present, then the comment above would no longer be applicable, so that would have to be modified.
>Also, this mens that octave is no longer available for x86_64 at all, even when using toolchains other than the Bootlin ones. Previously, it was possible to use an internal toolchain, or any custom external toolchain.

The intent was to enable internal toolchains. I misunderstood the behavior of that depends. I will correct that.

>> +comment "octave needs locale support via toolchain or libiconv">
> +	depends on !(BR2_ENABLE_LOCALE || BR2_PACKAGE_LIBICONV)

>If you use the select as I suggested above, this coment is no longer needed.

>> +ifeq ($(BR2_PACKAGE_LIBICONV),y)
>> +OCTAVE_DEPENDENCIES += libiconv

>Don't we also need to pass --with-libiconv-prefix?
>And since there is --without-libiconv-prefix, can't we use that to disable use of libiconv?
--with-libiconv-prefix doesn't seem to be necessary but I'd prefer to be explicit and like the idea of selectively enabling/disabling based on configuration.

Likewise there are other options like --without-curl and  --without-fftw3 that I plan to introduce with a separate patch. 

Is a patch for each option e.g, , --without-libiconv-prefix, --disable-openmp, --without-curl and  --without-fftw3 preferred so that patch is only changing one thing?

v/r
Alex Roberts
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot