fix: only delete first sys.path entry in the stage-2 bootstrap if PYTHONSAFEPATH is unset or unsupported

[chowder]

Unnconditionally deleting the first sys.path entry on the stage-2 bootstrap incorrecly removes a valid search path on Python 3.11 and above, since PYTHONSAFEPATH is already unconditionally set in stage-1.

It should be deleted only if it is unset or unsupported.

Fixes #2318

[chowder]

Whilst adding tests for this change, I discovered what might be another bug?

https://github.com/bazelbuild/rules_python/blob/0.39.0/python/private/stage2_bootstrap_template.py#L415-L436

This snippet always executes on Python 3.10 and below, because safe_path hasn't been implemented yet.

But it will never execute on Python 3.11, because PYTHONSAFEPATH is unconditionally set in stage 1.

Not sure if this is expected behaviour.

[rickeylev]

Hmm, I wish I could remember what this was about, exactly. Maybe if I went and dug through the change history...

This might be about how PYTHONSAFEPATH being set by the outer environment is respected (by the bootstrap impl). While stage1 bootstrap will set it by default, it first checks to see if the outer environment has set it. If so, it'll respect that.

[rickeylev]

Thanks for the PR. This mostly LGTM. Just need to use a non-integration test for this instead. See comment.

[rickeylev]

Hrm. I think "intended behavior" here is all murky.

I'm pretty sure the intent of del sys.path[0] was to have safe path like behavior before python 3.11. However, that extra logic you found in stage2_bootstrap negates that? And the comment for that code points back to it being there for some sort of compatibility reason with how the system-python bootstrap behaved (but I'm not sure how correct that comment is, in retrospect).

At the least, we can confidently say: for 3.11 and later, there shouldn't be any del sys.path[0] nor any re-adding it back. Instead, we rely on stage1 to set PYTHONSAFEPATH instead.

[aignas]

LGTM, I agree with the reasoning in @rickeylev's message above.

[rickeylev]

Ok, I think I know what this logic is about. It's fixing up the path0 entry to point to the proper directory (runfiles instead of the filepath on the command line).

At startup, path0 looks like e.g. <exec root>/tests/bootstrap_impls. That directory comes from the invocation python $runfiles_root/tests/bootstrap_impls/foo_stage2.py, which is symlink back to <exec root>/tests/bootstrap_impls.

But, that directory doesn't have much in it, plus it means things outside the runfiles become importable. So the later prepend_path_entries = ... logic fixes that: it addes back the correct path, <execroot>/tests/bootstrap_impls/foo.runfiles/_main/tests_bootstrap_impls.

Hence, if safe path is disabled, we have to go manipulating sys.path. If safe path is enabled, then there is nothing to manipulate.

I did come across an edge case while looking into this: I happen to have PYTHONPATH set in my env. on python 3.11, because of that del sys.path[0] line, it ends up popping that off when it shouldn't.

[chowder]

I did come across an edge case while looking into this: I happen to have PYTHONPATH set in my env. on python 3.11, because of that del sys.path[0] line, it ends up popping that off when it shouldn't.

This PR should address that problem, shouldn't it?

The burning question I have is - should PYTHONSAFEPATH be set in stage-1?

Removing it will have all Python versions behave identically, and in line with historical behaviour.

Is there something undesirable about having the script's directory (where it doesn't follow symlinks) in sys.path[0]?

[chowder]

Pretty sure the build failures with last_rc Bazel (8.0.0rc3) are unrelated to these changes.

It's because @bazel_features evaluates 8.0.0rc3 to be "less than" 8.0.0, so it still thinks these are still valid globals.

[aignas]

The burning question I have is - should PYTHONSAFEPATH be set in stage-1?

Removing it will have all Python versions behave identically, and in line with historical behaviour.

I think that PYTHONSAFEPATH was being set in the python template in the bazel repo since quite a while ago, so the historic behaviour might be to have PYTHONSPAFEPATH to be used if it is possible.

However I can't find a link to the old source quickly, so my memory may be failing me.