Re: [Buildroot] [PATCH 2/4] package/json-glib: add host build

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Jarkko, All,

Thanks for these patches; please find comments below.

On 2024-05-17 16:20 +0300, Jarkko Sakkinen spake thusly:
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  package/json-glib/json-glib.mk | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/package/json-glib/json-glib.mk b/package/json-glib/json-glib.mk
> index cd53f24cee..ffdc76f2c1 100644
> --- a/package/json-glib/json-glib.mk
> +++ b/package/json-glib/json-glib.mk
> @@ -18,19 +18,31 @@ JSON_GLIB_DEPENDENCIES = \
>  	host-pkgconf \
>  	libglib2
>  
> +HOST_JSON_GLIB_DEPENDENCIES = \
> +	$(HOST_NLS_DEPENDENCIES) \
> +	host-pkgconf \
> +	host-libglib2
> +
>  ifeq ($(BR2_PACKAGE_GOBJECT_INTROSPECTION),y)
>  JSON_GLIB_CONF_OPTS += -Dintrospection=enabled
>  JSON_GLIB_DEPENDENCIES += gobject-introspection
> +HOST_JSON_GLIB_CONF_OPTS += -Dintrospection=enabled

BR2_PACKAGE_GOBJECT_INTROSPECTION (the condition for this block) is a
target setting, so it semantically does not make sense to protect the
host variant with that condition.

Usually, for host variants, we do not have conditional compilation of
features; either the feature is needed to run on the host, in which case
we always enable it, or it is not needed, in which case we always
disable it.

The only case where we would have such an option for such a feature,
is when the feature needs a lot of dependencies, or time-consuming
dependencies (e.g. needs LLVM!). GOI is probably a good reasong to
add such an option, *iff* introspection is needed on the host, which I
doubt is.

> +HOST_JSON_GLIB_DEPENDENCIES += gobject-introspection

Here, you instruct a host variant to depend on a target variant, which
is usually not what you intended. And indeed, I believe here you'd need
a dependency on host-gobject-introspection. I guess it worked ion your
case, because gobject-introspection has a dependency on
host-gobject-introspection, so that pulled it in for you. Still, this is
probably not correct.

So, my proposal would be to always disable GOI unconditionally in the
host variant, unless there are cases where it is required, in which case
we always enable it.

Unless there is actually a reason that the host variant has the same
feature set as the target variant, in which case it should be explained
in the commit log.

>  else
>  JSON_GLIB_CONF_OPTS += -Dintrospection=disabled
> +HOST_JSON_GLIB_CONF_OPTS += -Dintrospection=disabled
>  endif
>  
>  ifeq ($(BR2_SYSTEM_ENABLE_NLS),y)
>  JSON_GLIB_CONF_OPTS += -Dnls=enabled
> +HOST_JSON_GLIB_CONF_OPTS += -Dnls=enabled
>  else
>  JSON_GLIB_CONF_OPTS += -Dnls=disabled
> +HOST_JSON_GLIB_CONF_OPTS += -Dnls=disabled
>  endif

Ditto: the BR2_SYSTEM_ENABLE_NLS option drives NLS support for the
target, not for the host. For the host, we assume it is never needed,
and it is already forcefully disabled in the autotools-package infra:
    https://gitlab.com/buildroot.org/buildroot/-/blob/master/package/pkg-autotools.mk#L196

>  JSON_GLIB_LDFLAGS = $(TARGET_LDFLAGS) $(TARGET_NLS_LIBS)
> +HOST_JSON_GLIB_LDFLAGS = $(HOST_LDFLAGS) $(HOST_NLS_LIBS)

HOST_NLS_LIBS is never defined anywhere, so this is basically a noop.
;-)

Regards,
Yann E. MORIN.

>  $(eval $(meson-package))
> +$(eval $(host-meson-package))
> -- 
> 2.45.0
> 
> _______________________________________________
> 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