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

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Fri May 17, 2024 at 4:39 PM EEST, Yann E. MORIN wrote:
> 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.
> ;-)

Thanks for the feedback.

I have already patch in my tree to address this, which I will later on
squash to the patch under the review:

https://gitlab.com/jarkkojs/buildroot/-/commits/swtpm

> Regards,
> Yann E. MORIN.

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