[PATCH-for-master,1/2] package/meson: don't install cross-compilation.conf during target-finalize
diff mbox series

Message ID 20190225211147.14947-2-patrickdepinguin@gmail.com
State New
Headers show
Series
  • misc fixes for 2019.02
Related show

Commit Message

Thomas De Schampheleire Feb. 25, 2019, 9:11 p.m. UTC
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

package/meson installs a cross-compilation.conf file in
$(HOST_DIR)/etc/meson, via TARGET_FINALIZE_HOOKS.

package/pkg-cmake.mk installs a toolchainfile.cmake in
$(HOST_DIR)/share/buildroot, via TOOLCHAIN_POST_INSTALL_STAGING_HOOKS.

Both files have a similar concept, they describe some flags/paths needed for
compilation using respective build systems. One difference is that the meson
file is added for external compilation, from the SDK, while the cmake file
is used internally in Buildroot.

The 'problem' of using TARGET_FINALIZE_HOOKS for the meson file, is that it
installs a 'host' file from target-finalize, which is conceptually incorrect
and breaks the invariant that only TARGET_DIR is changed on a subsequent
'make' when everything was already built (i.e. only target-finalize is run).

This can easily be fixed, by using the same hook as cmake uses, i.e.
TOOLCHAIN_POST_INSTALL_STAGING_HOOKS.

Note that actually even for cmake, TOOLCHAIN_POST_INSTALL_STAGING_HOOKS is
not the best hook to install a host file. A better hook would have been
TOOLCHAIN_POST_INSTALL_HOOKS, but this triggers only for 'host' packages,
and 'toolchain' is treated as a 'target' package.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 package/meson/meson.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Peter Korsgaard Feb. 26, 2019, 6:52 p.m. UTC | #1
>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

 > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
 > package/meson installs a cross-compilation.conf file in
 > $(HOST_DIR)/etc/meson, via TARGET_FINALIZE_HOOKS.

 > package/pkg-cmake.mk installs a toolchainfile.cmake in
 > $(HOST_DIR)/share/buildroot, via TOOLCHAIN_POST_INSTALL_STAGING_HOOKS.

 > Both files have a similar concept, they describe some flags/paths needed for
 > compilation using respective build systems. One difference is that the meson
 > file is added for external compilation, from the SDK, while the cmake file
 > is used internally in Buildroot.

 > The 'problem' of using TARGET_FINALIZE_HOOKS for the meson file, is that it
 > installs a 'host' file from target-finalize, which is conceptually incorrect
 > and breaks the invariant that only TARGET_DIR is changed on a subsequent
 > 'make' when everything was already built (i.e. only target-finalize is run).

 > This can easily be fixed, by using the same hook as cmake uses, i.e.
 > TOOLCHAIN_POST_INSTALL_STAGING_HOOKS.

 > Note that actually even for cmake, TOOLCHAIN_POST_INSTALL_STAGING_HOOKS is
 > not the best hook to install a host file. A better hook would have been
 > TOOLCHAIN_POST_INSTALL_HOOKS, but this triggers only for 'host' packages,
 > and 'toolchain' is treated as a 'target' package.

 > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
 > ---
 >  package/meson/meson.mk | 3 ++-
 >  1 file changed, 2 insertions(+), 1 deletion(-)

 > diff --git a/package/meson/meson.mk b/package/meson/meson.mk
 > index d76541cc93..aba7382cc9 100644
 > --- a/package/meson/meson.mk
 > +++ b/package/meson/meson.mk
 > @@ -65,6 +65,7 @@ define HOST_MESON_INSTALL_CROSS_CONF
 >> $(HOST_DIR)/etc/meson/cross-compilation.conf
 >  endef
 
 > -TARGET_FINALIZE_HOOKS += HOST_MESON_INSTALL_CROSS_CONF
 > +TOOLCHAIN_POST_INSTALL_STAGING_HOOKS += HOST_MESON_INSTALL_CROSS_CONF

Is this then going to use the correct (final) paths for per-package
build directories?
Thomas De Schampheleire Feb. 26, 2019, 7:48 p.m. UTC | #2
El mar., 26 feb. 2019 a las 19:52, Peter Korsgaard
(<peter@korsgaard.com>) escribió:
>
> >>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:
>
>  > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>  > package/meson installs a cross-compilation.conf file in
>  > $(HOST_DIR)/etc/meson, via TARGET_FINALIZE_HOOKS.
>
>  > package/pkg-cmake.mk installs a toolchainfile.cmake in
>  > $(HOST_DIR)/share/buildroot, via TOOLCHAIN_POST_INSTALL_STAGING_HOOKS.
>
>  > Both files have a similar concept, they describe some flags/paths needed for
>  > compilation using respective build systems. One difference is that the meson
>  > file is added for external compilation, from the SDK, while the cmake file
>  > is used internally in Buildroot.
>
>  > The 'problem' of using TARGET_FINALIZE_HOOKS for the meson file, is that it
>  > installs a 'host' file from target-finalize, which is conceptually incorrect
>  > and breaks the invariant that only TARGET_DIR is changed on a subsequent
>  > 'make' when everything was already built (i.e. only target-finalize is run).
>
>  > This can easily be fixed, by using the same hook as cmake uses, i.e.
>  > TOOLCHAIN_POST_INSTALL_STAGING_HOOKS.
>
>  > Note that actually even for cmake, TOOLCHAIN_POST_INSTALL_STAGING_HOOKS is
>  > not the best hook to install a host file. A better hook would have been
>  > TOOLCHAIN_POST_INSTALL_HOOKS, but this triggers only for 'host' packages,
>  > and 'toolchain' is treated as a 'target' package.
>
>  > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>  > ---
>  >  package/meson/meson.mk | 3 ++-
>  >  1 file changed, 2 insertions(+), 1 deletion(-)
>
>  > diff --git a/package/meson/meson.mk b/package/meson/meson.mk
>  > index d76541cc93..aba7382cc9 100644
>  > --- a/package/meson/meson.mk
>  > +++ b/package/meson/meson.mk
>  > @@ -65,6 +65,7 @@ define HOST_MESON_INSTALL_CROSS_CONF
>  >> $(HOST_DIR)/etc/meson/cross-compilation.conf
>  >  endef
>
>  > -TARGET_FINALIZE_HOOKS += HOST_MESON_INSTALL_CROSS_CONF
>  > +TOOLCHAIN_POST_INSTALL_STAGING_HOOKS += HOST_MESON_INSTALL_CROSS_CONF
>
> Is this then going to use the correct (final) paths for per-package
> build directories?
>

I haven't played with / investigated the per-package feature yet, so I
may miss some specifics.
Below is the content of the hook:

define HOST_MESON_INSTALL_CROSS_CONF
    mkdir -p $(HOST_DIR)/etc/meson
    sed -e "s%@TARGET_CROSS@%$(TARGET_CROSS)%g" \
        -e "s%@TARGET_ARCH@%$(HOST_MESON_TARGET_CPU_FAMILY)%g" \
        -e "s%@TARGET_CPU@%$(HOST_MESON_TARGET_CPU)%g" \
        -e "s%@TARGET_ENDIAN@%$(HOST_MESON_TARGET_ENDIAN)%g" \
        -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \
        -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \
        -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \
        -e "s%@HOST_DIR@%$(HOST_DIR)%g" \
        $(HOST_MESON_PKGDIR)/cross-compilation.conf.in \
        > $(HOST_DIR)/etc/meson/cross-compilation.conf
endef

Are there any variables in there that would change value?

In sequential build, the file before and after this change is identical.

Best regards,
Thomas
Thomas Petazzoni Feb. 27, 2019, 5:18 p.m. UTC | #3
Hello,

On Tue, 26 Feb 2019 20:48:49 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

> I haven't played with / investigated the per-package feature yet, so I
> may miss some specifics.
> Below is the content of the hook:
> 
> define HOST_MESON_INSTALL_CROSS_CONF
>     mkdir -p $(HOST_DIR)/etc/meson
>     sed -e "s%@TARGET_CROSS@%$(TARGET_CROSS)%g" \
>         -e "s%@TARGET_ARCH@%$(HOST_MESON_TARGET_CPU_FAMILY)%g" \
>         -e "s%@TARGET_CPU@%$(HOST_MESON_TARGET_CPU)%g" \
>         -e "s%@TARGET_ENDIAN@%$(HOST_MESON_TARGET_ENDIAN)%g" \
>         -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \
>         -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \
>         -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \
>         -e "s%@HOST_DIR@%$(HOST_DIR)%g" \

With per-package enabled, this $(HOST_DIR) here will point to the
per-package host directory of host-meson instead of pointing to the
global host directory.

>         $(HOST_MESON_PKGDIR)/cross-compilation.conf.in \
>         > $(HOST_DIR)/etc/meson/cross-compilation.conf  

Here as well, but this one is not an issue: the per-package host
directory of host-meson will be rsync'ed into the global host directory
at the end of the build.

> Are there any variables in there that would change value?

With per-package enabled, HOST_DIR, STAGING_DIR and TARGET_DIR have a
different value when building each package (they point to the current
package per-package directories) or when building stuff outside of a
package (they point to the global directories).

> In sequential build, the file before and after this change is identical.

It's not so much sequential vs. parallel, but per-package directories
or not. Per-package directory can be used while doing a sequential
build.

Best regards,

Thomas
Thomas De Schampheleire Feb. 27, 2019, 7:57 p.m. UTC | #4
El mié., 27 feb. 2019 a las 18:18, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribió:
>
> Hello,
>
> On Tue, 26 Feb 2019 20:48:49 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > I haven't played with / investigated the per-package feature yet, so I
> > may miss some specifics.
> > Below is the content of the hook:
> >
> > define HOST_MESON_INSTALL_CROSS_CONF
> >     mkdir -p $(HOST_DIR)/etc/meson
> >     sed -e "s%@TARGET_CROSS@%$(TARGET_CROSS)%g" \
> >         -e "s%@TARGET_ARCH@%$(HOST_MESON_TARGET_CPU_FAMILY)%g" \
> >         -e "s%@TARGET_CPU@%$(HOST_MESON_TARGET_CPU)%g" \
> >         -e "s%@TARGET_ENDIAN@%$(HOST_MESON_TARGET_ENDIAN)%g" \
> >         -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \
> >         -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \
> >         -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \
> >         -e "s%@HOST_DIR@%$(HOST_DIR)%g" \
>
> With per-package enabled, this $(HOST_DIR) here will point to the
> per-package host directory of host-meson instead of pointing to the
> global host directory.
>
> >         $(HOST_MESON_PKGDIR)/cross-compilation.conf.in \
> >         > $(HOST_DIR)/etc/meson/cross-compilation.conf
>
> Here as well, but this one is not an issue: the per-package host
> directory of host-meson will be rsync'ed into the global host directory
> at the end of the build.
>
> > Are there any variables in there that would change value?
>
> With per-package enabled, HOST_DIR, STAGING_DIR and TARGET_DIR have a
> different value when building each package (they point to the current
> package per-package directories) or when building stuff outside of a
> package (they point to the global directories).
>
> > In sequential build, the file before and after this change is identical.
>
> It's not so much sequential vs. parallel, but per-package directories
> or not. Per-package directory can be used while doing a sequential
> build.
>

Thanks for explaining everything.

If I understand it correctly, since the patch is hooking into
TOOLCHAIN_POST_INSTALL_STAGING_HOOKS, HOST_DIR will be the per-package
directory of 'toolchain', not of 'host-meson', right?

To solve it, we could change:
-        -e "s%@HOST_DIR@%$(HOST_DIR)%g" \
+        -e "s%@HOST_DIR@%$(call qstrip,$(BR2_HOST_DIR))%g" \

Is that correct?
And do you consider this acceptable?

Thanks,
Thomas
Thomas Petazzoni March 17, 2019, 3:23 p.m. UTC | #5
Hello,

On Mon, 25 Feb 2019 22:11:46 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
> Both files have a similar concept, they describe some flags/paths needed for
> compilation using respective build systems. One difference is that the meson
> file is added for external compilation, from the SDK, while the cmake file
> is used internally in Buildroot.
> 
> The 'problem' of using TARGET_FINALIZE_HOOKS for the meson file, is that it
> installs a 'host' file from target-finalize, which is conceptually incorrect
> and breaks the invariant that only TARGET_DIR is changed on a subsequent
> 'make' when everything was already built (i.e. only target-finalize is run).

In fact, I don't quite get what this commit is fixing. I don't really
understand what you call the "invariant that only TARGET_DIR is changed
on a subsequent make when everything was already built".

The current situation is not ideal because we create a file in
$(HOST_DIR) from target-finalize, but the proposed situation is also
not ideal because we create a file in $(HOST_DIR) from the staging
installation of a package. So I'd need to understand why the second
option has some advantages over the first, and that is currently
unclear.

Is it just for consistency for how the corresponding cmake file is
created ?

Best regards,

Thomas
Thomas De Schampheleire March 25, 2019, 9:47 a.m. UTC | #6
El dom., 17 mar. 2019 a las 16:23, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribió:
>
> Hello,
>
> On Mon, 25 Feb 2019 22:11:46 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
> > Both files have a similar concept, they describe some flags/paths needed for
> > compilation using respective build systems. One difference is that the meson
> > file is added for external compilation, from the SDK, while the cmake file
> > is used internally in Buildroot.
> >
> > The 'problem' of using TARGET_FINALIZE_HOOKS for the meson file, is that it
> > installs a 'host' file from target-finalize, which is conceptually incorrect
> > and breaks the invariant that only TARGET_DIR is changed on a subsequent
> > 'make' when everything was already built (i.e. only target-finalize is run).
>
> In fact, I don't quite get what this commit is fixing. I don't really
> understand what you call the "invariant that only TARGET_DIR is changed
> on a subsequent make when everything was already built".
>
> The current situation is not ideal because we create a file in
> $(HOST_DIR) from target-finalize, but the proposed situation is also
> not ideal because we create a file in $(HOST_DIR) from the staging
> installation of a package. So I'd need to understand why the second
> option has some advantages over the first, and that is currently
> unclear.

When Buildroot 'make' has completed successfully, and you run 'make'
without changes a second time, the only thing currently happening
again is target-finalize. As the name suggests, it should only touch
the TARGET_DIR. This is a principle I think we should keep.

The generation of the toolchain file for meson violated this, by
touching a host file from target-finalize, and thus also causing
changes to something else than the TARGET_DIR after a 'make' without
changes. In fact, there is totally no need to regenerate this 'static'
file more than once.

The solution I proposed fixes the described problem, but indeed uses
another apparent violation (present before in other places): touching
a host file from a staging step. Strictly speaking STAGING_DIR is
inside  HOST_DIR, but conceptually they are different indeed.
This is actually caused by the fact that the toolchain step does not
actually have a 'host' installation step, currently (because it is not
a 'host' package. Therefore, here and in other places, the 'staging'
step is used instead.


>
> Is it just for consistency for how the corresponding cmake file is
> created ?

I used the cmake thing as an example because it is very similar. It
makes sense to align, but alignment itself is not the goal.

Thanks,
Thomas

Patch
diff mbox series

diff --git a/package/meson/meson.mk b/package/meson/meson.mk
index d76541cc93..aba7382cc9 100644
--- a/package/meson/meson.mk
+++ b/package/meson/meson.mk
@@ -65,6 +65,7 @@  define HOST_MESON_INSTALL_CROSS_CONF
 	    > $(HOST_DIR)/etc/meson/cross-compilation.conf
 endef
 
-TARGET_FINALIZE_HOOKS += HOST_MESON_INSTALL_CROSS_CONF
+TOOLCHAIN_POST_INSTALL_STAGING_HOOKS += HOST_MESON_INSTALL_CROSS_CONF
+TOOLCHAIN_INSTALL_STAGING = YES
 
 $(eval $(host-python-package))