Title: Patch for --symlink support in pyvenv with framework python
Type: Stage: resolved
Components: Library (Lib), macOS Versions: Python 3.3
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ronaldoussoren Nosy List: ned.deily, python-dev, ronaldoussoren, vinay.sajip
Priority: normal Keywords: needs review, patch

Created on 2012-07-09 15:13 by ronaldoussoren, last changed 2012-08-17 20:23 by python-dev. This issue is now closed.

File name Uploaded Description Edit
venv-symlinks.txt ronaldoussoren, 2012-07-09 15:13 review
venv-symlinks-v2.txt ronaldoussoren, 2012-07-12 13:50 review
venv-symlinks-v3.txt ronaldoussoren, 2012-07-12 14:47 review
venv-symlinks-v4.txt ned.deily, 2012-07-13 07:32 review
venv-symlinks-v5.txt ronaldoussoren, 2012-07-13 13:57 review
venv-symlinks-v6.txt ronaldoussoren, 2012-07-16 12:31 review
venv-symlinks-v7.txt ronaldoussoren, 2012-07-16 14:28 review
Messages (20)
msg165087 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-09 15:13
The attached patch ensures that "python3.3 -mvenv --symlinks myenv" works with framework builds.

There are two functional changes:
1) don't call 'realpath' in pythonw.c because realpath resolves
   symlinks and that breaks the '--symlinks' options because the
   python command is a symlink with that option (and resolving that
   gives the path to the python executable in the framework)

2) Look for the __PYVENV_LAUNCHER__ environment variable in
   Modules/getpath.c and use that in preference on the already
   calculated value of argv0_path. That code is only active for
   framework builds

I'm not happy with the following line in this patch:

+            wcsncpy(argv0_path, (wchar_t*)pyvenv_launcher, MAXPATHLEN);

That mirrors a similar cast of the result of NSLibraryNameForModule earlier in Modules/getpath.c, but in both cases the input stream is a narrow character string ("char", not "wchar_t") and wcsncpy expects a "wchar_t". The the cast seems to paper over a real problem here.

Shouldn't both lines use "_Py_char2wchar" to convert the char* buffer to a wchar_t* one?
msg165289 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-12 08:57
I think you're right that the casts are incorrect.  I think the existing cast ia a day one bug in Python 3.  The question is why hasn't it been a problem?  That area needs fixing up since NSModuleForSymbol, NSLookupAndBindSymbol, and NSLibraryNameForModule are now deprecated interfaces.  Also, in the patch, shouldn't the wcsncpy be followed by a wcscpy(tmpbuffer, argv0_path) as well?
msg165294 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-12 10:22
The current code works, and I don't understand why it does.

I'd love to get rid of the long deprecated APIs like NSModuleForSymbol as well, but we'll have to ask the RM if that is an acceptable change at this point in the 3.3 release process.

BTW. Is adding symlink support on OSX a bug fix or a new feature? I'd prefer to treat this a bug fix, framework builds currently don't support the --symlinks option while they easily could do it.
msg165301 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-12 13:50
I've attached a new version of the patch:

* copy to tmp buffer instead of argv0_buffer (see comment by Ned)

* add include in pythonw.c to avoid compiler warning

* use _Py_char2wchar instead of blindly casting a char* to a wchar_t*

The behavior is not perfect yet, sys.executable is set to the wrong value (both with and without --symlinks).  I haven't checked yet if the value is correct without my patch.
msg165304 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-12 14:47
based on code inspection I'd say that sys.executable was broken without my patch as well. The code that sets that value is unchanged from Python 3.2, and that points to the executable inside the application bundle.

I've attached v3 of my patch, that ensures that sys.executable is the path used to start the executable. This gives points to the right executable when using venv and is a nicer (and arguably better) result outside of vent's as well.

There should probably be new tests as well that test that sys.executable has the right value in a venv.
msg165368 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-13 07:32
Fixing sys.executable to point to the stub launcher instead of the interpreter in the fw will also fix other unrelated issues, like making python3-32 work properly in 64-/32-bit builds for IDLE and for tests that spawn interpreters in subprocesses.  Today, while the main process is 32-bit,  the interpreters in subprocesses are 64-bit.

I did a little clean up and fixed up the documentation somewhat; see the revised patch.  But building a stock installer and running the tests showed two issues:
1) test_osx_env fails at line 28, checking PYTHONEXECUTABLE
2) when running the tests within a venv, test_venv now fails at line 95;
sys.baseprefix != sys.prefix.  This did not fail previously

Also, I saw another test failure when building and running with a test framework environment that included a relative link.  It appeared that _NSGetExecutablePath in pythonw.c returned an unnormalized path:
>>> sys.executable
which caused a test_venv failure.  I probably won't have time to look at this again for a few days.
msg165370 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-13 07:42
I removed the call to realpath from pythonw because resolving symlinks breaks the feature. Realpath also converts relative paths to absolute paths, and that probably explains the new failure you're having.

I'll try to find a solution for this, possibly by calling realpath on dirname(argv[0]) instead of the whole path.
msg165382 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-13 13:57
v5 adds test cases for sys.executable and calls realpath on dirname(argv[0]) in pythonw.c and also has the changes in Ned's v4.

The test failure for test_venv is expected, the note in <> explains that "sys.base_prefix and sys.base_exec_prefix point to the non-venv Python installation which was used to create the venv". The test fails because it assumes that sys.base_prefix of the created venv is same as sys.prefix of the running python, which is not true when you create a venv in a venv.

In particular line 95 of test_venv explicitly tests that sys.prefix == sys.base_prefix, which should fail when running the test within a virtual env. Tweaking the test suite to avoid that failure is beyond the scope of this issue.
msg165398 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-07-13 18:27
> In particular line 95 of test_venv explicitly tests that
> sys.prefix == sys.base_prefix, which should fail when running the
> test within a virtual env. Tweaking the test suite to avoid that
> failure is beyond the scope of this issue.

The first part of the test which includes line 95 (commented as "check our prefixes") checks the Python running the test, which is not running in a venv. The next part ("check a venv's prefixes") *does* run in a venv (using a subprocess to invoke the venv's Python), and the test compares the venv's prefixes to expected values. So, this test isn't wrong AFAICT, and has worked fine both on source builds and installed builds - so if it doesn't work after applying the patch, ISTM something else must be wrong.
msg165399 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-13 18:41
Ned mentioned that test_venv fails on line 95 when you run test_venv *in* a virtualenv, that is:

$ pyvenv testenv
$ testenv/bin/python3 -m test.test_venv

This fails because sys.prefix != sys.base_prefix for testenv/bin/python3, and that's expected. 

Unless I'm mistaken nothing is wrong here.
msg165400 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-13 18:50
That's correct, the failing test was being run from a venv.  I see now that what had changed is that the fixes for Issue15241 recently added the test_prefixes test case to test_venv and that fails when the test is run from within a venv.  Without that new test case, test_venv didn't fail at 3.3.0b1 when run from a venv.  Whether it should is another matter and is out of scope for this issue.
msg165416 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-07-13 20:29
As the test is constructed now, that test will fail if run from a venv. It will be a simple matter to skip it if test_venv being run from a venv - I can add that at some point, but I agree it's orthogonal to the crux of this issue.
msg165432 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-14 01:06
v5 fixes the non normalized path issue.  However, the PYTHONEXECUTABLE env var -> argv processing is still broken, as detected by the test_osx_env failure.  It's definitely caused by interaction between pythonw.c and the main interpreter; if the interpreter is launched directly (not via pythonw), PYTHONEXECUTABLE works correctly.

Also, the patch needs to be tab-free: no more tabs for C files!
msg165531 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-07-15 15:13
New changeset f954ee489896 by Vinay Sajip in branch 'default':
Issue #15307: Skipped test_venv:test_prefixes when run from a venv.
msg165594 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-16 12:31
The test failure in test_osx_env is in itself fairly harmless because PYTHONEXECUTABLE is only used to ensure works correct, and doesn't use the pythonw executable and hence won't hit the code that special-cases the venv location.

That said, the failure should be removed and points to a real issue in my patch: with my patch calculate_path replaces the value set by Py_SetProgramName and that is a real problem that could break 3th-party code without a good reason.

I've attached v5 of the patch, this fixes the test_osx_env failure by moving the the code that uses __PYVENV_LAUNCHER__ to main.c (where Py_SetProgramName is called)
msg165615 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-16 14:28
And a final update: don't use TAB characters
msg165648 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-16 19:18
v7 looks good to me
msg165725 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-07-17 16:34
New changeset b79d276041a8 by Vinay Sajip in branch 'default':
Closes #15307: symlinks now work on  OS X with framework Python builds. Patch by Ronald Oussoren.
msg165764 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2012-07-18 12:40
There is no NEWS entry, I'll commit one when I get home.
msg168474 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-08-17 20:23
New changeset 4610ac42130e by Ned Deily in branch 'default':
Issue #15678: Fix menu customization for IDLE started from OS X
Date User Action Args
2012-08-17 20:23:23python-devsetmessages: + msg168474
2012-07-18 12:40:51ronaldoussorensetmessages: + msg165764
2012-07-17 16:34:10python-devsetstatus: open -> closed
resolution: fixed
messages: + msg165725

stage: commit review -> resolved
2012-07-16 19:18:21ned.deilysetmessages: + msg165648
stage: commit review
2012-07-16 14:28:02ronaldoussorensetfiles: + venv-symlinks-v7.txt

messages: + msg165615
2012-07-16 12:31:48ronaldoussorensetfiles: + venv-symlinks-v6.txt

messages: + msg165594
2012-07-15 15:13:06python-devsetnosy: + python-dev
messages: + msg165531
2012-07-14 01:07:00ned.deilysetmessages: + msg165432
2012-07-13 20:29:58vinay.sajipsetmessages: + msg165416
2012-07-13 18:50:47ned.deilysetmessages: + msg165400
2012-07-13 18:41:01ronaldoussorensetmessages: + msg165399
2012-07-13 18:27:50vinay.sajipsetmessages: + msg165398
2012-07-13 13:57:26ronaldoussorensetfiles: + venv-symlinks-v5.txt

messages: + msg165382
2012-07-13 07:42:30ronaldoussorensetmessages: + msg165370
2012-07-13 07:32:18ned.deilysetfiles: + venv-symlinks-v4.txt

messages: + msg165368
2012-07-12 14:47:01ronaldoussorensetfiles: + venv-symlinks-v3.txt

messages: + msg165304
2012-07-12 13:50:10ronaldoussorensetfiles: + venv-symlinks-v2.txt

messages: + msg165301
2012-07-12 10:22:12ronaldoussorensetmessages: + msg165294
2012-07-12 08:57:18ned.deilysetmessages: + msg165289
2012-07-09 15:14:54ronaldoussorensetnosy: + vinay.sajip, ned.deily
2012-07-09 15:13:40ronaldoussorencreate