[v14,4/5] package/pkg-meson.mk: set Python sysconfigdata
diff mbox series

Message ID 20191021200438.97453-5-aduskett@gmail.com
State New
Headers show
Series
  • gobject-introspection: New package
Related show

Commit Message

Adam Duskett Oct. 21, 2019, 8:04 p.m. UTC
From: Adam Duskett <Aduskett@gmail.com>

meson has support for Python dependencies, to e.g. build Python modules.
It parses Python's sysconfig module to find the appropriate
configuration variables. However, the sysconfig module is the one for
the host, _not_ for the target. Therefore, wrong values may be set. Many
of them get overridden anyway by the cross-compilation.conf. One
particular one that is problematic is that it uses the python-XXX.pc
pkg-config file for the host instead of the target, because it
explicitly sets the PKG_CONFIG_LIBDIR found in sysconfig.

Use the same trick as for python package: pass PYTHONPATH,
PYTHONNOUSERSITE and _PYTHON_SYSCONFIGDATA_NAME in the environment to
point to staging instead of host. This ensures the sysconfig from the
target is used instead of the one from the host.

An alternative would be to patch meson to not use the PKG_CONFIG_LIBDIR
from sysconfig. However, it is likely that other problems will pop up at
some point because of using the wrong sysconfig.

Note that this approach will still break things when meson needs to
build something for the host during a target build.

Signed-off-by: Adam Duskett <Aduskett@gmail.com>
---
Changes v1 -> v14:
  - Add this patch to the series.

 package/pkg-meson.mk | 3 +++
 1 file changed, 3 insertions(+)

Comments

Thomas Petazzoni Oct. 22, 2019, 8:59 p.m. UTC | #1
On Mon, 21 Oct 2019 13:04:37 -0700
aduskett@gmail.com wrote:

> +	_PYTHON_SYSCONFIGDATA_NAME="$$(if $$(BR2_PACKAGE_PYTHON3),$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)" \

Why do we need this conditional on BR2_PACKAGE_PYTHON3 ?
package/pkg-python.mk is using PKG_PYTHON_SYSCONFIGDATA_NAME
unconditionally, regardless of Python 2.x vs. Python 3.x. Could you
explain this ?

Thanks!

Thomas
Arnout Vandecappelle Oct. 22, 2019, 9:58 p.m. UTC | #2
Note that this patch was authored by me, Adam accidentally took over authorship
(which is why he said on IRC he needs to send a v15).

On 22/10/2019 22:59, Thomas Petazzoni wrote:
> On Mon, 21 Oct 2019 13:04:37 -0700
> aduskett@gmail.com wrote:
> 
>> +	_PYTHON_SYSCONFIGDATA_NAME="$$(if $$(BR2_PACKAGE_PYTHON3),$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)" \
> 
> Why do we need this conditional on BR2_PACKAGE_PYTHON3 ?
> package/pkg-python.mk is using PKG_PYTHON_SYSCONFIGDATA_NAME
> unconditionally, regardless of Python 2.x vs. Python 3.x. Could you
> explain this ?

 I can :-)

 It's a mistake. I noticed that PKG_PYTHON_SYSCONFIGDATA_NAME was left empty for
python2, so I made sure it got the correct value (in Python2 there is only one
_sysconfigdata). However, python2 doesn't have the _PYTHON_SYSCONFIGDATA_NAME
variable so it's meaningless.

 Should be tested if things work OK with python2 on target, but I expect they do.

 Regards,
 Arnout
Arnout Vandecappelle Oct. 22, 2019, 10:19 p.m. UTC | #3
On 22/10/2019 23:58, Arnout Vandecappelle wrote:
>  Note that this patch was authored by me, Adam accidentally took over authorship
> (which is why he said on IRC he needs to send a v15).
> 
> On 22/10/2019 22:59, Thomas Petazzoni wrote:
>> On Mon, 21 Oct 2019 13:04:37 -0700
>> aduskett@gmail.com wrote:
>>
>>> +	_PYTHON_SYSCONFIGDATA_NAME="$$(if $$(BR2_PACKAGE_PYTHON3),$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)" \
>>
>> Why do we need this conditional on BR2_PACKAGE_PYTHON3 ?
>> package/pkg-python.mk is using PKG_PYTHON_SYSCONFIGDATA_NAME
>> unconditionally, regardless of Python 2.x vs. Python 3.x. Could you
>> explain this ?
> 
>  I can :-)
> 
>  It's a mistake. I noticed that PKG_PYTHON_SYSCONFIGDATA_NAME was left empty for
> python2, so I made sure it got the correct value (in Python2 there is only one
> _sysconfigdata). However, python2 doesn't have the _PYTHON_SYSCONFIGDATA_NAME
> variable so it's meaningless.
> 
>  Should be tested if things work OK with python2 on target, but I expect they do.

 I hadn't read Adam's remark in the cover letter yet, about how this patch
breaks the build of libglib2.

 I suspect that we shold not set _PYTHON_SYSCONFIGDATA_NAME if python3 is not
selected. Indeed, python3 *will* observe this environment variable, but if
target python3 is not selected, it will be empty. meson will do checks for
host-python, and setting that environment variable will break the check (which
is BTW an example of the comment I made in the commit log of this patch: "Note
that this approach will still break things when meson needs to build something
for the host during a target build."). libglib2 indeed does use python3 during
the build, so it asks meson to check for it, but since it doesn't try to build C
modules for the host, it still works even with a wrong _sysconfigdata.

 Adam, could you test if this works better?

+ 	$$(if
$$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME)")
\



 Regards,
 Arnout
Adam Duskett Oct. 23, 2019, 7:48 p.m. UTC | #4
Hey Arnout;

I absolutely can!
I will do so today, and if it works, I will send a V15 with you as the
author of the patch.

Thanks!

Adam

On Tue, Oct 22, 2019 at 3:19 PM Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
>
> On 22/10/2019 23:58, Arnout Vandecappelle wrote:
> >  Note that this patch was authored by me, Adam accidentally took over authorship
> > (which is why he said on IRC he needs to send a v15).
> >
> > On 22/10/2019 22:59, Thomas Petazzoni wrote:
> >> On Mon, 21 Oct 2019 13:04:37 -0700
> >> aduskett@gmail.com wrote:
> >>
> >>> +   _PYTHON_SYSCONFIGDATA_NAME="$$(if $$(BR2_PACKAGE_PYTHON3),$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)" \
> >>
> >> Why do we need this conditional on BR2_PACKAGE_PYTHON3 ?
> >> package/pkg-python.mk is using PKG_PYTHON_SYSCONFIGDATA_NAME
> >> unconditionally, regardless of Python 2.x vs. Python 3.x. Could you
> >> explain this ?
> >
> >  I can :-)
> >
> >  It's a mistake. I noticed that PKG_PYTHON_SYSCONFIGDATA_NAME was left empty for
> > python2, so I made sure it got the correct value (in Python2 there is only one
> > _sysconfigdata). However, python2 doesn't have the _PYTHON_SYSCONFIGDATA_NAME
> > variable so it's meaningless.
> >
> >  Should be tested if things work OK with python2 on target, but I expect they do.
>
>  I hadn't read Adam's remark in the cover letter yet, about how this patch
> breaks the build of libglib2.
>
>  I suspect that we shold not set _PYTHON_SYSCONFIGDATA_NAME if python3 is not
> selected. Indeed, python3 *will* observe this environment variable, but if
> target python3 is not selected, it will be empty. meson will do checks for
> host-python, and setting that environment variable will break the check (which
> is BTW an example of the comment I made in the commit log of this patch: "Note
> that this approach will still break things when meson needs to build something
> for the host during a target build."). libglib2 indeed does use python3 during
> the build, so it asks meson to check for it, but since it doesn't try to build C
> modules for the host, it still works even with a wrong _sysconfigdata.
>
>  Adam, could you test if this works better?
>
> +       $$(if
> $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME)")
> \
>
>
>
>  Regards,
>  Arnout
Adam Duskett Oct. 23, 2019, 8:42 p.m. UTC | #5
Arnout:

On Tue, Oct 22, 2019 at 3:19 PM Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
>
> On 22/10/2019 23:58, Arnout Vandecappelle wrote:
> >  Note that this patch was authored by me, Adam accidentally took over authorship
> > (which is why he said on IRC he needs to send a v15).
> >
> > On 22/10/2019 22:59, Thomas Petazzoni wrote:
> >> On Mon, 21 Oct 2019 13:04:37 -0700
> >> aduskett@gmail.com wrote:
> >>
> >>> +   _PYTHON_SYSCONFIGDATA_NAME="$$(if $$(BR2_PACKAGE_PYTHON3),$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)" \
> >>
> >> Why do we need this conditional on BR2_PACKAGE_PYTHON3 ?
> >> package/pkg-python.mk is using PKG_PYTHON_SYSCONFIGDATA_NAME
> >> unconditionally, regardless of Python 2.x vs. Python 3.x. Could you
> >> explain this ?
> >
> >  I can :-)
> >
> >  It's a mistake. I noticed that PKG_PYTHON_SYSCONFIGDATA_NAME was left empty for
> > python2, so I made sure it got the correct value (in Python2 there is only one
> > _sysconfigdata). However, python2 doesn't have the _PYTHON_SYSCONFIGDATA_NAME
> > variable so it's meaningless.
> >
> >  Should be tested if things work OK with python2 on target, but I expect they do.
>
>  I hadn't read Adam's remark in the cover letter yet, about how this patch
> breaks the build of libglib2.
>
>  I suspect that we shold not set _PYTHON_SYSCONFIGDATA_NAME if python3 is not
> selected. Indeed, python3 *will* observe this environment variable, but if
> target python3 is not selected, it will be empty. meson will do checks for
> host-python, and setting that environment variable will break the check (which
> is BTW an example of the comment I made in the commit log of this patch: "Note
> that this approach will still break things when meson needs to build something
> for the host during a target build."). libglib2 indeed does use python3 during
> the build, so it asks meson to check for it, but since it doesn't try to build C
> modules for the host, it still works even with a wrong _sysconfigdata.
>
>  Adam, could you test if this works better?
>
> +       $$(if
> $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME)")
> \
$$(if $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)"
Also does not work, because the target python3 may not yet be built.

>
>
>
>  Regards,
>  Arnout
Thomas Petazzoni Oct. 23, 2019, 8:50 p.m. UTC | #6
On Wed, 23 Oct 2019 13:42:54 -0700
Adam Duskett <aduskett@gmail.com> wrote:

> >  Adam, could you test if this works better?
> >
> > +       $$(if
> > $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME)")
> > \  
> $$(if $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)"
> Also does not work, because the target python3 may not yet be built.

But that is totally unrelated to the _sysconfigdata "fallback" that I
was criticizing. The issue of "you're passing
_PYTHON_SYSCONFIG_DATA_NAME regardless of whether Python was built
before or not" was already present in your initial submission.

Thomas
Arnout Vandecappelle Oct. 23, 2019, 9:03 p.m. UTC | #7
On 23/10/2019 22:50, Thomas Petazzoni wrote:
> On Wed, 23 Oct 2019 13:42:54 -0700
> Adam Duskett <aduskett@gmail.com> wrote:
> 
>>>  Adam, could you test if this works better?
>>>
>>> +       $$(if
>>> $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME)")
>>> \  
>> $$(if $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)"
>> Also does not work, because the target python3 may not yet be built.

 Ugh indeed, another side effect of this environment being used for both "host"
python and "cross" python...

 I'm starting to think this is not such a good solution after all...


> But that is totally unrelated to the _sysconfigdata "fallback" that I
> was criticizing. The issue of "you're passing
> _PYTHON_SYSCONFIG_DATA_NAME regardless of whether Python was built
> before or not" was already present in your initial submission.

 With "initial submission", do you mean *my* initial submission (i.e. this patch
4/5 from v14), or Adam's initial submission which patched meson itself and which
was taken over from yocto? The latter did not actually change sysconfigdata at
all; instead it patched meson to not make use of the pkg_config_libdir that
comes out of sysconfigdata (which is I think the only thing from sysconfigdata
that is actually used by meson).

 I'll dive into the meson code again to see if I can find something better...

 Regards,
 Arnout
Arnout Vandecappelle Oct. 23, 2019, 9:27 p.m. UTC | #8
On 23/10/2019 23:03, Arnout Vandecappelle wrote:
> 
> 
> On 23/10/2019 22:50, Thomas Petazzoni wrote:
>> On Wed, 23 Oct 2019 13:42:54 -0700
>> Adam Duskett <aduskett@gmail.com> wrote:
>>
>>>>  Adam, could you test if this works better?
>>>>
>>>> +       $$(if
>>>> $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME)")
>>>> \  
>>> $$(if $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)"
>>> Also does not work, because the target python3 may not yet be built.
> 
>  Ugh indeed, another side effect of this environment being used for both "host"
> python and "cross" python...
> 
>  I'm starting to think this is not such a good solution after all...
> 
> 
>> But that is totally unrelated to the _sysconfigdata "fallback" that I
>> was criticizing. The issue of "you're passing
>> _PYTHON_SYSCONFIG_DATA_NAME regardless of whether Python was built
>> before or not" was already present in your initial submission.
> 
>  With "initial submission", do you mean *my* initial submission (i.e. this patch
> 4/5 from v14), or Adam's initial submission which patched meson itself and which
> was taken over from yocto? The latter did not actually change sysconfigdata at
> all; instead it patched meson to not make use of the pkg_config_libdir that
> comes out of sysconfigdata (which is I think the only thing from sysconfigdata
> that is actually used by meson).
> 
>  I'll dive into the meson code again to see if I can find something better...

 OK, I *think* I've come up with an alternative (but I'm sure as hell not going
to implement it :-).


 Essentially, it's the same issue as for pkg-config: we should distinguish
between the host version and the cross version (i.e. executable that can run on
the host but that returns results for the target). Meson is very explicit about
such a distinction, so anything pertaining to cross-compilation should be passed
through cross-compilation.conf instead of in the environment.

 As far as I can see in the source code, we can in fact set a
"cross-compilation" python in the [binaries] section. So, we could create a
python wrapper that sets those environment variables, and point to that python
wrapper in the cross-compilation.conf.

 And of course, we can use the same approach in pkg-python.mk: set
$(2)_PYTHON_INTERPRETER to the python wrapper instead of $(HOST_DIR)/bin/python.


 Regards,
 Arnout
Adam Duskett Oct. 24, 2019, 4:22 p.m. UTC | #9
This might be a good solution, but it is beyond my skillset to
implement it properly.

If the rest of the series looks good, perhaps we could come up with a
solution during the developer conference and them
implement GOI?

Regards;
Adam

On Wed, Oct 23, 2019 at 2:27 PM Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
>
> On 23/10/2019 23:03, Arnout Vandecappelle wrote:
> >
> >
> > On 23/10/2019 22:50, Thomas Petazzoni wrote:
> >> On Wed, 23 Oct 2019 13:42:54 -0700
> >> Adam Duskett <aduskett@gmail.com> wrote:
> >>
> >>>>  Adam, could you test if this works better?
> >>>>
> >>>> +       $$(if
> >>>> $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME)")
> >>>> \
> >>> $$(if $$(BR2_PACKAGE_PYTHON3),_PYTHON_SYSCONFIGDATA_NAME="$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)"
> >>> Also does not work, because the target python3 may not yet be built.
> >
> >  Ugh indeed, another side effect of this environment being used for both "host"
> > python and "cross" python...
> >
> >  I'm starting to think this is not such a good solution after all...
> >
> >
> >> But that is totally unrelated to the _sysconfigdata "fallback" that I
> >> was criticizing. The issue of "you're passing
> >> _PYTHON_SYSCONFIG_DATA_NAME regardless of whether Python was built
> >> before or not" was already present in your initial submission.
> >
> >  With "initial submission", do you mean *my* initial submission (i.e. this patch
> > 4/5 from v14), or Adam's initial submission which patched meson itself and which
> > was taken over from yocto? The latter did not actually change sysconfigdata at
> > all; instead it patched meson to not make use of the pkg_config_libdir that
> > comes out of sysconfigdata (which is I think the only thing from sysconfigdata
> > that is actually used by meson).
> >
> >  I'll dive into the meson code again to see if I can find something better...
>
>  OK, I *think* I've come up with an alternative (but I'm sure as hell not going
> to implement it :-).
>
>
>  Essentially, it's the same issue as for pkg-config: we should distinguish
> between the host version and the cross version (i.e. executable that can run on
> the host but that returns results for the target). Meson is very explicit about
> such a distinction, so anything pertaining to cross-compilation should be passed
> through cross-compilation.conf instead of in the environment.
>
>  As far as I can see in the source code, we can in fact set a
> "cross-compilation" python in the [binaries] section. So, we could create a
> python wrapper that sets those environment variables, and point to that python
> wrapper in the cross-compilation.conf.
>
>  And of course, we can use the same approach in pkg-python.mk: set
> $(2)_PYTHON_INTERPRETER to the python wrapper instead of $(HOST_DIR)/bin/python.
>
>
>  Regards,
>  Arnout

Patch
diff mbox series

diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk
index 184a22a44a..b6360fb738 100644
--- a/package/pkg-meson.mk
+++ b/package/pkg-meson.mk
@@ -84,6 +84,9 @@  define $(2)_CONFIGURE_CMDS
 	    ) \
 	    package/meson/cross-compilation.conf.in \
 	    > $$($$(PKG)_SRCDIR)/build/cross-compilation.conf
+	_PYTHON_SYSCONFIGDATA_NAME="$$(if $$(BR2_PACKAGE_PYTHON3),$$(PKG_PYTHON_SYSCONFIGDATA_NAME),_sysconfigdata)" \
+	PYTHONPATH="$$(if $$(BR2_PACKAGE_PYTHON3),$$(PYTHON3_PATH),$$(PYTHON_PATH))" \
+	PYTHONNOUSERSITE=1 \
 	PATH=$$(BR_PATH) $$($$(PKG)_CONF_ENV) $$(MESON) \
 		--prefix=/usr \
 		--libdir=lib \