Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch for --symlink support in pyvenv with framework python #59512

Closed
ronaldoussoren opened this issue Jul 9, 2012 · 20 comments
Closed

Patch for --symlink support in pyvenv with framework python #59512

ronaldoussoren opened this issue Jul 9, 2012 · 20 comments
Assignees
Labels
OS-mac stdlib Python modules in the Lib dir

Comments

@ronaldoussoren
Copy link
Contributor

BPO 15307
Nosy @vsajip, @ronaldoussoren, @ned-deily
Files
  • venv-symlinks.txt
  • venv-symlinks-v2.txt
  • venv-symlinks-v3.txt
  • venv-symlinks-v4.txt
  • venv-symlinks-v5.txt
  • venv-symlinks-v6.txt
  • venv-symlinks-v7.txt
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ronaldoussoren'
    closed_at = <Date 2012-07-17.16:34:10.357>
    created_at = <Date 2012-07-09.15:13:40.909>
    labels = ['OS-mac', 'library']
    title = 'Patch for --symlink support in pyvenv with framework python'
    updated_at = <Date 2012-08-17.20:23:23.744>
    user = 'https://github.com/ronaldoussoren'

    bugs.python.org fields:

    activity = <Date 2012-08-17.20:23:23.744>
    actor = 'python-dev'
    assignee = 'ronaldoussoren'
    closed = True
    closed_date = <Date 2012-07-17.16:34:10.357>
    closer = 'python-dev'
    components = ['Library (Lib)', 'macOS']
    creation = <Date 2012-07-09.15:13:40.909>
    creator = 'ronaldoussoren'
    dependencies = []
    files = ['26332', '26364', '26366', '26373', '26375', '26396', '26398']
    hgrepos = []
    issue_num = 15307
    keywords = ['patch', 'needs review']
    message_count = 20.0
    messages = ['165087', '165289', '165294', '165301', '165304', '165368', '165370', '165382', '165398', '165399', '165400', '165416', '165432', '165531', '165594', '165615', '165648', '165725', '165764', '168474']
    nosy_count = 4.0
    nosy_names = ['vinay.sajip', 'ronaldoussoren', 'ned.deily', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue15307'
    versions = ['Python 3.3']

    @ronaldoussoren
    Copy link
    Contributor Author

    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?

    @ronaldoussoren ronaldoussoren self-assigned this Jul 9, 2012
    @ronaldoussoren ronaldoussoren added stdlib Python modules in the Lib dir OS-mac labels Jul 9, 2012
    @ned-deily
    Copy link
    Member

    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?

    @ronaldoussoren
    Copy link
    Contributor Author

    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.

    @ronaldoussoren
    Copy link
    Contributor Author

    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.

    @ronaldoussoren
    Copy link
    Contributor Author

    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 Python.app 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.

    @ned-deily
    Copy link
    Member

    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
    '/py/dev/default/b10.7_t10.7_x4.3_cclang_d/fw/./root/bin/python3.3'
    which caused a test_venv failure.  I probably won't have time to look at this again for a few days.

    @ronaldoussoren
    Copy link
    Contributor Author

    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.

    @ronaldoussoren
    Copy link
    Contributor Author

    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 <http://docs.python.org/dev/library/venv.html\> 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.

    @vsajip
    Copy link
    Member

    vsajip commented Jul 13, 2012

    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.

    @ronaldoussoren
    Copy link
    Contributor Author

    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.

    @ned-deily
    Copy link
    Member

    That's correct, the failing test was being run from a venv. I see now that what had changed is that the fixes for bpo-15241 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.

    @vsajip
    Copy link
    Member

    vsajip commented Jul 13, 2012

    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.

    @ned-deily
    Copy link
    Member

    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!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 15, 2012

    New changeset f954ee489896 by Vinay Sajip in branch 'default':
    Issue bpo-15307: Skipped test_venv:test_prefixes when run from a venv.
    http://hg.python.org/cpython/rev/f954ee489896

    @ronaldoussoren
    Copy link
    Contributor Author

    The test failure in test_osx_env is in itself fairly harmless because PYTHONEXECUTABLE is only used to ensure IDLE.app works correct, and IDLE.app 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)

    @ronaldoussoren
    Copy link
    Contributor Author

    And a final update: don't use TAB characters

    @ned-deily
    Copy link
    Member

    v7 looks good to me

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 17, 2012

    New changeset b79d276041a8 by Vinay Sajip in branch 'default':
    Closes bpo-15307: symlinks now work on OS X with framework Python builds. Patch by Ronald Oussoren.
    http://hg.python.org/cpython/rev/b79d276041a8

    @python-dev python-dev mannequin closed this as completed Jul 17, 2012
    @ronaldoussoren
    Copy link
    Contributor Author

    There is no NEWS entry, I'll commit one when I get home.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 17, 2012

    New changeset 4610ac42130e by Ned Deily in branch 'default':
    Issue bpo-15678: Fix menu customization for IDLE started from OS X
    http://hg.python.org/cpython/rev/4610ac42130e

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    OS-mac stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants