[1/1] package/tealdeer: exclude unsupported targets

Not all target architectures are supported by the "ring" dependency.

Signed-off-by: Danilo Bargen <mail@dbrgn.ch>
---
 package/tealdeer/Config.in | 8 ++++++++
 1 file changed, 8 insertions(+)

+    depends on !BR2_s390x
 	select BR2_PACKAGE_HOST_RUSTC
 	help
 	  A fast and full-featured tldr client. tldr pages are
--
2.38.1

Hello Danilo,

On Thu, 8 Dec 2022 11:02:47 +0100
Danilo Bargen <mail@dbrgn.ch> wrote:

> Not all target architectures are supported by the "ring" dependency.
> 
> Signed-off-by: Danilo Bargen <mail@dbrgn.ch>

Thanks for working on this. Your patch is badly line-wrapped by your
e-mail client. You can disable wrapping in Claws Mail, or better you
can use "git send-email". See below for another comment.

> diff --git a/package/tealdeer/Config.in b/package/tealdeer/Config.in
> index 96ed81614b..a61a59f3ee 100644
> --- a/package/tealdeer/Config.in
> +++ b/package/tealdeer/Config.in
> @@ -1,6 +1,14 @@
>  config BR2_PACKAGE_TEALDEER
>  	bool "tealdeer"
>  	depends on BR2_PACKAGE_HOST_RUSTC_TARGET_ARCH_SUPPORTS
> +    # Crypto dependency (ring) not available for mips:
> https://github.com/briansmith/ring/issues/562
> +    depends on !(BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el)
> +    # Crypto dependency (ring) not available for PowerPC:
> https://github.com/briansmith/ring/issues/389
> +    depends on !(BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le)
> +    # Crypto dependency (ring) not available for Sparc:
> https://github.com/briansmith/ring/issues/1512
> +    depends on !(BR2_sparc || BR2_sparc64)
> +    # Crypto dependency (ring) not available for s390x:
> https://github.com/briansmith/ring/commit/4d2e1a8fb80398f7c3c0137ea8fdd512f82d37a0
> +    depends on !BR2_s390x

We normally handle this kind of situation using a
BR2_PACKAGE_TEALDEER_ARCH_SUPPORTS variable, like this:

config BR2_PACKAGE_TEALDEER_ARCH_SUPPORTS
	bool
	default y if ...
	depends on ...

config BR2_PACKAGE_TEALDEER
	bool "tealdeer"
	depends on BR2_PACKAGE_TEALDEER_ARCH_SUPPORTS

Also, in this kind of case, we prefer to use "positive" logic: instead
of excluding the architectures that are not supported, it is better to
list the ones that are supported.

Side note: overall, if you take a step back, it is absolutely
mind-blowing that a simple application showing tldr pages has a
dependency on architecture-specific things. There is really no sane
reason to have such dependencies, but apparently the Rust ecosystem is
not completely sane... :-/

Thomas

Hello Thomas

> Thanks for working on this. Your patch is badly line-wrapped by your
> e-mail client. You can disable wrapping in Claws Mail, or better you
> can use "git send-email". See below for another comment.

Oops, I do use Claws Mail, and used "Edit with external editor" for pasting the patch. I didn't notice that this doesn't disable auto-wrapping.

> Side note: overall, if you take a step back, it is absolutely
> mind-blowing that a simple application showing tldr pages has a
> dependency on architecture-specific things. There is really no sane
> reason to have such dependencies, but apparently the Rust ecosystem is
> not completely sane... :-/

The reason is that Tealdeer uses https://github.com/rustls/rustls for TLS, which depends on the "ring" library https://github.com/briansmith/ring/. To quote from its README:

> ring is focused on the implementation, testing, and optimization of a core set of cryptographic operations exposed via an easy-to-use (and hard-to-misuse) API. ring exposes a Rust API and is written in a hybrid of Rust, C, and assembly language.
> ...
> Most of the C and assembly language code in ring comes from BoringSSL, and BoringSSL is derived from OpenSSL. ring merges changes from BoringSSL regularly. Also, several changes that were developed for ring have been contributed to and integrated into BoringSSL.

Due to the reliance on BoringSSL and assembly code for some parts of the project, it is not as portable as pure-Rust code. Support for MIPS, PowerPC and other architectures is in the works, but not yet finished.

Previously tealdeer used OpenSSL, but quite a few of the users had trouble with broken OpenSSL installations. Plus, it made cross-compilation and static builds much harder. Using rustls makes this much easier on the common platforms, but of course, this has certain limitations with regards to more obscure target architectures.

If you would rather remove tealdeer from Buildroot (because the config is getting too complex), that would be OK for me. Otherwise, I'd update my patch based on your suggestions.

Danilo

Danilo, All,

On 2022-12-08 11:02 +0100, Danilo Bargen spake thusly:
> Not all target architectures are supported by the "ring" dependency.
> 
> Signed-off-by: Danilo Bargen <mail@dbrgn.ch>
> ---
>  package/tealdeer/Config.in | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/package/tealdeer/Config.in b/package/tealdeer/Config.in
> index 96ed81614b..a61a59f3ee 100644
> --- a/package/tealdeer/Config.in
> +++ b/package/tealdeer/Config.in
> @@ -1,6 +1,14 @@
>  config BR2_PACKAGE_TEALDEER
>  	bool "tealdeer"
>  	depends on BR2_PACKAGE_HOST_RUSTC_TARGET_ARCH_SUPPORTS
> +    # Crypto dependency (ring) not available for mips:
> https://github.com/briansmith/ring/issues/562
> +    depends on !(BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el)
> +    # Crypto dependency (ring) not available for PowerPC:
> https://github.com/briansmith/ring/issues/389
> +    depends on !(BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le)
> +    # Crypto dependency (ring) not available for Sparc:
> https://github.com/briansmith/ring/issues/1512
> +    depends on !(BR2_sparc || BR2_sparc64)
> +    # Crypto dependency (ring) not available for s390x:
> https://github.com/briansmith/ring/commit/4d2e1a8fb80398f7c3c0137ea8fdd512f82d37a0
> +    depends on !BR2_s390x

As Thomas already noted, this patch is badly broken.

I've massaged into back into something that looks nicer, and applied to
master, after simplifying the comment and moving the references to the
commit log.

Regards,
Yann E. MORIN.

>  	select BR2_PACKAGE_HOST_RUSTC
>  	help
>  	  A fast and full-featured tldr client. tldr pages are
> --
> 2.38.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

Danilo, All,

On 2022-12-08 16:48 +0100, Danilo Bargen spake thusly:
> > Side note: overall, if you take a step back, it is absolutely
> > mind-blowing that a simple application showing tldr pages has a
> > dependency on architecture-specific things. There is really no sane
> > reason to have such dependencies, but apparently the Rust ecosystem is
> > not completely sane... :-/
> 
> The reason is that Tealdeer uses https://github.com/rustls/rustls for
> TLS, which depends on the "ring" library
[--SNIP--]
> Previously tealdeer used OpenSSL, but quite a few of the users had
> trouble with broken OpenSSL installations. Plus, it made
> cross-compilation and static builds much harder. Using rustls makes
> this much easier on the common platforms, but of course, this has
> certain limitations with regards to more obscure target architectures.

cross-compilation with openssl is very easu. It just works.

That static build is harder I consider that a benefit rather than a
deficiency. ;-]

> If you would rather remove tealdeer from Buildroot (because the config
> is getting too complex), that would be OK for me. Otherwise, I'd
> update my patch based on your suggestions.

I think what Thomas meant, was that it is a pity that we need a TLS
implementation just to read the equivalent of man-pages, which by virtue
of being local oalways work, and that that TLS implementation has
limitations on architectures, which man-pages do not...

Regards,
Yann E. MORIN.

Hello Yann

Thanks for adapting and merging the patch!

> I think what Thomas meant, was that it is a pity that we need a TLS
> implementation just to read the equivalent of man-pages, which by virtue
> of being local oalways work, and that that TLS implementation has
> limitations on architectures, which man-pages do not...

Yes, I understand that :) The only reason is that the current tldr pages are fetched from the domain "tldr.sh" via HTTPS. An alternative to this would be to add a build flag to Tealdeer to not require/support network connectivity at all, and to bundle all current tldr pages with the buildroot package. This way, no network connectivity and no TLS would be needed. This feature was requested by a user a few years ago, and I already implemented it, but never got feedback on the implementation and nobody ever requested that feature again. So in the interest of keeping the codebase simple, I never merged the branch.

Anyways, I brought back a build flag to allow linking against OpenSSL instead: https://github.com/dbrgn/tealdeer/pull/303 Initially I had quite some trouble getting it to find OpenSSL, until I discovered that I need to add a dependency on host-pkgconf. Once the next release is out, I can submit a patch to build against OpenSSL instead of Rustls.

Cheers,
Danilo

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Danilo, All,
 > On 2022-12-08 11:02 +0100, Danilo Bargen spake thusly:
 >> Not all target architectures are supported by the "ring" dependency.
 >> 
 >> Signed-off-by: Danilo Bargen <mail@dbrgn.ch>
 >> ---
 >> package/tealdeer/Config.in | 8 ++++++++
 >> 1 file changed, 8 insertions(+)
 >> 
 >> diff --git a/package/tealdeer/Config.in b/package/tealdeer/Config.in
 >> index 96ed81614b..a61a59f3ee 100644
 >> --- a/package/tealdeer/Config.in
 >> +++ b/package/tealdeer/Config.in
 >> @@ -1,6 +1,14 @@
 >> config BR2_PACKAGE_TEALDEER
 >> bool "tealdeer"
 >> depends on BR2_PACKAGE_HOST_RUSTC_TARGET_ARCH_SUPPORTS
 >> +    # Crypto dependency (ring) not available for mips:
 >> https://github.com/briansmith/ring/issues/562
 >> +    depends on !(BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el)
 >> +    # Crypto dependency (ring) not available for PowerPC:
 >> https://github.com/briansmith/ring/issues/389
 >> +    depends on !(BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le)
 >> +    # Crypto dependency (ring) not available for Sparc:
 >> https://github.com/briansmith/ring/issues/1512
 >> +    depends on !(BR2_sparc || BR2_sparc64)
 >> +    # Crypto dependency (ring) not available for s390x:
 >> https://github.com/briansmith/ring/commit/4d2e1a8fb80398f7c3c0137ea8fdd512f82d37a0
 >> +    depends on !BR2_s390x

 > As Thomas already noted, this patch is badly broken.

 > I've massaged into back into something that looks nicer, and applied to
 > master, after simplifying the comment and moving the references to the
 > commit log.

Committed to 2022.11.x, thanks.