PYTHONSAFEPATH in tests?

[joerick]

@joerick would setting PYTHONSAFEPATH makes sense for tests? That would only help 3.11+, but SPEC 0 packages are 3.11+ already and they tend to be old packages that didn't use src folders and don't want to change. Maybe that would be too invasive, but it seems like a possible idea.

Edit: even though scikit-learn is listed on SPEC 0, they still support 3.10+. Though if they are actually still on the NEP 29 schedule instead, today is the drop day for 3.10.

Originally posted by @henryiii in #2348

[joerick]

Could you expand on what you mean here @henryiii ? I don't quite follow what you're proposing - what would you set it to, and how would that change behaviour?

[henryiii]

If you set PYTHONSAFEPATH (to any non-empty string), then Python (3.11+) will not add the current directory to sys.path. If someone doesn't use src structure, then the local directory will get imported instead of the installed package if someone uses a python -m command, such as python -m pytest. Scikit-learn, for example, does this (See scikit-learn/scikit-learn#31125). Setting PYTHONSAFEPATH will fix the python -m commands, at least if Python is new enough. (The pytest command already does this, but python -m pytest can't without the variable set.)

[joerick]

Ah, thanks for the info.

I see it's also available as a flag, python -P. We could document that, where we suggest python -Pm MODULE.

I'm a bit unsure about adding it across the board... it feels like it could break things like python myscript.py quite easily, and in quite confusing ways. If we were to add it, we should maybe also add a CIBW_TEST_ENVIRONMENT (that has been requested before) as a way to 'unset' that variable and undo the choice.

Is the cwd-in-pythonpath a change in cibuildwheel v3.0? I had understood that the change to running tests from inside the project dir didn't affect that, as pytest added the cwd to sys.path anyway.

[henryiii]

I see it's also available as a flag

That flag would require 3.11+, though. The environment variable would just be ignored on 3.10+. And the point was to try to reduce breakage for people upgrading 2->3; if people need to change things, they could just add test-sources, or change python -m pytest to pytest.

it feels like it could break things

You can't do python myscript.py in cibuildwheel 2 already, since we change the directory before running the test command, so it's not backward incompatible. python {project}/myscript.py doesn't load {project} into the path, it loads the empty directory you are in (that's why we did this in the first place). And you should be explicit if you are trying to load stuff locally anyway (like cibuildwheel 2 required).

Is the cwd-in-pythonpath a change in cibuildwheel v3.0

Yes, cibuildwheel 2 changed the directory to a new empty directory (plus a small test file designed to produce an error if you ran it by mistake), and forced everyone to use {project}. This was to work around this "feature" of Python adding the current working directory to the top of the path, which would could load the local files instead of the built wheel. This is exactly what PYTHONSAFEPATH is designed to fix. pytest doesn't add cwd to the sys.path as they also consider this to be bad, but Python does it for you if you do python -m <anything>, including python -m pytest (without the new variable or flag).

Since you can always do PYTHONPATH=., or add it to sys.path in your script (which is what most would do anyway), and you had to do some sort of workaround like that in cibuildwheel 2 anyway, I think it's safe to do before 3.0 is out.

PS: If PYTHONSAFEPATH was Python 3.8+, I think it would be absolutely clearly better to always set it. It does exactly what we tried to do before but with fewer side effects. The only minor issue is it's 3.11+, but I think long term we'll be 3.11+ only eventually, so it's better to set it now.

[henryiii]

So maybe we could add CIBW_TEST_ENVIRONMENT that defaults to having PYTHONSAFEPATH in it? How does this interact with CIBW_ENVIRONMENT?

[joerick]

Yes, we could add CIBW_TEST_ENVIRONMENT. I'd maybe phrase it a little different - cibuildwheel would set PYTHONSAFEPATH=1, but users can choose to unset it by doing CIBW_TEST_ENVIRONMENT="PYTHONSAFEPATH=".

That would prevent the accidental unsetting of PYTHONSAFEPATH when using CIBW_TEST_ENVIRONMENT for something else.

Given the cwd change that's already in v3.0, maybe it's worth trying get this change into v3.0 as well. I'll have a look at how much work it would be...