Re: [Buildroot] [PATCH v3 1/1] package/pkg-meson: use meson to build/install packages

[Arnout Vandecappelle <arnout@mind.be>]

On 24/07/2022 17:37, Yann E. MORIN wrote:
> James, All,
> 
> On 2022-07-22 00:09 -0600, James Hilliard spake thusly:
>> As of version 0.54.0 meson has had the ability to build and install
>> packages rather than having to run ninja directly as before.
>>
>> This will allow us to use features such as meson install tags in
>> the future which require meson to be used for the installation.
>>
>> It appears we need to ensure the cmake prefix path is set for the
>> meson generated relocatable pkg-config format to work properly.
>>
>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>> ---
> [--SNIP--]
>> diff --git a/docs/manual/adding-packages-meson.txt b/docs/manual/adding-packages-meson.txt
>> index 029c8c2488..c8aed65c2f 100644
>> --- a/docs/manual/adding-packages-meson.txt
>> +++ b/docs/manual/adding-packages-meson.txt
>> @@ -125,8 +125,8 @@ will therefore only use a few of them.
>>     +c_link_args+, +cpp_args+, +cpp_link_args+, +sys_root+, and
>>     +pkg_config_libdir+.
>>   
>> -* +FOO_NINJA_ENV+, to specify additional environment variables to pass to
>> -  +ninja+, meson companion tool in charge of the build operations. By default,
>> +* +FOO_MESON_ENV+, to specify additional environment variables to pass to
>> +  +meson+, meson tool in charge of the build/install operations. By default,
>>     empty.
> 
> IIRC, the topic of what to do about external packages that set their
> FOO_NINJA_ENV, was raised in a previous iteration.
> 
> I understand that the conclusion was basically "the probability is low,
> we don't handle it". This should have been noted in the commit log.
> 
>>   * +FOO_NINJA_OPTS+, to specify a space-separated list of targets to build. By
> 
> Now that we use meson to build and instal, does it still make sense to
> have FOO_NINJA_OPTS to be the list of targets to install?
> 
> I mean, isn't there a way to tell meson what to install, instead?

  FOO_NINJA_OPTS could theoretically be used to control other things about 
ninja, e.g. warnings and debugging. Though I don't really see why you'd ever 
want to do that in a buildroot package.

> 
> [--SNIP--]
>> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
>> index 3b1db35fb6..4fa620080d 100644
>> --- a/package/pkg-cmake.mk
>> +++ b/package/pkg-cmake.mk
>> @@ -90,6 +90,8 @@ define $(2)_CONFIGURE_CMDS
>>   	$$($$(PKG)_CONF_ENV) $$(BR2_CMAKE) $$($$(PKG)_SRCDIR) \
>>   		-DCMAKE_TOOLCHAIN_FILE="$$(HOST_DIR)/share/buildroot/toolchainfile.cmake" \
>>   		-DCMAKE_INSTALL_PREFIX="/usr" \
>> +		-DCMAKE_PREFIX_PATH="$$(STAGING_DIR)/usr" \

  Wouldn't it be better to put this in the toolchainfile?

>> +		-DPKG_CONFIG_USE_CMAKE_PREFIX_PATH=ON \

  This one as well, I guess.

> 
> But doesn't that needs to be in a separate patch? I mean, does it not
> make sense opn its own?

  +1.

  The commit message of that patch could also explain better what it does exactly.

> 
> [--SNIP--]
>> diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk
>> index 0835e08e3a..de5817d5da 100644
>> --- a/package/pkg-meson.mk
>> +++ b/package/pkg-meson.mk
> [--SNIP--]
>> @@ -138,6 +138,7 @@ define $(2)_CONFIGURE_CMDS
>>   	$$(MESON) \
>>   		--prefix=/usr \
>>   		--libdir=lib \
>> +		--pkgconfig.relocatable \
> 
> Please explain what that means and why this is needed in the context of
> a target package.

  And also it should probably be a separate patch - pkgconfig stuff should be 
done at configure time, not at build time, so using meson to start the build 
shouldn't really be related to it.

> 
>>   		--default-library=$(if $(BR2_STATIC_LIBS),static,shared) \
>>   		--buildtype=$(if $(BR2_ENABLE_RUNTIME_DEBUG),debug,release) \
>>   		--cross-file=$$($$(PKG)_SRCDIR)/build/cross-compilation.conf \
>> @@ -156,10 +157,11 @@ define $(2)_CONFIGURE_CMDS
>>   	mkdir -p $$($$(PKG)_SRCDIR)/build
>>   	$$(HOST_CONFIGURE_OPTS) \
>>   	$$($$(PKG)_CONF_ENV) $$(MESON) \
>> -		--prefix=$$(HOST_DIR) \
>> +		--prefix=/ \
> 
> It is very weird that host packages are configured with / as prefix, and
> then installed in HOST_DIR set as DEST_DIR.

  It's not weird, it's wrong! There is no reasonable explanation how this can be 
correct. We are really installing stuff in that specific location, any paths 
should refer to that location and not a path beginning with /.

  If for some reason meson doesn't work when the prefix is not / or /usr or 
/usr/local, it's a bug in meson, really.

  Regards,
  Arnout

> 
> I guess this is working thanks to --pkgconfig.relocatable a few lines
> below?
> 
> This must be explained in the commit log (and probably a little comment
> above $(2)_CONFIGURE_CMDS for host packages.
> 
>>   		--libdir=lib \
>> -		--sysconfdir=$$(HOST_DIR)/etc \
>> -		--localstatedir=$$(HOST_DIR)/var \
>> +		--pkgconfig.relocatable \
>> +		--sysconfdir=etc \
>> +		--localstatedir=var \

  These two could be correct, however, since I think they're taken relative to 
prefix (the default values are absolute because the default prefix is /usr and 
sysconfdir is /etc, not /usr/etc - but in our case a relative path is fine).

  Regards,
  Arnout

> 
> Ditto for etc and var?
> 
> So, to make it easier to follow, I'd change the ordering:
> 
>      --pkgconfig.relocatable \
>      --prefix=/ \
>      --sysconfdir=etc \
>      --localstatedir=var \
> 
> Regards,
> Yann E. MORIN.
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot