classification
Title: sysconfig confused by relative paths
Type: behavior Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, chris.jerdonek, eric.araujo, fdrake, georg.brandl, ned.deily, python-dev, ronaldoussoren, sbt, tarek
Priority: normal Keywords: patch

Created on 2012-07-16 10:01 by sbt, last changed 2012-08-14 00:01 by sbt. This issue is now closed.

Files
File name Uploaded Description Edit
sysconf.patch sbt, 2012-07-16 23:06 review
sysconf.patch sbt, 2012-07-17 13:37 review
sysconf.patch sbt, 2012-07-17 23:13 review
sysconf.patch sbt, 2012-07-18 12:21 review
sysconf.patch sbt, 2012-07-18 12:28 review
Messages (27)
msg165581 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-16 10:01
With unix, and a source build, sysconfig.get_config_var('srcdir') and sysconfig.get_path('include') misbehave:

  user@mint-vm ~/Repos/cpython $ cd /
  user@mint-vm / $ ~/Repos/cpython/python 
  Python 3.3.0b1 (default:671894ae19a2, Jul 16 2012, 10:43:27) 
  [GCC 4.5.2] on linux
  Type "help", "copyright", "credits" or "license" for more information.
  >>> import sysconfig
  >>> sysconfig.get_config_var('srcdir')
  '/'
  >>> sysconfig.get_path('include')
  '//Include'

The problem seems to be the else clause of

        if 'srcdir' not in _CONFIG_VARS:
            _CONFIG_VARS['srcdir'] = _PROJECT_BASE
        else:
            _CONFIG_VARS['srcdir'] = _safe_realpath(_CONFIG_VARS['srcdir'])

The else clause (in slightly modified form) was introduced in 356d0ea8ea34, and it turns a relative path into an absolute path, using the current working directory as a base.

For me, simply removing the line allows the absolute path to be calculated correctly a few lines later on.

I don't know what problem 356d0ea8ea34 was intended to fix.
msg165620 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-16 14:43
I don't recall what the issue was the resulted in the check-in that you mention. Sadly enough it is not-trivial to find that check-in I mention due to the migration from Subversion to Mercurial.

How was python itself configured (configure command line)? What OS are you running on? Given the shell prompt I'd say Mint Linux. And finally, is '~/Repos/cpython/python' a checkout or installed location (that is, did you run 'make install')?

Based on code inspection I'd say that your change is correct, and my change only works when running from the build directory.
msg165628 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-16 15:47
> I don't recall what the issue was the resulted in the check-in that you
> mention.

I think it was http://bugs.python.org/issue8577.  The issue was about having srcdir != builddir.  The initial patch caused a test failure, and 356d0ea8ea34 fixed (or at least silenced) the failure.

> How was python itself configured (configure command line)?

I tried it with both

   ./configure
   make

and

   mdkir release
   cd release
   ../configure
   make

It fails with both.

> What OS are you running on? Given the shell prompt I'd say Mint Linux. 

Yes (in a VM).

> And finally, is '~/Repos/cpython/python' a checkout or installed 
> location (that is, did you run 'make install')?

It is a checkout.

The argument passed to _safe_realpath() (either "." or ".." in my case) should be interpreted relative to the directory containing the Makefile.  But os.path.realpath() assumes that its argument is either absolute or relative to os.getcwd().  It therefore returns the wrong absolute path.
msg165632 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-16 16:23
It is actually simple to find the revision: http://hg.python.org/lookup/r81999 :)
msg165633 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-16 16:25
I have to add that I’m quite confused by srcdir vs. projectbase.  There are a handful of open bugs related to sysconfig and built but uninstalled Pythons, and many commits changing code to use srcdir or projectbase after empirical testing or buildbot failures. :/
msg165634 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-16 16:41
srcdir vs. project base is quite easy: srcdir is the directory containing the source files, the project base is where you ran configure. These are the same if you run configure in the root of a checkout, but don't have to be, for example when you do:

$ mkdir buildroot
$ cd buildroot
$ ../configure # ...

Now de base directory is the 'buildroot' directory, while srcdir is its parent directory.
msg165654 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-16 20:36
Issue 15322 is a recently filed bug regarding srcdir: "sysconfig.get_config_var('srcdir') returns unexpected value"
msg165664 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-16 23:06
In the attached patch _safe_realpath() is only called after calculating the absolute path for srcdir.
msg165720 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-17 13:37
Updated patch which clarifies that for an installed python on a posix system, get_config_var('srcdir') == get_path('stdlib').

Eg, if sys.executable == '/usr/local/bin/python3.3' then get_config_var('srcdir') == '/usr/local/lib/python3.3'.

This is a little arbitrary, but seems as sensible a value as any other.  (Or maybe it would be better to have get_config_var('srcdir') == None instead.)
msg165729 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-17 20:36
> This is a little arbitrary, but seems as sensible a value as any other.
> (Or maybe it would be better to have get_config_var('srcdir') == None
> instead.)

'srcdir' is problematic for any installed build.  Once "make install" has been performed, there is no obligation to keep the original source and build dirs around anymore.  For Pythons installed on other machines via a binary installer, like the python.org OS X ones, some of the paths returned by sysconfig are not meaningful on the installed machine.  For an OS X framework build, the Makefile and friends are copied into a 'config' directory that is created within the installed framework. So, with the patch, the contents of that directory (returned by the patched 'srcdir' value) is:
['Makefile',
 'Setup',
 'Setup.config',
 'Setup.local',
 'config.c',
 'config.c.in',
 'install-sh',
 'libpython3.3.a',
 'libpython3.3.dylib',
 'makesetup',
 'python.o']

Is that a meaningful surrogate for the build 'srcdir'?

Beyond that, I don't understand why the patch behavior depends on whether the srcdir is an absolute path or not.  I often use absolute paths to configure a build.  If not in a build directory, 'srcdir' should always return either the proposed value based on what get_makefile_filename() returns or it should return None.  I don't have a strong opinion at the moment about which.  One factor should be an inspection and running of all of the tests.  A quick grep of Lib/ didn't find many references to 'srcdir'.

One other point: distutils still has its own copy of sysconfig.  Strong consideration should be given to making a similar change there.
msg165735 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-17 23:13
> Beyond that, I don't understand why the patch behavior depends on 
> whether the srcdir is an absolute path or not.

The os.path.isabs() test is actually redundant since os.path.join(base, srcdir) == srcdir if srcdir is an absolute path.  I only added that test to be explicit about what happens if the original value is an absolute path. 

> I often use absolute paths to configure a build.  If not in a build 
> directory, 'srcdir' should always return either the proposed value 
> based on what get_makefile_filename() returns or it should return 
> None.

Removing the isabs() test would not change the calculated value as I indicated above.

Using None would be the sanest option, but I don't know if any current code depends on the value being a string.

> One other point: distutils still has its own copy of sysconfig.  
> Strong consideration should be given to making a similar change there.

For a source build, distutils.sysconfig.get_config_var('srcdir') gives the right answer as an absolute path, *except* when the current working directory contains sys.executable.  I think it should always return an absolute path.  The fact that currently it does not probably explains the "mysteriously" comment below from distutils/test/support.py:

def _get_xxmodule_path():
    srcdir = sysconfig.get_config_var('srcdir')
    candidates = [
        # use installed copy if available
        os.path.join(os.path.dirname(__file__), 'xxmodule.c'),
        # otherwise try using copy from build directory
        os.path.join(srcdir, 'Modules', 'xxmodule.c'),
        # srcdir mysteriously can be $srcdir/Lib/distutils/tests when
        # this file is run from its parent directory, so walk up the
        # tree to find the real srcdir
        os.path.join(srcdir, '..', '..', '..', 'Modules', 'xxmodule.c'),
    ]
    for path in candidates:
        if os.path.exists(path):
            return path

BTW, I was wrong in my earlier message when I claimed that srcdir == get_path('stdlib') for an installed python.  That is only true if the relative srcdir is '..'.  The attached patch removes that check from the unit tests.
msg165737 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-18 00:03
Upon further consideration, while it sounds appealing, perhaps returning None is not the better choice. As far as I know, up until now all config vars are either of type str or type int.  Adding None to the mix might be asking for trouble.

With no patch, running from an installed framework build:
>>> sys.executable
'/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/bin/python3.3'
>>> import sysconfig as sc, distutils.sysconfig as dsc
>>> sc.is_python_build()
False
>>> sc.get_config_var('srcdir')
'/py/dev/default/b10.7_t10.7_x4.3_cclang_d'
>>> dsc.get_config_var('srcdir')
'.'

With the latest patch, as above:
>>> sys.executable
'/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/bin/python3.3'
>>> import sysconfig as sc, distutils.sysconfig as dsc
>>> sc.is_python_build()
False
>>> sc.get_config_var('srcdir')
'/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/3.3/lib/python3.3/config-3.3dm'
>>> dsc.get_config_var('srcdir')
'.'

The patched results looks OK except, of course, for the discrepancy with distutils.sysconfig.  I think we should fix that and get this all into 3.3.0.  Otherwise we'll be stuck with the current broken behavior until 3.4 at the earliest.

One nit comment on the currrent patch: s/"will not effect it"/"will not affect it" (one of those gotchas in English).
msg165738 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-18 00:15
I should have noted that, in the above tests, distutils.sysconfig.get_makefile_filename() is already doing the right thing:

>>> dsc.get_makefile_filename()
'/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/root/Library/Frameworks/pytest_10_7.framework/Versions/3.3/lib/python3.3/config-3.3dm/Makefile'
[70312 refs]
>>> sc.get_makefile_filename() == dsc.get_makefile_filename()
True
msg165739 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-18 00:30
Agree on fixing this for 3.3, if it’s not possible to backport to all stable branches.  Will review.
msg165761 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-18 12:21
Here is an updated patch which also modifies distutils.sysconfig.

For "installed" pythons on posix systems it makes

    get_config_var('srcdir') == os.path.dirname(get_makefile_filename())

(The older patches would give the same result only if the relative srcdir was '.')
msg165762 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-18 12:28
Forgot to remove some unnecessary code in patch.
msg165763 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-18 12:30
I'd make get_config_var('srcdir') to be None for installed systems, because the source tree is not available there.

The advantage of making the value None is that this is easy to test for and would quickly break path manipulation code that tries to construct a path to a specific file in the source tree.  

Making the value to be the name of the directory containing Makefile would still break code that tries to access files relative to the source tree, but could that breakage could be harder to track.

BTW. sysconfig really needs a better description of which variables are supposed to be available, it currently exports every definition in the Makefile and pyconfig.h, and not all of those definitions are usable outside of CPython's build process.
msg165844 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-19 15:16
> I'd make get_config_var('srcdir') to be None for installed systems, 
> because the source tree is not available there.

While playing with a version of the patch which returns None for non-source builds, I found that distutils.support._get_xxmodule_path() depends on get_config_var('srcdir') always returning a string:

def _get_xxmodule_path():
    srcdir = sysconfig.get_config_var('srcdir')
    candidates = [
        os.path.join(os.path.dirname(__file__), 'xxmodule.c'),
        os.path.join(srcdir, 'Modules', 'xxmodule.c'),
        os.path.join(srcdir, '..', '..', '..', 'Modules', 'xxmodule.c'),
    ]
    for path in candidates:
        if os.path.exists(path):
            return path

It is easy enough to modify _get_xxmodule_path() to check for None.  But this does suggest that returning None might break some currently working 3rd party code.
msg166481 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-26 13:17
Any objection if I commit the last patch before the next beta?  This is the one which on installed Pythons have

    get_config_var('srcdir') == os.path.dirname(get_makefile_filename())

on posix systems.
msg166506 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-26 19:43
LGTM
msg166522 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-26 21:29
Does get_config_var('srcdir') always return a string or sometimes None?
msg166525 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-26 22:31
> Does get_config_var('srcdir') always return a string or sometimes None?

Always a string.

distutils.support._get_xxmodule_path() is one place which (currently) would throw an exception if it returned None.
msg166553 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-07-27 11:08
New changeset d2f448dbc30d by Richard Oudkerk in branch 'default':
Issue #15364: Fix sysconfig.get_config_var('srcdir') to be an absolute path.
http://hg.python.org/cpython/rev/d2f448dbc30d
msg166566 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-27 13:54
Okay, so LGTM.  Let’s ask the RMs if this is a new behavior or a bugfix.
msg166569 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-27 14:10
The current patch should be fine, although it is IMHO not the long-term solution. For installed python's get_config_var('srcdir') still lies, but now at least returns slightly more sane path.

As I noted before the real problem is that it is not clear which config_vars are usable in which situations. One example is 'srcdir': this value is only useful in the build tree and there currently is no clean way to signal that you are retrieving a value that is not useful.  Another example is 'RUNSHARED', that's also only useable when building python.

I'm not sure what the right solution for the larger issue is. It might be good enough to document generally useful config_vars and warn that other's might exist but might not be safe to use, on the other hand it might be better to explicitly reduce the set of config_vars to something sane (and document which those are and when they can be used)
msg166628 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-07-28 06:38
New changeset c2618b916081 by Ned Deily in branch 'default':
Issue #15364: Fix test_srcdir for the installed case.
http://hg.python.org/cpython/rev/c2618b916081
msg166753 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-07-29 12:37
> One example is 'srcdir': this value is only useful in the build tree and 
> there currently is no clean way to signal that you are retrieving a 
> value that is not useful.

In the particular case of 'srcdir', sysconfig.is_python_build() tells you whether the value is useful.

Concerns about sysconfig often returning values which are meaningless for the current configuration can be discussed in another issue.

Unless there are objections I will close.
History
Date User Action Args
2012-08-14 00:01:57sbtlinkissue15322 superseder
2012-08-14 00:01:25sbtsetstatus: open -> closed
2012-07-29 12:37:53sbtsetresolution: fixed
messages: + msg166753
stage: commit review -> resolved
2012-07-28 06:38:19python-devsetmessages: + msg166628
2012-07-27 14:10:50ronaldoussorensetmessages: + msg166569
2012-07-27 13:54:41eric.araujosetnosy: + georg.brandl, benjamin.peterson

messages: + msg166566
versions: + Python 3.4, - Python 3.3
2012-07-27 11:08:44python-devsetnosy: + python-dev
messages: + msg166553
2012-07-26 22:31:50sbtsetmessages: + msg166525
2012-07-26 21:29:12eric.araujosetmessages: + msg166522
2012-07-26 19:43:15ned.deilysetmessages: + msg166506
stage: patch review -> commit review
2012-07-26 13:17:22sbtsetmessages: + msg166481
2012-07-19 15:16:27sbtsetmessages: + msg165844
2012-07-18 12:30:58ronaldoussorensetmessages: + msg165763
2012-07-18 12:28:38sbtsetfiles: + sysconf.patch

messages: + msg165762
2012-07-18 12:21:54sbtsetfiles: + sysconf.patch

messages: + msg165761
2012-07-18 00:30:29eric.araujosetmessages: + msg165739
versions: + Python 3.3
2012-07-18 00:15:08ned.deilysetmessages: + msg165738
2012-07-18 00:03:45ned.deilysetmessages: + msg165737
2012-07-17 23:13:43sbtsetfiles: + sysconf.patch

messages: + msg165735
2012-07-17 20:36:41ned.deilysetnosy: + ned.deily
messages: + msg165729
2012-07-17 13:37:19sbtsetfiles: + sysconf.patch

messages: + msg165720
stage: needs patch -> patch review
2012-07-16 23:06:11sbtsetfiles: + sysconf.patch
keywords: + patch
messages: + msg165664
2012-07-16 20:36:48chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg165654
2012-07-16 16:41:58ronaldoussorensetmessages: + msg165634
2012-07-16 16:25:23eric.araujosetnosy: + fdrake, tarek
messages: + msg165633
2012-07-16 16:23:05eric.araujosetmessages: + msg165632
2012-07-16 15:59:05pitrousetnosy: + eric.araujo
2012-07-16 15:47:36sbtsetmessages: + msg165628
2012-07-16 14:43:28ronaldoussorensetmessages: + msg165620
2012-07-16 10:25:08ned.deilysetnosy: + ronaldoussoren
2012-07-16 10:01:38sbtcreate