fix: Downgrade "running as root" error to a warning by default #2636
Conversation
164609b to
ae10fef
Compare
Currently, by default, rules_python immediately fails when Bazel is run as root. The reasoning behind this involves .pyc files being generated for hermetic toolchains when they're first used, causing cache misses; to work around this, rules_python opts to make the toolchain installation directory read-only, but running Bazel as root would circumvent this. So rules_python actively detects if the current user is root, and hard fails. This check can be disabled by the root module by setting `python.override(ignore_root_user_error=True)`. (See more context in the linked issues/PRs.) This causes a reverberating effect across the Bazel ecosystem, as rules_python is essentially a dependency of every single Bazel project through protobuf. Effectively, any Bazel project wishing to run as root need to add the override tag above, even if they don't have anything to do with Python at all. This PR changes the default value of the `ignore_root_user_error` to True instead. Besides, it now unconditionally tries to make the toolchain installation directory read-only, and only outputs a warning if it's detected that the current user is root. See previous discussions at #713, #749, #907, #1008, #1169, etc. Fixes #1169.
python/private/python_repository.bzl
Outdated
| logger = logger, | ||
| ) | ||
| exec_result = repo_utils.execute_unchecked( | ||
| fail_or_warn = print if rctx.attr.ignore_root_user_error else fail |
There was a problem hiding this comment.
Using logger.warn and logger.fail here would be slightly better.
Do you have a pointer where that should be done? I also feel like it's a bit outside the scope of this PR. In fact, wouldn't adding -B completely obviate this PR, since no .pyc files would ever be generated for the hermetic toolchains? I'm also having trouble fixing the tests. Specifically, this block of code dealing with overrides: https://github.com/bazelbuild/rules_python/blob/5a5560264d26d1c4bf35428da71b82f7aa502f1e/python/private/python.bzl#L430-L432 |
There was a problem hiding this comment.
re: setting -B: Yeah, I agree; outside the scope of the PR.
I think Ignas is referring to python/private/pypi/pypi_repo_utils.bzl, where there's a helper to run a command during repo phase.
-B would obviate this PR
It may or may not -- honestly we're not sure. Adding it could help the situation. We could never fully figure out what was going on and it was flaky behavior that was hard to replicate. At times, it almost seemed like repo-phase invocations, build actions, and runtime executions were all racing against each other trying to create pyc files in the directory the python runtime had been downloaded into.
I'm ok with that. i.e. I'm OK with ignore_root_user_error=True the only behavior; never fail, just warn. I just don't see why anyone would set ignore_root_user_error=False -- that means you're trying to guard against, what? Accidentally running bazel as root? |
Well, yeah... I would agree with you in that |
Yes. SGTM. |
…-contrib#2636) Currently, by default, rules_python immediately fails when Bazel is run as root. The reasoning behind this involves .pyc files being generated for hermetic toolchains when they're first used, causing cache misses; to work around this, rules_python opts to make the toolchain installation directory read-only, but running Bazel as root would circumvent this. So rules_python actively detects if the current user is root, and hard fails. This check can be disabled by the root module by setting `python.override(ignore_root_user_error=True)`. (See more context in the linked issues/PRs.) This causes a reverberating effect across the Bazel ecosystem, as rules_python is essentially a dependency of every single Bazel project through protobuf. Effectively, any Bazel project wishing to run as root need to add the override tag above, even if they don't have anything to do with Python at all. This PR changes the default value of the `ignore_root_user_error` to True instead. Besides, it now unconditionally tries to make the toolchain installation directory read-only, and only outputs a warning if it's detected that the current user is root. See previous discussions at bazel-contrib#713, bazel-contrib#749, bazel-contrib#907, bazel-contrib#1008, bazel-contrib#1169, etc. Fixes bazel-contrib#1169. --------- Co-authored-by: Richard Levasseur <rlevasseur@google.com>
Previously [#2636](#2636) changed the semantics of `ignore_root_user_error` from "ignore" to "warning". This is now flipped back to ignoring the issue, and will only emit a warning when the attribute is set `False`. This does also change the semantics of what #2636 did by flipping the attribute, as now there is no warning, and the user would have to explicitly set it to `False` (they don't want to ignore the error) to see the warning. Co-authored-by: Richard Levasseur <rlevasseur@google.com>
Currently, by default, rules_python immediately fails when Bazel is run as root. The reasoning behind this involves .pyc files being generated for hermetic toolchains when they're first used, causing cache misses; to work around this, rules_python opts to make the toolchain installation directory read-only, but running Bazel as root would circumvent this. So rules_python actively detects if the current user is root, and hard fails. This check can be disabled by the root module by setting
python.override(ignore_root_user_error=True). (See more context in the linked issues/PRs.)This causes a reverberating effect across the Bazel ecosystem, as rules_python is essentially a dependency of every single Bazel project through protobuf. Effectively, any Bazel project wishing to run as root need to add the override tag above, even if they don't have anything to do with Python at all.
This PR changes the default value of the
ignore_root_user_errorto True instead. Besides, it now unconditionally tries to make the toolchain installation directory read-only, and only outputs a warning if it's detected that the current user is root.See previous discussions at #713, #749, #907, #1008, #1169, etc.
Fixes #1169.