| Message ID | 20240517132039.7124-3-jarkko@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | swtpm and libtpms host packages | expand |
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
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
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 +HOST_JSON_GLIB_DEPENDENCIES += gobject-introspection 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 JSON_GLIB_LDFLAGS = $(TARGET_LDFLAGS) $(TARGET_NLS_LIBS) +HOST_JSON_GLIB_LDFLAGS = $(HOST_LDFLAGS) $(HOST_NLS_LIBS) $(eval $(meson-package)) +$(eval $(host-meson-package))
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- package/json-glib/json-glib.mk | 12 ++++++++++++ 1 file changed, 12 insertions(+)