From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Date: Mon, 7 May 2018 17:08:55 +0200
Subject: [Buildroot] [RFC PATCH 2/7] docs/manual: document pkg-meson
 infra
In-Reply-To: <20180507105741.31747-3-eric.le.bihan.dev@free.fr>
References: <20180507105741.31747-1-eric.le.bihan.dev@free.fr>
 <20180507105741.31747-3-eric.le.bihan.dev@free.fr>
Message-ID: <20180507170855.3bf884fa@windsurf>
List-Id: <buildroot.busybox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: buildroot@busybox.net

Hello,

On Mon,  7 May 2018 12:57:36 +0200, Eric Le Bihan wrote:
> Update documentation about adding meson-based packages with instructions for
> using pkg-meson infrastructure.
> 
> Signed-off-by: Eric Le Bihan <eric.le.bihan.dev@free.fr>

Looks good overall.


> -47: $(eval $(generic-package))
> +16: ifeq ($(BR2_PACKAGE_BAZ),y)
> +17: FOO_CONF_OPTS += -Dbaz
> +18: endif

I know it wasn't in the example previously, but would it make sense to
add:

FOO_DEPENDENCIES += baz

inside this condition ? It is quite common that external packages
enabling features for a given package are build dependencies of that
package.

> +==== +meson-package+ reference
> +
> +The main macro of the Meson package infrastructure is +meson-package+. It is
> +similar to the +generic-package+ macro.
> +
> +Just like the generic infrastructure, the Meson infrastructure works by defining
> +a number of variables before calling the +meson-package+ macro.
> +
> +First, all the package metadata information variables that exist in the generic
> +infrastructure also exist in the Meson infrastructure: +FOO_VERSION+,
> ++FOO_SOURCE+, +FOO_PATCH+, +FOO_SITE+, +FOO_SUBDIR+, +FOO_DEPENDENCIES+,
> ++FOO_INSTALL_STAGING+, +FOO_INSTALL_TARGET+.
> +
> +A few additional variables, specific to the Meson infrastructure, can also be
> +defined. Many of them are only useful in very specific cases, typical packages
> +will therefore only use a few of them.
> +
> +* +FOO_CONF_ENV+, to specify additional environment variables to pass to
> +  +meson+ for the configuration step. By default, empty.
> +
> +* +FOO_CONF_OPTS+, to specify additional options to pass to +meson+ for the
> +  configuration step. By default, empty.
> +
> +* +FOO_NINJA_ENV+, to specify additional environment variables to pass to
> +  +ninja+, meson companion tool in charge of the build operations. By default,
> +  empty.

Regarding the variable naming, I'm wondering if FOO_CONF_{ENV,OPTS} and
FOO_NINJA_ENV is very consistent.

Indeed, either you name the options after the step to which they apply:
FOO_CONF_ENV, FOO_CONF_OPTS, FOO_BUILD_ENV. Or after the tool to which
they apply: FOO_MESON_ENV, FOO_MESON_OPTS, FOO_NINJA_ENV.

But:

 - We already use this inconsistent notation: CONF_ENV and MAKE_ENV for
   autotools packages

 - Ninja options are not only used at build time, but also at install
   time.

 - Renaming FOO_CONF_OPTS to FOO_MESON_OPTS would really be weird, and
   FOO_CONF_OPTS definitely makes a lot more sense.

Conclusion: don't change anything, your current naming is my preference.

However, I think you forgot to mention the existence of a host variant
for Meson packages. The other package infrastructures say: "The ability
to have target and host packages is also available, with the
host-cmake-package macro."

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com