msg165581 - (view) |
Author: Richard Oudkerk (sbt) * ![Python committer (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
Date: 2012-07-18 12:28 |
Forgot to remove some unnecessary code in patch.
|
msg165763 - (view) |
Author: Ronald Oussoren (ronaldoussoren) * ![Python committer (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
Date: 2012-07-26 19:43 |
LGTM
|
msg166522 - (view) |
Author: Éric Araujo (eric.araujo) * ![Python committer (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python triager)](@@file/triager.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python triager)](@@file/triager.png) |
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 (Python committer)](@@file/committer.png) |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:32 | admin | set | github: 59569 |
2012-08-14 00:01:57 | sbt | link | issue15322 superseder |
2012-08-14 00:01:25 | sbt | set | status: open -> closed |
2012-07-29 12:37:53 | sbt | set | resolution: fixed messages:
+ msg166753 stage: commit review -> resolved |
2012-07-28 06:38:19 | python-dev | set | messages:
+ msg166628 |
2012-07-27 14:10:50 | ronaldoussoren | set | messages:
+ msg166569 |
2012-07-27 13:54:41 | eric.araujo | set | nosy:
+ georg.brandl, benjamin.peterson
messages:
+ msg166566 versions:
+ Python 3.4, - Python 3.3 |
2012-07-27 11:08:44 | python-dev | set | nosy:
+ python-dev messages:
+ msg166553
|
2012-07-26 22:31:50 | sbt | set | messages:
+ msg166525 |
2012-07-26 21:29:12 | eric.araujo | set | messages:
+ msg166522 |
2012-07-26 19:43:15 | ned.deily | set | messages:
+ msg166506 stage: patch review -> commit review |
2012-07-26 13:17:22 | sbt | set | messages:
+ msg166481 |
2012-07-19 15:16:27 | sbt | set | messages:
+ msg165844 |
2012-07-18 12:30:58 | ronaldoussoren | set | messages:
+ msg165763 |
2012-07-18 12:28:38 | sbt | set | files:
+ sysconf.patch
messages:
+ msg165762 |
2012-07-18 12:21:54 | sbt | set | files:
+ sysconf.patch
messages:
+ msg165761 |
2012-07-18 00:30:29 | eric.araujo | set | messages:
+ msg165739 versions:
+ Python 3.3 |
2012-07-18 00:15:08 | ned.deily | set | messages:
+ msg165738 |
2012-07-18 00:03:45 | ned.deily | set | messages:
+ msg165737 |
2012-07-17 23:13:43 | sbt | set | files:
+ sysconf.patch
messages:
+ msg165735 |
2012-07-17 20:36:41 | ned.deily | set | nosy:
+ ned.deily messages:
+ msg165729
|
2012-07-17 13:37:19 | sbt | set | files:
+ sysconf.patch
messages:
+ msg165720 stage: needs patch -> patch review |
2012-07-16 23:06:11 | sbt | set | files:
+ sysconf.patch keywords:
+ patch messages:
+ msg165664
|
2012-07-16 20:36:48 | chris.jerdonek | set | nosy:
+ chris.jerdonek messages:
+ msg165654
|
2012-07-16 16:41:58 | ronaldoussoren | set | messages:
+ msg165634 |
2012-07-16 16:25:23 | eric.araujo | set | nosy:
+ fdrake, tarek messages:
+ msg165633
|
2012-07-16 16:23:05 | eric.araujo | set | messages:
+ msg165632 |
2012-07-16 15:59:05 | pitrou | set | nosy:
+ eric.araujo
|
2012-07-16 15:47:36 | sbt | set | messages:
+ msg165628 |
2012-07-16 14:43:28 | ronaldoussoren | set | messages:
+ msg165620 |
2012-07-16 10:25:08 | ned.deily | set | nosy:
+ ronaldoussoren
|
2012-07-16 10:01:38 | sbt | create | |