From 5a5560264d26d1c4bf35428da71b82f7aa502f1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Thu, 27 Feb 2025 22:45:06 -0500 Subject: [PATCH 1/3] fix: Downgrade "running as root" error to a warning by default 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 https://github.com/bazelbuild/rules_python/issues/1169. --- CHANGELOG.md | 1 + python/private/python.bzl | 46 ++++++++++++----------- python/private/python_repository.bzl | 55 ++++++++++++++-------------- tests/python/python_tests.bzl | 22 +++++------ 4 files changed, 64 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c075af80b..c66dd95344 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ Unreleased changes template. * (pypi) The `ppc64le` is now pointing to the right target in the `platforms` package. * (gazelle) No longer incorrectly merge `py_binary` targets during partial updates in `file` generation mode. Fixed in [#2619](https://github.com/bazelbuild/rules_python/pull/2619). +* The hard error thrown when Bazel is run as root is now downgraded to a warning by default. {#v0-0-0-added} ### Added diff --git a/python/private/python.bzl b/python/private/python.bzl index ec6f73e41f..8c0e2051b3 100644 --- a/python/private/python.bzl +++ b/python/private/python.bzl @@ -72,9 +72,9 @@ def parse_modules(*, module_ctx, _fail = fail): logger = repo_utils.logger(module_ctx, "python") # if the root module does not register any toolchain then the - # ignore_root_user_error takes its default value: False + # ignore_root_user_error takes its default value: True if not module_ctx.modules[0].tags.toolchain: - ignore_root_user_error = False + ignore_root_user_error = True config = _get_toolchain_config(modules = module_ctx.modules, _fail = _fail) @@ -559,7 +559,7 @@ def _create_toolchain_attrs_struct(*, tag = None, python_version = None, toolcha is_default = is_default, python_version = python_version if python_version else tag.python_version, configure_coverage_tool = getattr(tag, "configure_coverage_tool", False), - ignore_root_user_error = getattr(tag, "ignore_root_user_error", False), + ignore_root_user_error = getattr(tag, "ignore_root_user_error", True), ) def _get_bazel_version_specific_kwargs(): @@ -636,16 +636,18 @@ Then the python interpreter will be available as `my_python_name`. doc = "Whether or not to configure the default coverage tool provided by `rules_python` for the compatible toolchains.", ), "ignore_root_user_error": attr.bool( - default = False, + default = True, doc = """\ -If `False`, the Python runtime installation will be made read only. This improves -the ability for Bazel to cache it, but prevents the interpreter from creating -`.pyc` files for the standard library dynamically at runtime as they are loaded. - -If `True`, the Python runtime installation is read-write. This allows the -interpreter to create `.pyc` files for the standard library, but, because they are -created as needed, it adversely affects Bazel's ability to cache the runtime and -can result in spurious build failures. +The Python runtime installation is made read only. This improves the ability for +Bazel to cache it by preventing the interpreter from creating `.pyc` files for +the standard library dynamically at runtime as they are loaded (this often leads +to spurious cache misses or build failures). + +However, if the user is running Bazel as root, this read-onlyness is not +respected. Bazel will print a warning message when it detects that the runtime +installation is writable despite being made read only (i.e. it's running with +root access). If this attribute is set to `False`, Bazel will make it a hard +error to run with root access instead. """, mandatory = False, ), @@ -690,16 +692,18 @@ dependencies are introduced. default = DEFAULT_RELEASE_BASE_URL, ), "ignore_root_user_error": attr.bool( - default = False, + default = True, doc = """\ -If `False`, the Python runtime installation will be made read only. This improves -the ability for Bazel to cache it, but prevents the interpreter from creating -`.pyc` files for the standard library dynamically at runtime as they are loaded. - -If `True`, the Python runtime installation is read-write. This allows the -interpreter to create `.pyc` files for the standard library, but, because they are -created as needed, it adversely affects Bazel's ability to cache the runtime and -can result in spurious build failures. +The Python runtime installation is made read only. This improves the ability for +Bazel to cache it by preventing the interpreter from creating `.pyc` files for +the standard library dynamically at runtime as they are loaded (this often leads +to spurious cache misses or build failures). + +However, if the user is running Bazel as root, this read-onlyness is not +respected. Bazel will print a warning message when it detects that the runtime +installation is writable despite being made read only (i.e. it's running with +root access). If this attribute is set to `False`, Bazel will make it a hard +error to run with root access instead. """, mandatory = False, ), diff --git a/python/private/python_repository.bzl b/python/private/python_repository.bzl index c7407c8f2c..2336f245e8 100644 --- a/python/private/python_repository.bzl +++ b/python/private/python_repository.bzl @@ -127,37 +127,36 @@ def _python_repository_impl(rctx): # pycs being generated at runtime: # * The pycs are not deterministic (they contain timestamps) # * Multiple processes trying to write the same pycs can result in errors. - if not rctx.attr.ignore_root_user_error: - if "windows" not in platform: - lib_dir = "lib" if "windows" not in platform else "Lib" + if "windows" not in platform: + repo_utils.execute_checked( + rctx, + op = "python_repository.MakeReadOnly", + arguments = [repo_utils.which_checked(rctx, "chmod"), "-R", "ugo-w", "lib"], + logger = logger, + ) - repo_utils.execute_checked( - rctx, - op = "python_repository.MakeReadOnly", - arguments = [repo_utils.which_checked(rctx, "chmod"), "-R", "ugo-w", lib_dir], - logger = logger, - ) - exec_result = repo_utils.execute_unchecked( + fail_or_warn = print if rctx.attr.ignore_root_user_error else fail + exec_result = repo_utils.execute_unchecked( + rctx, + op = "python_repository.TestReadOnly", + arguments = [repo_utils.which_checked(rctx, "touch"), "lib/.test"], + logger = logger, + ) + + # The issue with running as root is the installation is no longer + # read-only, so the problems due to pyc can resurface. + if exec_result.return_code == 0: + stdout = repo_utils.execute_checked_stdout( rctx, - op = "python_repository.TestReadOnly", - arguments = [repo_utils.which_checked(rctx, "touch"), "{}/.test".format(lib_dir)], + op = "python_repository.GetUserId", + arguments = [repo_utils.which_checked(rctx, "id"), "-u"], logger = logger, ) - - # The issue with running as root is the installation is no longer - # read-only, so the problems due to pyc can resurface. - if exec_result.return_code == 0: - stdout = repo_utils.execute_checked_stdout( - rctx, - op = "python_repository.GetUserId", - arguments = [repo_utils.which_checked(rctx, "id"), "-u"], - logger = logger, - ) - uid = int(stdout.strip()) - if uid == 0: - fail("The current user is root, please run as non-root when using the hermetic Python interpreter. See https://github.com/bazelbuild/rules_python/pull/713.") - else: - fail("The current user has CAP_DAC_OVERRIDE set, please drop this capability when using the hermetic Python interpreter. See https://github.com/bazelbuild/rules_python/pull/713.") + uid = int(stdout.strip()) + if uid == 0: + fail_or_warn("The current user is root, which can cause spurious cache misses or build failures with the hermetic Python interpreter. See https://github.com/bazelbuild/rules_python/pull/713.") + else: + fail_or_warn("The current user has CAP_DAC_OVERRIDE set, which can cause spurious cache misses or build failures with the hermetic Python interpreter. See https://github.com/bazelbuild/rules_python/pull/713.") python_bin = "python.exe" if ("windows" in platform) else "bin/python3" @@ -294,7 +293,7 @@ For more information see {attr}`py_runtime.coverage_tool`. mandatory = False, ), "ignore_root_user_error": attr.bool( - default = False, + default = True, doc = "Whether the check for root should be ignored or not. This causes cache misses with .pyc files.", mandatory = False, ), diff --git a/tests/python/python_tests.bzl b/tests/python/python_tests.bzl index e7828b92f5..ef8c7ea2bd 100644 --- a/tests/python/python_tests.bzl +++ b/tests/python/python_tests.bzl @@ -62,7 +62,7 @@ def _override( auth_patterns = {}, available_python_versions = [], base_url = "", - ignore_root_user_error = False, + ignore_root_user_error = True, minor_mapping = {}, netrc = "", register_all_versions = False): @@ -139,7 +139,7 @@ def _test_default(env): "ignore_root_user_error", "tool_versions", ]) - env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(False) + env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(True) env.expect.that_str(py.default_python_version).equals("3.11") want_toolchain = struct( @@ -212,13 +212,13 @@ def _test_default_non_rules_python_ignore_root_user_error(env): module_ctx = _mock_mctx( _mod( name = "my_module", - toolchain = [_toolchain("3.12", ignore_root_user_error = True)], + toolchain = [_toolchain("3.12", ignore_root_user_error = False)], ), _mod(name = "rules_python", toolchain = [_toolchain("3.11")]), ), ) - env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(True) + env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(False) env.expect.that_str(py.default_python_version).equals("3.12") my_module_toolchain = struct( @@ -244,13 +244,13 @@ def _test_default_non_rules_python_ignore_root_user_error_override(env): _mod( name = "my_module", toolchain = [_toolchain("3.12")], - override = [_override(ignore_root_user_error = True)], + override = [_override(ignore_root_user_error = False)], ), _mod(name = "rules_python", toolchain = [_toolchain("3.11")]), ), ) - env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(True) + env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(False) env.expect.that_str(py.default_python_version).equals("3.12") my_module_toolchain = struct( @@ -274,13 +274,13 @@ def _test_default_non_rules_python_ignore_root_user_error_non_root_module(env): py = parse_modules( module_ctx = _mock_mctx( _mod(name = "my_module", toolchain = [_toolchain("3.13")]), - _mod(name = "some_module", toolchain = [_toolchain("3.12", ignore_root_user_error = True)]), + _mod(name = "some_module", toolchain = [_toolchain("3.12", ignore_root_user_error = False)]), _mod(name = "rules_python", toolchain = [_toolchain("3.11")]), ), ) env.expect.that_str(py.default_python_version).equals("3.13") - env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(False) + env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(True) my_module_toolchain = struct( name = "python_3_13", @@ -338,8 +338,8 @@ def _test_first_occurance_of_the_toolchain_wins(env): env.expect.that_dict(py.debug_info).contains_exactly({ "toolchains_registered": [ - {"ignore_root_user_error": False, "module": {"is_root": True, "name": "my_module"}, "name": "python_3_12"}, - {"ignore_root_user_error": False, "module": {"is_root": False, "name": "rules_python"}, "name": "python_3_11"}, + {"ignore_root_user_error": True, "module": {"is_root": True, "name": "my_module"}, "name": "python_3_12"}, + {"ignore_root_user_error": True, "module": {"is_root": False, "name": "rules_python"}, "name": "python_3_11"}, ], }) @@ -364,7 +364,7 @@ def _test_auth_overrides(env): env.expect.that_dict(py.config.default).contains_at_least({ "auth_patterns": {"foo": "bar"}, - "ignore_root_user_error": False, + "ignore_root_user_error": True, "netrc": "/my/netrc", }) env.expect.that_str(py.default_python_version).equals("3.12") From 04513e544a9827a045739b884b50fe561f1a0293 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Fri, 28 Feb 2025 12:52:43 -0800 Subject: [PATCH 2/3] Apply suggestions from code review --- CHANGELOG.md | 5 ++++- python/private/python_repository.bzl | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c66dd95344..bed653fa11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,7 +59,10 @@ Unreleased changes template. * (pypi) The `ppc64le` is now pointing to the right target in the `platforms` package. * (gazelle) No longer incorrectly merge `py_binary` targets during partial updates in `file` generation mode. Fixed in [#2619](https://github.com/bazelbuild/rules_python/pull/2619). -* The hard error thrown when Bazel is run as root is now downgraded to a warning by default. +* (bzlmod) Running as root is no longer an error. `ignore_root_user_error=True` + is now the default. Note that running as root may still cause spurious + Bazel cache invalidation + ([#1169](https://github.com/bazelbuild/rules_python/issues/1169)). {#v0-0-0-added} ### Added diff --git a/python/private/python_repository.bzl b/python/private/python_repository.bzl index 2336f245e8..075d4b1195 100644 --- a/python/private/python_repository.bzl +++ b/python/private/python_repository.bzl @@ -135,7 +135,7 @@ def _python_repository_impl(rctx): logger = logger, ) - fail_or_warn = print if rctx.attr.ignore_root_user_error else fail + fail_or_warn = logger.warn if rctx.attr.ignore_root_user_error else logger.fail exec_result = repo_utils.execute_unchecked( rctx, op = "python_repository.TestReadOnly", From 9eec15e2c97908d14cf38f864fab1e4ab57a6d69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Fri, 28 Feb 2025 17:42:52 -0500 Subject: [PATCH 3/3] mark the ignore_root_user_error override deprecated --- python/private/python.bzl | 13 +------------ tests/python/python_tests.bzl | 32 -------------------------------- 2 files changed, 1 insertion(+), 44 deletions(-) diff --git a/python/private/python.bzl b/python/private/python.bzl index 8c0e2051b3..304a1d7745 100644 --- a/python/private/python.bzl +++ b/python/private/python.bzl @@ -693,18 +693,7 @@ dependencies are introduced. ), "ignore_root_user_error": attr.bool( default = True, - doc = """\ -The Python runtime installation is made read only. This improves the ability for -Bazel to cache it by preventing the interpreter from creating `.pyc` files for -the standard library dynamically at runtime as they are loaded (this often leads -to spurious cache misses or build failures). - -However, if the user is running Bazel as root, this read-onlyness is not -respected. Bazel will print a warning message when it detects that the runtime -installation is writable despite being made read only (i.e. it's running with -root access). If this attribute is set to `False`, Bazel will make it a hard -error to run with root access instead. -""", + doc = """Deprecated; do not use. This attribute has no effect.""", mandatory = False, ), "minor_mapping": attr.string_dict( diff --git a/tests/python/python_tests.bzl b/tests/python/python_tests.bzl index ef8c7ea2bd..6552251331 100644 --- a/tests/python/python_tests.bzl +++ b/tests/python/python_tests.bzl @@ -238,38 +238,6 @@ def _test_default_non_rules_python_ignore_root_user_error(env): _tests.append(_test_default_non_rules_python_ignore_root_user_error) -def _test_default_non_rules_python_ignore_root_user_error_override(env): - py = parse_modules( - module_ctx = _mock_mctx( - _mod( - name = "my_module", - toolchain = [_toolchain("3.12")], - override = [_override(ignore_root_user_error = False)], - ), - _mod(name = "rules_python", toolchain = [_toolchain("3.11")]), - ), - ) - - env.expect.that_bool(py.config.default["ignore_root_user_error"]).equals(False) - env.expect.that_str(py.default_python_version).equals("3.12") - - my_module_toolchain = struct( - name = "python_3_12", - python_version = "3.12", - register_coverage_tool = False, - ) - rules_python_toolchain = struct( - name = "python_3_11", - python_version = "3.11", - register_coverage_tool = False, - ) - env.expect.that_collection(py.toolchains).contains_exactly([ - rules_python_toolchain, - my_module_toolchain, - ]).in_order() - -_tests.append(_test_default_non_rules_python_ignore_root_user_error_override) - def _test_default_non_rules_python_ignore_root_user_error_non_root_module(env): py = parse_modules( module_ctx = _mock_mctx(