wip: fix pyc_collection bug

Author: rickeylev

docs/api/rules_python/python/config_settings/index.md

@@ -38,9 +38,7 @@ Values:
 
 * `auto`: (default) Automatically decide the effective value based on environment,
   target platform, etc.
-* `enabled`: Compile Python source files at build time. Note that
-  {bzl:obj}`--precompile_add_to_runfiles` affects how the compiled files are included into
-  a downstream binary.
+* `enabled`: Compile Python source files at build time.
 * `disabled`: Don't compile Python source files at build time.
 * `if_generated_source`: Compile Python source files, but only if they're a
   generated file.
@@ -79,37 +77,6 @@ The `auto` value
 :::
 ::::
 
-::::{bzl:flag} precompile_add_to_runfiles
-Determines if a target adds its compiled files to its runfiles.
-
-When a target compiles its files, but doesn't add them to its own runfiles, it
-relies on a downstream target to retrieve them from
-{bzl:obj}`PyInfo.transitive_pyc_files`
-
-Values:
-* `always`: Always include the compiled files in the target's runfiles.
-* `decided_elsewhere`: Don't include the compiled files in the target's
-  runfiles; they are still added to {bzl:obj}`PyInfo.transitive_pyc_files`. See
-  also: {bzl:obj}`py_binary.pyc_collection` attribute. This is useful for allowing
-  incrementally enabling precompilation on a per-binary basis.
-:::{versionadded} 0.33.0
-:::
-::::
-
-::::{bzl:flag} pyc_collection
-Determine if `py_binary` collects transitive pyc files.
-
-:::{note}
-This flag is overridden by the target level `pyc_collection` attribute.
-:::
-
-Values:
-* `include_pyc`: Include `PyInfo.transitive_pyc_files` as part of the binary.
-* `disabled`: Don't include `PyInfo.transitive_pyc_files` as part of the binary.
-:::{versionadded} 0.33.0
-:::
-::::
-
 ::::{bzl:flag} py_linux_libc
 Set what libc is used for the target platform. This will affect which whl binaries will be pulled and what toolchain will be auto-detected. Currently `rules_python` only supplies toolchains compatible with `glibc`.
 

docs/precompiling.md

@@ -20,24 +20,24 @@ While precompiling helps runtime performance, it has two main costs:
 
 ## Binary-level opt-in
 
-Because of the costs of precompiling, it may not be feasible to globally enable it
-for your repo for everything. For example, some binaries may be
-particularly large, and doubling the number of runfiles isn't doable.
+Binary-level opt-in allows enabling precompiling on a per-target basic. This is
+useful for situations such as:
 
-If this is the case, there's an alternative way to more selectively and
-incrementally control precompiling on a per-binry basis.
+* Globally enabling precompiling in your `.bazelrc` isn't feasible. This may
+  be because some targets don't work with precompiling, e.g. because they're too
+  big.
+* Enabling precompiling for build tools (exec config targets) separately from
+  target-config programs.
 
-To use this approach, the two basic steps are:
-1. Disable pyc files from being automatically added to runfiles:
-   {bzl:obj}`--@rules_python//python/config_settings:precompile_add_to_runfiles=decided_elsewhere`,
-2. Set the `pyc_collection` attribute on the binaries/tests that should or should
-   not use precompiling.
+To use this approach, set the {bzl:attr}`pyc_collection` attribute on the
+binaries/tests that should or should not use precompiling. Then change the
+{bzl:flag}`--precompile` default.
 
-The default for the `pyc_collection` attribute is controlled by the flag
-{bzl:obj}`--@rules_python//python/config_settings:pyc_collection`, so you
+The default for the {bzl:attr}`pyc_collection` attribute is controlled by the flag
+{bzl:obj}`--@rules_python//python/config_settings:precompile`, so you
 can use an opt-in or opt-out approach by setting its value:
-* targets must opt-out: `--@rules_python//python/config_settings:pyc_collection=include_pyc`
-* targets must opt-in: `--@rules_python//python/config_settings:pyc_collection=disabled`
+* targets must opt-out: `--@rules_python//python/config_settings:precompile=enabled`
+* targets must opt-in: `--@rules_python//python/config_settings:precompile=disabled`
 
 ## Advanced precompiler customization
 
@@ -48,7 +48,7 @@ not work as well for remote execution builds. To customize the precompiler, two
 mechanisms are available:
 
 * The exec tools toolchain allows customizing the precompiler binary used with
-  the `precompiler` attribute. Arbitrary binaries are supported.
+  the {bzl:attr}`precompiler` attribute. Arbitrary binaries are supported.
 * The execution requirements can be customized using
   `--@rules_python//tools/precompiler:execution_requirements`. This is a list
   flag that can be repeated. Each entry is a key=value that is added to the

python/config_settings/BUILD.bazel

@@ -4,10 +4,8 @@ load(
     "//python/private:flags.bzl",
     "BootstrapImplFlag",
     "ExecToolsToolchainFlag",
-    "PrecompileAddToRunfilesFlag",
     "PrecompileFlag",
     "PrecompileSourceRetentionFlag",
-    "PycCollectionFlag",
 )
 load(
     "//python/private/pypi:flags.bzl",
@@ -67,22 +65,6 @@ string_flag(
     visibility = ["//visibility:public"],
 )
 
-string_flag(
-    name = "precompile_add_to_runfiles",
-    build_setting_default = PrecompileAddToRunfilesFlag.ALWAYS,
-    values = sorted(PrecompileAddToRunfilesFlag.__members__.values()),
-    # NOTE: Only public because it's an implicit dependency
-    visibility = ["//visibility:public"],
-)
-
-string_flag(
-    name = "pyc_collection",
-    build_setting_default = PycCollectionFlag.DISABLED,
-    values = sorted(PycCollectionFlag.__members__.values()),
-    # NOTE: Only public because it's an implicit dependency
-    visibility = ["//visibility:public"],
-)
-
 string_flag(
     name = "bootstrap_impl",
     build_setting_default = BootstrapImplFlag.SYSTEM_PYTHON,

python/private/common/attributes.bzl

@@ -29,6 +29,25 @@ load(
 
 _PackageSpecificationInfo = getattr(py_internal, "PackageSpecificationInfo", None)
 
+# Due to how the common exec_properties attribute works, rules must add exec
+# groups even if they don't actually use them. This is due to two interactions:
+# 1. Rules give an error if users pass an unsupported exec group.
+# 2. exec_properties is configurable, so macro-code can't always filter out
+#    exec group names that aren't supported by the rule.
+# The net effect is, if a user passes exec_properties to a macro, and the macro
+# invokes two rules, the macro can't always ensure each rule is only passed
+# valid exec groups, and is thus liable to cause an error.
+#
+# NOTE: These are no-op/empty exec groups. If a rule *does* support an exec
+# group and needs custom settings, it should merge this dict with one that
+# overrides the supported key.
+REQUIRED_EXEC_GROUPS = {
+    # py_binary may invoke C++ linking, or py rules may be used in combination
+    # with cc rules (e.g. within the same macro), so support that exec group.
+    "cpp_link": exec_group(),
+    "py_precompile": exec_group(),
+}
+
 _STAMP_VALUES = [-1, 0, 1]
 
 def _precompile_attr_get_effective_value(ctx):
@@ -50,7 +69,6 @@ def _precompile_attr_get_effective_value(ctx):
     if precompile not in (
         PrecompileAttr.ENABLED,
         PrecompileAttr.DISABLED,
-        PrecompileAttr.IF_GENERATED_SOURCE,
     ):
         fail("Unexpected final precompile value: {}".format(repr(precompile)))
 
@@ -60,14 +78,10 @@ def _precompile_attr_get_effective_value(ctx):
 PrecompileAttr = enum(
     # Determine the effective value from --precompile
     INHERIT = "inherit",
-    # Compile Python source files at build time. Note that
-    # --precompile_add_to_runfiles affects how the compiled files are included
-    # into a downstream binary.
+    # Compile Python source files at build time.
     ENABLED = "enabled",
     # Don't compile Python source files at build time.
     DISABLED = "disabled",
-    # Compile Python source files, but only if they're a generated file.
-    IF_GENERATED_SOURCE = "if_generated_source",
     get_effective_value = _precompile_attr_get_effective_value,
 )
 
@@ -90,7 +104,6 @@ def _precompile_source_retention_get_effective_value(ctx):
     if attr_value not in (
         PrecompileSourceRetentionAttr.KEEP_SOURCE,
         PrecompileSourceRetentionAttr.OMIT_SOURCE,
-        PrecompileSourceRetentionAttr.OMIT_IF_GENERATED_SOURCE,
     ):
         fail("Unexpected final precompile_source_retention value: {}".format(repr(attr_value)))
     return attr_value
@@ -100,14 +113,17 @@ PrecompileSourceRetentionAttr = enum(
     INHERIT = "inherit",
     KEEP_SOURCE = "keep_source",
     OMIT_SOURCE = "omit_source",
-    OMIT_IF_GENERATED_SOURCE = "omit_if_generated_source",
     get_effective_value = _precompile_source_retention_get_effective_value,
 )
 
 def _pyc_collection_attr_is_pyc_collection_enabled(ctx):
     pyc_collection = ctx.attr.pyc_collection
     if pyc_collection == PycCollectionAttr.INHERIT:
-        pyc_collection = ctx.attr._pyc_collection_flag[BuildSettingInfo].value
+        precompile_flag = PrecompileFlag.get_effective_value(ctx)
+        if precompile_flag in (PrecompileFlag.ENABLED, PrecompileFlag.FORCE_ENABLED):
+            pyc_collection = PycCollectionAttr.INCLUDE_PYC
+        else:
+            pyc_collection = PycCollectionAttr.DISABLED
 
     if pyc_collection not in (PycCollectionAttr.INCLUDE_PYC, PycCollectionAttr.DISABLED):
         fail("Unexpected final pyc_collection value: {}".format(repr(pyc_collection)))
@@ -282,13 +298,9 @@ Whether py source files **for this target** should be precompiled.
 
 Values:
 
-* `inherit`: Determine the value from the {flag}`--precompile` flag.
-* `enabled`: Compile Python source files at build time. Note that
-  --precompile_add_to_runfiles affects how the compiled files are included into
-  a downstream binary.
+* `inherit`: Allow the downstream binary decide if precompiled files are used.
+* `enabled`: Compile Python source files at build time.
 * `disabled`: Don't compile Python source files at build time.
-* `if_generated_source`: Compile Python source files, but only if they're a
-  generated file.
 
 :::{seealso}
 
@@ -343,8 +355,6 @@ in the resulting output or not. Valid values are:
 * `inherit`: Inherit the value from the {flag}`--precompile_source_retention` flag.
 * `keep_source`: Include the original Python source.
 * `omit_source`: Don't include the original py source.
-* `omit_if_generated_source`: Keep the original source if it's a regular source
-  file, but omit it if it's a generated file.
 """,
         ),
         # Required attribute, but details vary by rule.
@@ -356,10 +366,6 @@ in the resulting output or not. Valid values are:
         # Required attribute, but the details vary by rule.
         # Use create_srcs_version_attr to create one.
         "srcs_version": None,
-        "_precompile_add_to_runfiles_flag": attr.label(
-            default = "//python/config_settings:precompile_add_to_runfiles",
-            providers = [BuildSettingInfo],
-        ),
         "_precompile_flag": attr.label(
             default = "//python/config_settings:precompile",
             providers = [BuildSettingInfo],

python/private/common/common.bzl

@@ -348,15 +348,29 @@ def collect_runfiles(ctx, files = depset()):
         collect_default = True,
     )
 
-def create_py_info(ctx, *, direct_sources, direct_pyc_files, imports):
+def create_py_info(
+        ctx,
+        *,
+        required_py_files,
+        required_pyc_files,
+        implicit_pyc_files,
+        implicit_pyc_source_files,
+        imports):
     """Create PyInfo provider.
 
     Args:
         ctx: rule ctx.
-        direct_sources: depset of Files; the direct, raw `.py` sources for the
-            target. This should only be Python source files. It should not
-            include pyc files.
-        direct_pyc_files: depset of Files; the direct `.pyc` sources for the target.
+        required_py_files: `depset[File]`; the direct, `.py` sources for the
+            target that **must** be included by downstream targets. This should
+            only be Python source files. It should not include pyc files.
+        required_pyc_files: `depset[File]`; the direct `.pyc` files this target
+            produces.
+        implicit_pyc_files: `depset[File]` pyc files that are only used if pyc
+            collection is enabled.
+        implicit_pyc_source_files: `depset[File]` source files for implicit pyc
+            files that are used when the implicit pyc files are not.
+        implicit_pyc_files: {type}`depset[File]` Implicitly generated pyc files
+            that a binary can choose to include.
         imports: depset of strings; the import path values to propagate.
 
     Returns:
@@ -366,8 +380,10 @@ def create_py_info(ctx, *, direct_sources, direct_pyc_files, imports):
     """
 
     py_info = PyInfoBuilder()
-    py_info.direct_pyc_files.add(direct_pyc_files)
-    py_info.transitive_pyc_files.add(direct_pyc_files)
+    py_info.direct_pyc_files.add(required_pyc_files)
+    py_info.transitive_pyc_files.add(required_pyc_files)
+    py_info.transitive_implicit_pyc_files.add(implicit_pyc_files)
+    py_info.transitive_implicit_pyc_source_files.add(implicit_pyc_source_files)
     py_info.imports.add(imports)
     py_info.merge_has_py2_only_sources(ctx.attr.srcs_version in ("PY2", "PY2ONLY"))
     py_info.merge_has_py3_only_sources(ctx.attr.srcs_version in ("PY3", "PY3ONLY"))
@@ -386,7 +402,7 @@ def create_py_info(ctx, *, direct_sources, direct_pyc_files, imports):
                 py_info.merge_uses_shared_libraries(cc_helper.is_valid_shared_library_artifact(f))
 
     deps_transitive_sources = py_info.transitive_sources.build()
-    py_info.transitive_sources.add(direct_sources)
+    py_info.transitive_sources.add(required_py_files)
 
     # We only look at data to calculate uses_shared_libraries, if it's already
     # true, then we don't need to waste time looping over it.

python/private/common/common_bazel.bzl

@@ -14,7 +14,9 @@
 """Common functions that are specific to Bazel rule implementation"""
 
 load("@bazel_skylib//lib:paths.bzl", "paths")
+load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo")
 load("@rules_cc//cc:defs.bzl", "CcInfo", "cc_common")
+load("//python/private:flags.bzl", "PrecompileFlag")
 load("//python/private:py_interpreter_program.bzl", "PyInterpreterProgramInfo")
 load("//python/private:toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE", "TARGET_TOOLCHAIN_TYPE")
 load(":attributes.bzl", "PrecompileAttr", "PrecompileInvalidationModeAttr", "PrecompileSourceRetentionAttr")
@@ -60,7 +62,7 @@ def maybe_precompile(ctx, srcs):
     Returns:
         Struct of precompiling results with fields:
         * `keep_srcs`: list of File; the input sources that should be included
-          as default outputs and runfiles.
+          as default outputs.
         * `pyc_files`: list of File; the precompiled files.
         * `py_to_pyc_map`: dict of src File input to pyc File output. If a source
           file wasn't precompiled, it won't be in the dict.
@@ -72,36 +74,45 @@ def maybe_precompile(ctx, srcs):
     if exec_tools_toolchain == None or exec_tools_toolchain.exec_tools.precompiler == None:
         precompile = PrecompileAttr.DISABLED
     else:
-        precompile = PrecompileAttr.get_effective_value(ctx)
+        precompile_flag = ctx.attr._precompile_flag[BuildSettingInfo].value
+
+        if precompile_flag == PrecompileFlag.FORCE_ENABLED:
+            precompile = PrecompileAttr.ENABLED
+        elif precompile_flag == PrecompileFlag.FORCE_DISABLED:
+            precompile = PrecompileAttr.DISABLED
+        else:
+            precompile = ctx.attr.precompile
+
+    # Unless explicitly disabled, we always generate a pyc. This allows
+    # binaries to decide whether to include them or not later.
+    if precompile != PrecompileAttr.DISABLED:
+        should_precompile = True
+    else:
+        should_precompile = False
 
     source_retention = PrecompileSourceRetentionAttr.get_effective_value(ctx)
+    keep_source = (
+        not should_precompile or
+        source_retention == PrecompileSourceRetentionAttr.KEEP_SOURCE
+    )
 
     result = struct(
         keep_srcs = [],
         pyc_files = [],
         py_to_pyc_map = {},
     )
     for src in srcs:
-        # The logic below is a bit convoluted. The gist is:
-        # * If precompiling isn't done, add the py source to default outputs.
-        #   Otherwise, the source retention flag decides.
-        # * In order to determine `use_pycache`, we have to know if the source
-        #   is being added to the default outputs.
-        is_generated_source = not src.is_source
-        should_precompile = (
-            precompile == PrecompileAttr.ENABLED or
-            (precompile == PrecompileAttr.IF_GENERATED_SOURCE and is_generated_source)
-        )
-        keep_source = (
-            not should_precompile or
-            source_retention == PrecompileSourceRetentionAttr.KEEP_SOURCE or
-            (source_retention == PrecompileSourceRetentionAttr.OMIT_IF_GENERATED_SOURCE and not is_generated_source)
-        )
         if should_precompile:
+            # NOTE: _precompile() may return None
             pyc = _precompile(ctx, src, use_pycache = keep_source)
+        else:
+            pyc = None
+
+        if pyc:
             result.pyc_files.append(pyc)
             result.py_to_pyc_map[src] = pyc
-        if keep_source:
+
+        if keep_source or not pyc:
             result.keep_srcs.append(src)
 
     return result