Patchwork [resend] python-native: distutils: don't use libdir, remove dead code path

login
register
mail settings
Submitter Andreas Oberritter
Date March 7, 2012, 9:07 p.m.
Message ID <1331154474-17257-1-git-send-email-obi@opendreambox.org>
Download mbox | patch
Permalink /patch/22873/
State New
Headers show

Comments

Andreas Oberritter - March 7, 2012, 9:07 p.m.
* Coming from OE-classic it was surprising that python-native now
  requires 'libdir' to be exported. Otherwise autoconf would fail
  to detect python libraries. This happend using a customized
  environment setup script to use OE's compiler and libs without
  bitbake.
* Use sys.lib instead of libdir's suffix.
* While at it, simplify redundant if/and-statments.

Signed-off-by: Andreas Oberritter <obi@opendreambox.org>
---
* This patch hasn't been commented since its first submission
  on Feb 21st.
* Original patch URL: http://patches.openembedded.org/patch/21481/

 ...2-distutils-prefix-is-inside-staging-area.patch |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)
Saul Wold - March 12, 2012, 5:58 p.m.
On 03/07/2012 01:07 PM, Andreas Oberritter wrote:
> * Coming from OE-classic it was surprising that python-native now
>    requires 'libdir' to be exported. Otherwise autoconf would fail
>    to detect python libraries. This happend using a customized
>    environment setup script to use OE's compiler and libs without
>    bitbake.
> * Use sys.lib instead of libdir's suffix.
> * While at it, simplify redundant if/and-statments.
>
> Signed-off-by: Andreas Oberritter<obi@opendreambox.org>
> ---
> * This patch hasn't been commented since its first submission
>    on Feb 21st.
> * Original patch URL: http://patches.openembedded.org/patch/21481/
>
>   ...2-distutils-prefix-is-inside-staging-area.patch |   15 +++++----------
>   1 files changed, 5 insertions(+), 10 deletions(-)
>
This will need a PR bump so the modified patch will be noticed, I know 
this patch was put in a while ago.

I am not so sure about the changes though, I have been meaning to dig 
into this, the orignial code looks strange in that it includes 
plat_specific in the else clause!  You are also dropping the EXEC_PREFIX

How have you tested this change?

Sau!


> diff --git a/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch b/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch
> index b46caf6..7b743e5 100644
> --- a/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch
> +++ b/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch
> @@ -26,26 +26,21 @@ Upstream-Status: Inappropriate [embedded specific]
>
>        if os.name == "posix":
>            if python_build:
> -@@ -115,12 +115,16 @@
> -     If 'prefix' is supplied, use it instead of sys.prefix or
> +@@ -116,11 +116,11 @@
>        sys.exec_prefix -- i.e., ignore 'plat_specific'.
>        """
> -+    lib_basename = os.getenv("libdir").split('/')[-1]
>        if prefix is None:
>   -        prefix = plat_specific and EXEC_PREFIX or PREFIX
> -+        if plat_specific:
> -+            prefix = plat_specific and os.environ['STAGING_LIBDIR'].rstrip(lib_basename)
> -+        else:
> -+            prefix = plat_specific and EXEC_PREFIX or PREFIX
> ++        prefix = plat_specific and os.environ['STAGING_LIBDIR'].rstrip(sys.lib) or PREFIX
>
>        if os.name == "posix":
>            libpython = os.path.join(prefix,
>   -                                 "lib", "python" + get_python_version())
> -+                                 lib_basename, "python" + get_python_version())
> ++                                 sys.lib, "python" + get_python_version())
>            if standard_lib:
>                return libpython
>            else:
> -@@ -216,7 +220,7 @@
> +@@ -216,7 +216,7 @@
>        else:
>            # The name of the config.h file changed in 2.2
>            config_h = 'pyconfig.h'
> @@ -54,7 +49,7 @@ Upstream-Status: Inappropriate [embedded specific]
>
>
>    def get_makefile_filename():
> -@@ -225,7 +229,7 @@
> +@@ -225,7 +225,7 @@
>            return os.path.join(os.path.dirname(os.path.realpath(sys.executable)),
>                                "Makefile")
>        lib_dir = get_python_lib(plat_specific=1, standard_lib=1)
Andreas Oberritter - March 12, 2012, 7:22 p.m.
On 12.03.2012 18:58, Saul Wold wrote:
> On 03/07/2012 01:07 PM, Andreas Oberritter wrote:
>> * Coming from OE-classic it was surprising that python-native now
>>    requires 'libdir' to be exported. Otherwise autoconf would fail
>>    to detect python libraries. This happend using a customized
>>    environment setup script to use OE's compiler and libs without
>>    bitbake.
>> * Use sys.lib instead of libdir's suffix.
>> * While at it, simplify redundant if/and-statments.
>>
>> Signed-off-by: Andreas Oberritter<obi@opendreambox.org>
>> ---
>> * This patch hasn't been commented since its first submission
>>    on Feb 21st.
>> * Original patch URL: http://patches.openembedded.org/patch/21481/
>>
>>   ...2-distutils-prefix-is-inside-staging-area.patch |   15
>> +++++----------
>>   1 files changed, 5 insertions(+), 10 deletions(-)
>>
> This will need a PR bump so the modified patch will be noticed, I know
> this patch was put in a while ago.

Right. I'm waiting before resubmitting it. See below.

> I am not so sure about the changes though, I have been meaning to dig
> into this, the orignial code looks strange in that it includes
> plat_specific in the else clause!  You are also dropping the EXEC_PREFIX

I'm dropping EXEC_PREFIX, because it's dead code.

The code from the original patch can be rewritten as:

if plat_specific:
	if plat_specific:
		prefix = os.environ['STAGING_LIBDIR'].rstrip(lib_basename)
else:
	if plat_specific:
		prefix = EXEC_PREFIX
	else:
		prefix = PREFIX

Does this make it clear?
 
> How have you tested this change?

I built from scratch and also runtime tested it, but without multilib. 

As I'm currently investing a different problem in python, digging into
python internals, I doubt that sys.lib really contains the correct
value for multilib, unless multilib builds have multiple python-natives.

I think it would be better to just drop the last element of STAGING_LIBDIR,
i.e.:

if plat_specific:
	prefix = '/'.join(os.environ['STAGING_LIBDIR'].split('/')[:-1])
else:
	prefix = PREFIX

Do you think this would be OK?

Regards,
Andreas
Richard Purdie - March 17, 2012, 12:30 a.m.
On Mon, 2012-03-12 at 20:22 +0100, Andreas Oberritter wrote:
> On 12.03.2012 18:58, Saul Wold wrote:
> > On 03/07/2012 01:07 PM, Andreas Oberritter wrote:
> >> * Coming from OE-classic it was surprising that python-native now
> >>    requires 'libdir' to be exported. Otherwise autoconf would fail
> >>    to detect python libraries. This happend using a customized
> >>    environment setup script to use OE's compiler and libs without
> >>    bitbake.
> >> * Use sys.lib instead of libdir's suffix.
> >> * While at it, simplify redundant if/and-statments.
> >>
> >> Signed-off-by: Andreas Oberritter<obi@opendreambox.org>
> >> ---
> >> * This patch hasn't been commented since its first submission
> >>    on Feb 21st.
> >> * Original patch URL: http://patches.openembedded.org/patch/21481/
> >>
> >>   ...2-distutils-prefix-is-inside-staging-area.patch |   15
> >> +++++----------
> >>   1 files changed, 5 insertions(+), 10 deletions(-)
> >>
> > This will need a PR bump so the modified patch will be noticed, I know
> > this patch was put in a while ago.
> 
> Right. I'm waiting before resubmitting it. See below.
> 
> > I am not so sure about the changes though, I have been meaning to dig
> > into this, the orignial code looks strange in that it includes
> > plat_specific in the else clause!  You are also dropping the EXEC_PREFIX
> 
> I'm dropping EXEC_PREFIX, because it's dead code.
> 
> The code from the original patch can be rewritten as:
> 
> if plat_specific:
> 	if plat_specific:
> 		prefix = os.environ['STAGING_LIBDIR'].rstrip(lib_basename)
> else:
> 	if plat_specific:
> 		prefix = EXEC_PREFIX
> 	else:
> 		prefix = PREFIX
> 
> Does this make it clear?
>  
> > How have you tested this change?
> 
> I built from scratch and also runtime tested it, but without multilib. 
> 
> As I'm currently investing a different problem in python, digging into
> python internals, I doubt that sys.lib really contains the correct
> value for multilib, unless multilib builds have multiple python-natives.
> 
> I think it would be better to just drop the last element of STAGING_LIBDIR,
> i.e.:
> 
> if plat_specific:
> 	prefix = '/'.join(os.environ['STAGING_LIBDIR'].split('/')[:-1])
> else:
> 	prefix = PREFIX
> 
> Do you think this would be OK?

Whilst we merged this patch, its now creating all kinds of issues and is
breaking builds. As a small testcase:

libdir=2 BUILD_SYS=1 HOST_SYS=2 /media/build1/poky/build/tmp/sysroots/x86_64-linux/usr/bin/python -c "import sys; from distutils import sysconfig; sys.stdout.write(sysconfig.get_python_lib(0,0,prefix='foorbar'))"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/media/build1/poky/build/tmp/sysroots/x86_64-linux/usr/lib/python2.7/distutils/sysconfig.py", line 113, in get_python_lib
    lib_basename = os.environ['STAGING_LIBDIR'].split('/')[-1]
  File "/media/build1/poky/build/tmp/sysroots/x86_64-linux/usr/lib/python2.7/UserDict.py", line 23, in __getitem__
    raise KeyError(key)
KeyError: 'STAGING_LIBDIR'

which leads to packaging warnings from xcb-proto's python module
components due to pythondir not being set to anything. Its looking
likely I'll have to revert this whilst the impact of that patch is
better understood.

Yes, I appreciate you could export STAGING_LIBDIR everywhere but we
never used to have to do that and I'd really prefer we didn't have to.

Cheers,

Richard
Richard Purdie - March 17, 2012, 11:01 a.m.
On Sat, 2012-03-17 at 00:30 +0000, Richard Purdie wrote:
> On Mon, 2012-03-12 at 20:22 +0100, Andreas Oberritter wrote:
> > On 12.03.2012 18:58, Saul Wold wrote:
> > > On 03/07/2012 01:07 PM, Andreas Oberritter wrote:
> > >> * Coming from OE-classic it was surprising that python-native now
> > >>    requires 'libdir' to be exported. Otherwise autoconf would fail
> > >>    to detect python libraries. This happend using a customized
> > >>    environment setup script to use OE's compiler and libs without
> > >>    bitbake.
> > >> * Use sys.lib instead of libdir's suffix.
> > >> * While at it, simplify redundant if/and-statments.
> > >>
> > >> Signed-off-by: Andreas Oberritter<obi@opendreambox.org>
> > >> ---
> > >> * This patch hasn't been commented since its first submission
> > >>    on Feb 21st.
> > >> * Original patch URL: http://patches.openembedded.org/patch/21481/
> > >>
> > >>   ...2-distutils-prefix-is-inside-staging-area.patch |   15
> > >> +++++----------
> > >>   1 files changed, 5 insertions(+), 10 deletions(-)
> > >>
> > > This will need a PR bump so the modified patch will be noticed, I know
> > > this patch was put in a while ago.
> > 
> > Right. I'm waiting before resubmitting it. See below.
> > 
> > > I am not so sure about the changes though, I have been meaning to dig
> > > into this, the orignial code looks strange in that it includes
> > > plat_specific in the else clause!  You are also dropping the EXEC_PREFIX
> > 
> > I'm dropping EXEC_PREFIX, because it's dead code.
> > 
> > The code from the original patch can be rewritten as:
> > 
> > if plat_specific:
> > 	if plat_specific:
> > 		prefix = os.environ['STAGING_LIBDIR'].rstrip(lib_basename)
> > else:
> > 	if plat_specific:
> > 		prefix = EXEC_PREFIX
> > 	else:
> > 		prefix = PREFIX
> > 
> > Does this make it clear?
> >  
> > > How have you tested this change?
> > 
> > I built from scratch and also runtime tested it, but without multilib. 
> > 
> > As I'm currently investing a different problem in python, digging into
> > python internals, I doubt that sys.lib really contains the correct
> > value for multilib, unless multilib builds have multiple python-natives.
> > 
> > I think it would be better to just drop the last element of STAGING_LIBDIR,
> > i.e.:
> > 
> > if plat_specific:
> > 	prefix = '/'.join(os.environ['STAGING_LIBDIR'].split('/')[:-1])
> > else:
> > 	prefix = PREFIX
> > 
> > Do you think this would be OK?
> 
> Whilst we merged this patch, its now creating all kinds of issues and is
> breaking builds. As a small testcase:
> 
> libdir=2 BUILD_SYS=1 HOST_SYS=2 /media/build1/poky/build/tmp/sysroots/x86_64-linux/usr/bin/python -c "import sys; from distutils import sysconfig; sys.stdout.write(sysconfig.get_python_lib(0,0,prefix='foorbar'))"
> Traceback (most recent call last):
>   File "<string>", line 1, in <module>
>   File "/media/build1/poky/build/tmp/sysroots/x86_64-linux/usr/lib/python2.7/distutils/sysconfig.py", line 113, in get_python_lib
>     lib_basename = os.environ['STAGING_LIBDIR'].split('/')[-1]
>   File "/media/build1/poky/build/tmp/sysroots/x86_64-linux/usr/lib/python2.7/UserDict.py", line 23, in __getitem__
>     raise KeyError(key)
> KeyError: 'STAGING_LIBDIR'
> 
> which leads to packaging warnings from xcb-proto's python module
> components due to pythondir not being set to anything. Its looking
> likely I'll have to revert this whilst the impact of that patch is
> better understood.
> 
> Yes, I appreciate you could export STAGING_LIBDIR everywhere but we
> never used to have to do that and I'd really prefer we didn't have to.

I've tracked down what is happening and why I'm getting very confused
over the builds. The proposed automake update changes the python m4
macros in a way which is incompatible with the way we're been using
them. The changes would imply that we'd have to inherit distutils for
every recipe using the python macros since otherwise the exports like
BUILD_SYS and HOST_SYS are not available.

I'm therefore not going to revert this patch but ensure we look into the
automake update to resolve the problems there.

Sorry about the confusion.

Cheers,

Richard

Patch

diff --git a/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch b/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch
index b46caf6..7b743e5 100644
--- a/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch
+++ b/meta/recipes-devtools/python/python-native/12-distutils-prefix-is-inside-staging-area.patch
@@ -26,26 +26,21 @@  Upstream-Status: Inappropriate [embedded specific]
  
      if os.name == "posix":
          if python_build:
-@@ -115,12 +115,16 @@
-     If 'prefix' is supplied, use it instead of sys.prefix or
+@@ -116,11 +116,11 @@
      sys.exec_prefix -- i.e., ignore 'plat_specific'.
      """
-+    lib_basename = os.getenv("libdir").split('/')[-1]
      if prefix is None:
 -        prefix = plat_specific and EXEC_PREFIX or PREFIX
-+        if plat_specific:
-+            prefix = plat_specific and os.environ['STAGING_LIBDIR'].rstrip(lib_basename)
-+        else:
-+            prefix = plat_specific and EXEC_PREFIX or PREFIX
++        prefix = plat_specific and os.environ['STAGING_LIBDIR'].rstrip(sys.lib) or PREFIX
  
      if os.name == "posix":
          libpython = os.path.join(prefix,
 -                                 "lib", "python" + get_python_version())
-+                                 lib_basename, "python" + get_python_version())
++                                 sys.lib, "python" + get_python_version())
          if standard_lib:
              return libpython
          else:
-@@ -216,7 +220,7 @@
+@@ -216,7 +216,7 @@
      else:
          # The name of the config.h file changed in 2.2
          config_h = 'pyconfig.h'
@@ -54,7 +49,7 @@  Upstream-Status: Inappropriate [embedded specific]
  
  
  def get_makefile_filename():
-@@ -225,7 +229,7 @@
+@@ -225,7 +225,7 @@
          return os.path.join(os.path.dirname(os.path.realpath(sys.executable)),
                              "Makefile")
      lib_dir = get_python_lib(plat_specific=1, standard_lib=1)