fix: Use -P to enable safe path semantics instead of PYTHONSAFEPATH

[allsey87]

Setting PYTHONSAFEPATH has an unintentional side effect of propagating to child processes/non-py_binary instances of the Python interpreter that rely on the non-safepath behavior. The best example of this is the Meson build system whose support is included in rules_foreign_cc which will start subprocesses and sub-interpreters for tools such as Cython and the Emscripten compiler.

This PR changes the bootstrap script to use -P instead which will not have the side effect of propagating to child processes. Since versions of Python lower than 3.11.x do not support this argument, a check is necessary to prevent older interpreters from choking on this argument.

This solves the XY problem raised in issue https://github.com/bazelbuild/rules_python/issues/2060.

[aignas]

How does your bazelrc look like? Check out our docs on the flags we support to enable the two stage bootstrap which is not yet the default.

On 16 August 2024 12:52:05 EEST, Michael Allwright @.***> wrote:

Sorry for what is perhaps a stupid question, but when using rules_python to set up a py_binary it seems that the file python/private/stage1_bootstrap_template.sh is never executed. I can literally delete its contents (as well as running bazel clean --expunge and nuking .cache/bazel) and rebuild and get the same result...

Is this file perhaps not used on Linux? My modules.bazel file looks like this:

bazel_dep(name = "rules_python", version = "0.33.1")
# use my locally checked out version of rules_python
local_path_override(
   module_name = "rules_python",
   path = "rules_python"
)
python = ***@***.***_python//python/extensions:python.bzl", "python")
python.toolchain(python_version = "3.12.1", is_default = True)
use_repo(python, python_3_12_1 = "python_3_12_1")

-- Reply to this email directly or view it on GitHub: https://github.com/bazelbuild/rules_python/pull/2122#issuecomment-2293198563 You are receiving this because your review was requested.

Message ID: @.***>

[allsey87]

How does your bazelrc look like? Check out our docs on the flags we support to enable the two stage bootstrap which is not yet the default. … On 16 August 2024 12:52:05 EEST, Michael Allwright @.> wrote: Sorry for what is perhaps a stupid question, but when using rules_python to set up a py_binary it seems that the file python/private/stage1_bootstrap_template.sh is never executed. I can literally delete its contents (as well as running bazel clean --expunge and nuking .cache/bazel) and rebuild and get the same result... Is this file perhaps not used on Linux? My modules.bazel file looks like this: starlark bazel_dep(name = "rules_python", version = "0.33.1") # use my locally checked out version of rules_python local_path_override( module_name = "rules_python", path = "rules_python" ) python = ***@***.***_python//python/extensions:python.bzl", "python") python.toolchain(python_version = "3.12.1", is_default = True) use_repo(python, python_3_12_1 = "python_3_12_1") -- Reply to this email directly or view it on GitHub: #2122 (comment) You are receiving this because your review was requested. Message ID: @.>

Yep I figured this out. I originally missed the is_script_bootstrap_enabled config setting. I have run out of time to work on this today and unfortunately will be abroad for a couple weeks. After which I will pick this up again and try to finish it off.

[allsey87]

@rickeylev this PR is ready for review again. I mostly followed your instructions with two exceptions:

1. I used a space-separated string for interpreter_opts since a comma-separated string would break things like --option=value1,value2
2. I do not set PYTHONSAFEPATH if the interpreter version cannot be detected.

The reasoning for (2) is I believe it leads to less unexpected behavior. For example, it would be strange and hard to debug if changing the configuration of rules_python broke someone's build system due to PYTHONSAFEPATH being silently switched on. The counter argument is that not setting this environment variable may hide bugs in py_binary that try to access code from unsafe paths. However, the latter was already the case when PYTHONSAFEPATH was set and the Python version was less than 3.11, hence I feel that overall this solution has the least amount of surprising behavior.

[aignas]

Thanks for the update. Could you add a test to check that a python interpreter launched from within a python interpreter can still access to the same sitepackages. Right nowwe are only checking that we are not regressing, but this PR technically adds a new feature that allows starting new python interpreters from within a py_binary targets and those python processes will share the pythonpath with the parent.

#2076 did not allow this behaviour, if I understand it correctly.

As for reverting #2076, I would love to just have the same behaviour as regular python interpreter. So if reverting brings us closer, I'd be +1 to that, but right now as I understand, we don't need to do that.

[allsey87]

@aignas it's a little bit unclear to me what this new test would actually be testing. Would you mind elaborating a bit on what you had in mind. For example, what would count as passing the test vs. failing the test?

To the best of my understanding, the site packages will be the same since (1) rules_python uses PYTHONPATH to set the site packages and (2) the child process/python interpreter will inherit the PYTHONPATH environment variable. I believe that safe-path mode should not impact this behavior.

I just checked the changes from #2076 and can confirm that they are already effectively reverted by this PR and that the test from #2076 is updated and used in this PR.

[aignas]

Sorry for the delay in response. I thought that this PR would help with #2169, but thinking about it one more time, I am not sure. If you have time to experiment, feel free to do so in this PR, but we can look into this later.

[allsey87]

@aignas @rickeylev is it possible to get this reviewed and merged? As I understand, issue https://github.com/bazelbuild/rules_python/issues/2169 only applies to bootstrap_impl=script which is a new approach and which is not enabled by default (for now). Moreover, the propagation of PYTHONPATH to subprocesses is only tangentially related to this PR since using PYTHONSAFEPATH or -P has no impact on PYTHONPATH.

[allsey87]

@rickeylev is there anything blocking this that I can work on?

[aignas]

I think this got lost in the stream of emails/notifications, a ping to come back to this. :)

[rickeylev]

I'm really torn about this. I do think using -P is the right thing, but switching to it is almost certainly going to break something. I think we'll just have to accept that, because I don't see a way to work around it. If we have to, we can add a global flag to also set the environ variable.

Ignoring that, two main issues I see with the PR:

(1) Bazel 6 support and python_bootstrap_template.txt -- I don't see how this could be working? I would expect CI to show a failure. In Bazel 6, it's going to use the builtin py_runtime object, but point to rules_python's python_bootstrap_template.txt. Since the builtin py_runtime isn't expanding %interpreter_opts%, that should result in e.g. python.exe %interpreter_opts% blabla, and thus fail.

(2) This PR changes behavior for the "we don't know the version" case, which is, unfortunately, the default situation for workspace builds. This is because workspace builds default to using @bazel_tools//tools/python:autodetecting_toolchain, which define a builtin py_runtime object that won't have any of the version information. I think what we should do is, if the runtime doesn't have the version, check the @rules_python//python/config_settings:python_version flag (ctx.attr._python_version_flag). I think it's reasonable to assume such a situation is the builtin py_runtime case (rules_python py_runtime will use that flag value). This will give users a way to cause the right logic to happen, if necessary.

[aignas]

Is this still needed given the latest devolpments and fixing of #2409?

[allsey87]

@aignas I don't think #2409 solves this issue since PYTHONPATH and PYTHONSAFEPATH are not as related as they might seem. The latter basically prevents files in the same directory from being imported (which a lot of scripts, wrappers, and tools rely on).

Is there still interest in getting this PR merged or should I just patch rules_python locally and move on?

[aignas]

I'd be still +1 to set it. We had changes under the hood that may make it easier to ensure that we don't break. I would be especially +1 to set it in the script bootstrap case.

(2) This PR changes behavior for the "we don't know the version" case

In order to make the script bootstrap the default for linux OSes @rickeylev did some great work to ensure that we can at least know if we are on 3.11 or later, if I remember correctly.

[allsey87]

In order to make the script bootstrap the default for linux OSes @rickeylev did some great work to ensure that we can at least know if we are on 3.11 or later, if I remember correctly.

That sounds promising! Could you confirm @rickeylev? If so, I think that would unblock this PR after a rebase, right?

[aignas]

Just to clarify what I was thinking about: https://github.com/bazel-contrib/rules_python/blob/c678623fce4b5213b3c7661c166c0dac1ee22661/python/private/runtime_env_toolchain.bzl#L121

This allows the non-hermetic toolchains to work better. One could probable use something similar to enable the -P conditionally for only particular python versions or only for script=bootstrap.