fix: ignore_root_user_error should be taken from root module only (#1662)

Only the root module should be able to decide `ignore_root_user_error`.
Modules being depended upon don't know the final environment, so they
aren't in the right position to know or decide what the correct setting
is. This PR implements a solution to take the `ignore_root_user_error`
value from the root module only.

Fixes #1658

---------

Co-authored-by: Richard Levasseur <[email protected]>

Author: shabanzd

.bazelrc

@@ -4,8 +4,8 @@
 # (Note, we cannot use `common --deleted_packages` because the bazel version command doesn't support it)
 # To update these lines, execute
 # `bazel run @rules_bazel_integration_test//tools:update_deleted_packages`
-build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered
-query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered
+build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered,tests/integration/ignore_root_user_error/submodule
+query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered,tests/integration/ignore_root_user_error/submodule
 
 test --test_output=errors
 

CHANGELOG.md

@@ -124,6 +124,9 @@ A brief description of the categories of changes:
 * (toolchains) Workspace builds register the py cc toolchain (bzlmod already
   was). This makes e.g. `//python/cc:current_py_cc_headers` Just Work.
   ([#1669](https://github.com/bazelbuild/rules_python/issues/1669))
+* (bzlmod python.toolchain) The value of `ignore_root_user_error` is now decided
+  by the root module only.
+  ([#1658](https://github.com/bazelbuild/rules_python/issues/1658))
 
 ### Added
 

python/extensions/BUILD.bazel

@@ -35,5 +35,8 @@ bzl_library(
     name = "python_bzl",
     srcs = ["python.bzl"],
     visibility = ["//:__subpackages__"],
-    deps = ["//python/private/bzlmod:python_bzl"],
+    deps = [
+        "//python/private:util_bzl",
+        "//python/private/bzlmod:python_bzl",
+    ],
 )

python/private/bzlmod/python.bzl

@@ -16,6 +16,7 @@
 
 load("//python:repositories.bzl", "python_register_toolchains")
 load("//python/private:toolchains_repo.bzl", "multi_toolchain_aliases")
+load("//python/private:util.bzl", "IS_BAZEL_6_4_OR_HIGHER")
 load(":pythons_hub.bzl", "hub_repo")
 
 # This limit can be increased essentially arbitrarily, but doing so will cause a rebuild of all
@@ -43,14 +44,14 @@ def _left_pad_zero(index, length):
 def _print_warn(msg):
     print("WARNING:", msg)
 
-def _python_register_toolchains(name, toolchain_attr, module):
+def _python_register_toolchains(name, toolchain_attr, module, ignore_root_user_error):
     """Calls python_register_toolchains and returns a struct used to collect the toolchains.
     """
     python_register_toolchains(
         name = name,
         python_version = toolchain_attr.python_version,
         register_coverage_tool = toolchain_attr.configure_coverage_tool,
-        ignore_root_user_error = toolchain_attr.ignore_root_user_error,
+        ignore_root_user_error = ignore_root_user_error,
     )
     return struct(
         python_version = toolchain_attr.python_version,
@@ -59,6 +60,13 @@ def _python_register_toolchains(name, toolchain_attr, module):
     )
 
 def _python_impl(module_ctx):
+    if module_ctx.os.environ.get("RULES_PYTHON_BZLMOD_DEBUG", "0") == "1":
+        debug_info = {
+            "toolchains_registered": [],
+        }
+    else:
+        debug_info = None
+
     # The toolchain_info structs to register, in the order to register them in.
     # NOTE: The last element is special: it is treated as the default toolchain,
     # so there is special handling to ensure the last entry is the correct one.
@@ -72,6 +80,13 @@ def _python_impl(module_ctx):
     # Map of string Major.Minor to the toolchain_info struct
     global_toolchain_versions = {}
 
+    ignore_root_user_error = None
+
+    # if the root module does not register any toolchain then the
+    # ignore_root_user_error takes its default value: False
+    if not module_ctx.modules[0].tags.toolchain:
+        ignore_root_user_error = False
+
     for mod in module_ctx.modules:
         module_toolchain_versions = []
 
@@ -84,16 +99,27 @@ def _python_impl(module_ctx):
                 _fail_duplicate_module_toolchain_version(toolchain_version, mod.name)
             module_toolchain_versions.append(toolchain_version)
 
-            # Only the root module and rules_python are allowed to specify the default
-            # toolchain for a couple reasons:
-            # * It prevents submodules from specifying different defaults and only
-            #   one of them winning.
-            # * rules_python needs to set a soft default in case the root module doesn't,
-            #   e.g. if the root module doesn't use Python itself.
-            # * The root module is allowed to override the rules_python default.
             if mod.is_root:
+                # Only the root module and rules_python are allowed to specify the default
+                # toolchain for a couple reasons:
+                # * It prevents submodules from specifying different defaults and only
+                #   one of them winning.
+                # * rules_python needs to set a soft default in case the root module doesn't,
+                #   e.g. if the root module doesn't use Python itself.
+                # * The root module is allowed to override the rules_python default.
+
                 # A single toolchain is treated as the default because it's unambiguous.
                 is_default = toolchain_attr.is_default or len(mod.tags.toolchain) == 1
+
+                # Also only the root module should be able to decide ignore_root_user_error.
+                # Modules being depended upon don't know the final environment, so they aren't
+                # in the right position to know or decide what the correct setting is.
+
+                # If an inconsistency in the ignore_root_user_error among multiple toolchains is detected, fail.
+                if ignore_root_user_error != None and toolchain_attr.ignore_root_user_error != ignore_root_user_error:
+                    fail("Toolchains in the root module must have consistent 'ignore_root_user_error' attributes")
+
+                ignore_root_user_error = toolchain_attr.ignore_root_user_error
             elif mod.name == "rules_python" and not default_toolchain:
                 # We don't do the len() check because we want the default that rules_python
                 # sets to be clearly visible.
@@ -128,8 +154,14 @@ def _python_impl(module_ctx):
                     toolchain_name,
                     toolchain_attr,
                     module = mod,
+                    ignore_root_user_error = ignore_root_user_error,
                 )
                 global_toolchain_versions[toolchain_version] = toolchain_info
+                if debug_info:
+                    debug_info["toolchains_registered"].append({
+                        "ignore_root_user_error": ignore_root_user_error,
+                        "name": toolchain_name,
+                    })
 
             if is_default:
                 # This toolchain is setting the default, but the actual
@@ -192,6 +224,12 @@ def _python_impl(module_ctx):
         },
     )
 
+    if debug_info != None:
+        _debug_repo(
+            name = "rules_python_bzlmod_debug",
+            debug_info = json.encode_indent(debug_info),
+        )
+
 def _fail_duplicate_module_toolchain_version(version, module):
     fail(("Duplicate module toolchain version: module '{module}' attempted " +
           "to use version '{version}' multiple times in itself").format(
@@ -220,6 +258,14 @@ def _fail_multiple_default_toolchains(first, second):
         second = second,
     ))
 
+def _get_bazel_version_specific_kwargs():
+    kwargs = {}
+
+    if IS_BAZEL_6_4_OR_HIGHER:
+        kwargs["environ"] = ["RULES_PYTHON_BZLMOD_DEBUG"]
+
+    return kwargs
+
 python = module_extension(
     doc = """Bzlmod extension that is used to register Python toolchains.
 """,
@@ -263,7 +309,16 @@ A toolchain's repository name uses the format `python_{major}_{minor}`, e.g.
                 ),
                 "ignore_root_user_error": attr.bool(
                     default = False,
-                    doc = "Whether the check for root should be ignored or not. This causes cache misses with .pyc files.",
+                    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.
+""",
                     mandatory = False,
                 ),
                 "is_default": attr.bool(
@@ -279,4 +334,23 @@ A toolchain's repository name uses the format `python_{major}_{minor}`, e.g.
             },
         ),
     },
+    **_get_bazel_version_specific_kwargs()
+)
+
+_DEBUG_BUILD_CONTENT = """
+package(
+    default_visibility = ["//visibility:public"],
+)
+exports_files(["debug_info.json"])
+"""
+
+def _debug_repo_impl(repo_ctx):
+    repo_ctx.file("BUILD.bazel", _DEBUG_BUILD_CONTENT)
+    repo_ctx.file("debug_info.json", repo_ctx.attr.debug_info)
+
+_debug_repo = repository_rule(
+    implementation = _debug_repo_impl,
+    attrs = {
+        "debug_info": attr.string(),
+    },
 )

python/private/util.bzl

@@ -89,3 +89,16 @@ IS_BAZEL_7_OR_HIGHER = hasattr(native, "starlark_doc_extract")
 # Bazel 5.4 has a bug where every access of testing.ExecutionInfo is a
 # different object that isn't equal to any other. This is fixed in bazel 6+.
 IS_BAZEL_6_OR_HIGHER = testing.ExecutionInfo == testing.ExecutionInfo
+
+_marker_rule_to_detect_bazel_6_4_or_higher = rule(implementation = lambda ctx: None)
+
+# Bazel 6.4 and higher have a bug fix where rule names show up in the str()
+# of a rule. See
+# https://github.com/bazelbuild/bazel/commit/002490b9a2376f0b2ea4a37102c5e94fc50a65ba
+# https://github.com/bazelbuild/bazel/commit/443cbcb641e17f7337ccfdecdfa5e69bc16cae55
+# This technique is done instead of using native.bazel_version because,
+# under stardoc, the native.bazel_version attribute is entirely missing, which
+# prevents doc generation from being able to correctly generate docs.
+IS_BAZEL_6_4_OR_HIGHER = "_marker_rule_to_detect_bazel_6_4_or_higher" in str(
+    _marker_rule_to_detect_bazel_6_4_or_higher,
+)

tests/integration/ignore_root_user_error/.bazelrc

@@ -1,3 +1,5 @@
+common --action_env=RULES_PYTHON_BZLMOD_DEBUG=1
+common --lockfile_mode=off
 test --test_output=errors
 
 # Windows requires these for multi-python support:

tests/integration/ignore_root_user_error/BUILD.bazel

@@ -1,7 +1,32 @@
-load("@rules_python//python:defs.bzl", "py_test")
+# Copyright 2024 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load("@rules_python//python:py_test.bzl", "py_test")
+load("@rules_python//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED")  # buildifier: disable=bzl-visibility
 
 py_test(
     name = "foo_test",
     srcs = ["foo_test.py"],
     visibility = ["//visibility:public"],
 )
+
+py_test(
+    name = "bzlmod_test",
+    srcs = ["bzlmod_test.py"],
+    data = [
+        "@rules_python//python/runfiles",
+        "@rules_python_bzlmod_debug//:debug_info.json",
+    ],
+    target_compatible_with = [] if BZLMOD_ENABLED else ["@platforms//:incompatible"],
+)

tests/integration/ignore_root_user_error/MODULE.bazel

@@ -0,0 +1,20 @@
+module(name = "ignore_root_user_error")
+
+bazel_dep(name = "rules_python", version = "0.0.0")
+local_path_override(
+    module_name = "rules_python",
+    path = "../../..",
+)
+
+bazel_dep(name = "submodule")
+local_path_override(
+    module_name = "submodule",
+    path = "submodule",
+)
+
+python = use_extension("@rules_python//python/extensions:python.bzl", "python")
+python.toolchain(
+    ignore_root_user_error = True,
+    python_version = "3.11",
+)
+use_repo(python, "rules_python_bzlmod_debug")

tests/integration/ignore_root_user_error/bzlmod_test.py

@@ -0,0 +1,37 @@
+# Copyright 2024 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import unittest
+import pathlib
+import json
+
+from python.runfiles import runfiles
+
+class BzlmodTest(unittest.TestCase):
+    def test_toolchains(self):
+        rf = runfiles.Create()
+        debug_path = pathlib.Path(rf.Rlocation(
+                "rules_python_bzlmod_debug/debug_info.json"
+        ))
+        debug_info = json.loads(debug_path.read_bytes())
+
+        expected = [
+            {'ignore_root_user_error': True, 'name': 'python_3_11', },
+            {'ignore_root_user_error': True, 'name': 'python_3_10'}
+        ]
+        self.assertCountEqual(debug_info["toolchains_registered"],
+                              expected)
+
+if __name__ == "__main__":
+    unittest.main()