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

Support PYTHONSAFEPATH #1700

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

flying-sheep
Copy link

@flying-sheep flying-sheep commented Nov 13, 2023

Fixes #1696

Unfortunately, the test I added doesn’t really do much, since the code always hits this branch while testing.

# sys.path fakery. If we are being run as a command, then sys.path[0]
# is the directory of the "coverage" script. If this is so, replace
# sys.path[0] with the directory of the file we're running, or the
# current directory when running modules. If it isn't so, then we
# don't know what's going on, and just leave it alone.
top_file = inspect.stack()[-1][0].f_code.co_filename
sys_path_0_abs = os.path.abspath(sys.path[0])
top_file_dir_abs = os.path.abspath(os.path.dirname(top_file))
sys_path_0_abs = canonical_filename(sys_path_0_abs)
top_file_dir_abs = canonical_filename(top_file_dir_abs)
if sys_path_0_abs != top_file_dir_abs:
path0 = None

I manually confirmed that my change works, but allowing to actually test PyRunner.prepare, more extensive changes to the test setup are necessary that I don’t feel comfortable making.

Once a change is made that skips that branch during testing, my test will start working and catch breakage to the PYTHONSAFEPATH handling.

coverage/execfile.py Outdated Show resolved Hide resolved
tests/test_execfile.py Outdated Show resolved Hide resolved
tests/test_execfile.py Outdated Show resolved Hide resolved
tests/test_execfile.py Outdated Show resolved Hide resolved
@nedbat
Copy link
Owner

nedbat commented Nov 14, 2023

Thanks! I'll have to look into what you are saying about the check in execfile. That code gets very fiddly.

@flying-sheep
Copy link
Author

All done!

@nedbat
Copy link
Owner

nedbat commented Nov 18, 2023

Hmm, this is hard to test. The test might work better in tests/test_process.py, which launches new subprocesses so that environment variables can have better effect. Though: I tried making one, and your code change still didn't affect the outcome...

nedbat added a commit that referenced this pull request Nov 18, 2023
@nedbat
Copy link
Owner

nedbat commented Nov 18, 2023

I've proposed some test fixes here: flying-sheep#1

@flying-sheep
Copy link
Author

OK, I’ll take a look tomorrow!

@flying-sheep
Copy link
Author

OK, looks good. Seems like on windows, there‘s an additional line

           "d:\\a\\coveragepy\\coveragepy\\.tox\\py311\\scripts\\coverage.exe",

in the PYTHONPATH.

I don’t know why, and I don’t have access to windows to figure it out.

@doctaphred
Copy link

I've started looking into the failing Windows builds in hopes of getting this PR unblocked. I'm currently suspicious of the PYTHONSAFEPATH behavior of CPython itself on Windows.

This diff in particular seems sus. At first glance it looks like it should be a no-op, since the function referenced in the comment was already not used within that file. It's odd to see what appears to be a cleanup diff included in an otherwise very focused commit, though, and I wonder whether it was unintentional and/or had unintended side effects. It may have also been a follow-up to changes from a previous commit, but I haven't found any candidates yet.

The referenced _PyPathConfig_ComputeSysPath0() function certainly seems relevant, anyway. I'm planning to continue looking through its usage history to try to get a handle on what's actually supposed to be happening to sys.path on Windows, and how that changed with PYTHONSAFEPATH.

This might be a red herring, but I have to go now, so figured I'd go ahead and share it in case I'm not able to return to the case for a while.

@doctaphred
Copy link

Additional data points, which seem more relevant:

  • In Python 3.11, the default value of sys.platlibdir changed from 'lib' to 'DLLs' on Windows.
  • Possibly due to this change (or possibly not!), when running a script via coverage run run_me.py on Windows, the value of inspect.stack()[-1][0].f_code.co_filename (e.g., top_file in PyRunner.prepare()) changed from C:\hostedtoolcache\windows\Python\3.X.X\x64\lib\runpy.py (Python 3.10) to <frozen runpy> (Python 3.11+).
  • I am still not quite able to follow the logic of PyRunner.prepare() (though I think I've determined that the "sys.path fakery" comment is no longer accurate), but the subsequent dirname and abspath calls probably won't behave correctly with this value.
  • In comparison, on Ubuntu, the value is /home/runner/work/coveragepy/coveragepy/.tox/py3X/bin/coverage. (macOS is similar.) I am not sure why this is different: is runpy only used on Windows? Does one platform or the other have an extra stack frame?
  • The path D:\a\coveragepy\coveragepy\.tox\py311\Scripts\coverage.exe is found atsys.orig_argv[1] on Windows. (Full value is ["C:\\hostedtoolcache\\windows\\Python\\3.X.X\\x64\\python.exe", "D:\\a\\coveragepy\\coveragepy\\.tox\\py3X\\Scripts\\coverage.exe", "run", "run_me.py"].)
  • On Windows, a few sys.path entries with a lib component in Python 3.10 have that component capitalized (Lib) in Python 3.11+. This matches the capitalization of the same paths when they are returned by sysconfig.getpaths() in both versions. I am not sure what effect this might have (if any), especially with a case-insensitive filesystem, but it might interfere with path equality comparisons.
  • Setting PYTHONPLATLIBDIR=lib on Windows / Python 3.11 just breaks everything: ModuleNotFoundError: No module named '_socket' ¯\_(ツ)_/¯

I feel like there's probably enough info there to figure out what's happening, but I'm having trouble putting the pieces together and have to go again. Let me know if anyone else has a lightbulb moment, and I'll follow up with some more detailed debug dumps later.

@flying-sheep
Copy link
Author

Maybe we can ask a core dev about this?

@flying-sheep
Copy link
Author

Hi, as said, I don’t have access to windows. Can you suggest a course of action that gets this merged?

Like “just remove that entry during test execution and add a TODO with a link to an issue we open for it”

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

coverage run breaks PYTHONSAFEPATH
3 participants