fix(precompile): skip precompilation for directory and pyc inputs in srcs

news/4113.fixed.md

@@ -0,0 +1,4 @@
+(precompile) Fixed handling of directory and `.pyc` file inputs in
+{obj}`srcs` when {obj}`precompile` is enabled on {obj}`py_library`,
+{obj}`py_binary`, and {obj}`py_test` targets
+([#4113](https://github.com/bazel-contrib/rules_python/pull/4113)).

python/private/BUILD.bazel

@@ -469,6 +469,7 @@ bzl_library(
     srcs = ["precompile.bzl"],
     deps = [
         ":attributes",
+        ":common",
         ":flags",
         ":py_interpreter_program",
         ":toolchain_types",

python/private/attributes.bzl

@@ -331,14 +331,29 @@ as part of a runnable program (packaging rules may include them, however).
             allow_files = True,
         ),
         "srcs": lambda: attrb.LabelList(
-            allow_files = [".py", ".py3"],
+            allow_files = True,
             # Necessary for --compile_one_dependency to work.
             flags = ["DIRECT_COMPILE_TIME_INPUT"],
             doc = """
 The list of Python source files that are processed to create the target. This
-includes all your checked-in code and may include generated source files.  The
-`.py` files belong in `srcs` and library targets belong in `deps`. Other binary
-files that may be needed at run time belong in `data`.
+includes all your checked-in code and may include generated source files.
+
+Allowed file types:
+* `.py`
+* `.pyc`
+* directories
+
+Library targets belong in `deps`. Other binary files that may be needed at run
+time belong in `data`.
+
+:::{versionchanged} 2.3.2
+As an exception, empty targets in `srcs` that provide {obj}`PyInfo` are
+allowed. Ordinary library dependencies should remain in `deps`.
+:::
+
+:::{versionchanged} VERSION_NEXT_PATCH
+Allowed `.pyc` and directory inputs in `srcs`.
+:::
 """,
         ),
         "srcs_version": lambda: attrb.String(

python/private/common.bzl

@@ -198,13 +198,21 @@ def csv(values):
     """Convert a list of strings to comma separated value string."""
     return ", ".join(sorted(values))
 
+def is_py_source(f):
+    """Whether the given file is considered a Python source file."""
+    return f.extension == "py"
+
 def filter_to_py_srcs(srcs):
     """Filters .py files from the given list of files"""
+    return [f for f in srcs if is_py_source(f)]
 
-    # TODO(b/203567235): Get the set of recognized extensions from
-    # elsewhere, as there may be others. e.g. Bazel recognizes .py3
-    # as a valid extension.
-    return [f for f in srcs if f.extension == "py"]
+def filter_to_direct_sources(srcs):
+    """Filters Python sources, pyc files, and directory artifacts from srcs."""
+    return [
+        f
+        for f in srcs
+        if f.is_directory or is_py_source(f) or f.extension == "pyc"
+    ]
 
 def collect_cc_info(ctx, extra_deps = []):
     """Collect C++ information from dependencies for Bazel.
@@ -398,7 +406,7 @@ def create_py_info(
             # longer supported in `deps`.
             files = target[DefaultInfo].files.to_list()
             for f in files:
-                if f.extension == "py":
+                if is_py_source(f):
                     py_info.transitive_sources.add(f)
                 py_info.merge_uses_shared_libraries(cc_helper.is_valid_shared_library_artifact(f))
     for target in ctx.attr.pyi_deps:

python/private/precompile.bzl

@@ -15,7 +15,7 @@
 
 load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo")
 load(":attributes.bzl", "PrecompileAttr", "PrecompileInvalidationModeAttr", "PrecompileSourceRetentionAttr")
-load(":common.bzl", "actions_run")
+load(":common.bzl", "actions_run", "is_py_source")
 load(":flags.bzl", "PrecompileFlag")
 load(":py_interpreter_program.bzl", "PyInterpreterProgramInfo")
 load(":toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE", "TARGET_TOOLCHAIN_TYPE")
@@ -98,14 +98,20 @@ def _precompile(ctx, src, *, use_pycache):
             file.
 
     Returns:
-        File of the generated pyc file.
+        File of the generated pyc file, or None if the source file was skipped.
     """
 
     # Generating a file in another package is an error, so we have to skip
     # such cases.
     if ctx.label.package != src.owner.package:
         return None
 
+    if src.is_directory:
+        return None
+
+    if not is_py_source(src):
+        return None
+
     exec_tools_info = ctx.toolchains[EXEC_TOOLS_TOOLCHAIN_TYPE].exec_tools
     target_toolchain = ctx.toolchains[TARGET_TOOLCHAIN_TYPE].py3_runtime
 

python/private/py_executable.bzl

@@ -50,6 +50,7 @@ load(
     "create_py_info",
     "create_windows_exe_launcher",
     "csv",
+    "filter_to_direct_sources",
     "filter_to_py_srcs",
     "is_bool",
     "is_windows_platform",
@@ -1198,13 +1199,13 @@ def py_executable_base_impl(ctx, *, semantics, is_test, inherited_environment =
     # precompiled pyc below) so the test-main validation can statically analyze
     # the original source.
     main_py_source = main_py
-    direct_sources = filter_to_py_srcs(ctx.files.srcs)
+    direct_sources = filter_to_direct_sources(ctx.files.srcs)
     precompile_result = maybe_precompile(ctx, direct_sources)
 
-    required_py_files = precompile_result.keep_srcs
+    required_py_files = filter_to_py_srcs(precompile_result.keep_srcs)
     required_pyc_files = []
     implicit_pyc_files = []
-    implicit_pyc_source_files = direct_sources
+    implicit_pyc_source_files = filter_to_py_srcs(direct_sources)
 
     if ctx.attr.precompile == PrecompileAttr.ENABLED:
         required_pyc_files.extend(precompile_result.pyc_files)

python/private/py_library.bzl

@@ -34,6 +34,7 @@ load(
     "create_instrumented_files_info",
     "create_output_group_info",
     "create_py_info",
+    "filter_to_direct_sources",
     "filter_to_py_srcs",
 )
 load(":common_labels.bzl", "labels")
@@ -128,19 +129,13 @@ def _validate_srcs(ctx):
         ):
             continue
 
-        found_match = False
-        for file in files:
-            if file.is_directory or file.extension in ("py", "py3"):
-                found_match = True
-                break
-
-        if found_match:
+        if filter_to_direct_sources(files):
             continue
 
         fail(
             ("{} does not produce any py_library srcs files " +
-             "(expected .py or .py3) and is not an empty target providing " +
-             "PyInfo").format(
+             "(expected .py, .pyc, or directory) and is not an empty target " +
+             "providing PyInfo").format(
                 target.label,
             ),
             attr = "srcs",
@@ -156,14 +151,14 @@ def py_library_impl(ctx):
         A list of modern providers to propagate.
     """
     _validate_srcs(ctx)
-    direct_sources = filter_to_py_srcs(ctx.files.srcs)
+    direct_sources = filter_to_direct_sources(ctx.files.srcs)
 
     precompile_result = maybe_precompile(ctx, direct_sources)
 
-    required_py_files = precompile_result.keep_srcs
+    required_py_files = filter_to_py_srcs(precompile_result.keep_srcs)
     required_pyc_files = []
     implicit_pyc_files = []
-    implicit_pyc_source_files = direct_sources
+    implicit_pyc_source_files = filter_to_py_srcs(direct_sources)
 
     precompile_attr = ctx.attr.precompile
     precompile_flag = ctx.attr._precompile_flag[BuildSettingInfo].value
@@ -314,7 +309,7 @@ def create_py_library_rule_builder():
         {obj}`ruleb.Rule` with the necessary settings
         for creating a `py_library` rule.
     """
-    builder = ruleb.Rule(
+    return ruleb.Rule(
         implementation = py_library_impl,
         doc = _DEFAULT_PY_LIBRARY_DOC,
         exec_groups = dict(REQUIRED_EXEC_GROUP_BUILDERS),
@@ -326,13 +321,3 @@ def create_py_library_rule_builder():
             ruleb.ToolchainType(EXEC_TOOLS_TOOLCHAIN_TYPE, mandatory = False),
         ],
     )
-    srcs_attr = builder.attrs.get("srcs")
-    srcs_attr.set_allow_files(True)
-    srcs_attr.set_doc(srcs_attr.doc() + """
-
-:::{versionchanged} 2.3.2
-As an exception, empty targets in `srcs` that provide {obj}`PyInfo` are
-allowed. Ordinary library dependencies should remain in `deps`.
-:::
-""")
-    return builder

tests/base_rules/precompile/precompile_tests.bzl

@@ -529,6 +529,148 @@ def _test_precompile_enabled_succeeds(name):
 
 _tests.append(_test_precompile_enabled_succeeds)
 
+def _directory_impl(ctx):
+    out = ctx.actions.declare_directory(ctx.label.name)
+    ctx.actions.run_shell(
+        outputs = [out],
+        command = """\
+mkdir -p "$1"
+echo "x = 1" > "$1/foo.py"
+echo "y = 2" > "$1/bar.py"
+""",
+        arguments = [out.path],
+        mnemonic = "TestDirectory",
+    )
+    return [DefaultInfo(files = depset([out]))]
+
+_directory = rule(implementation = _directory_impl)
+
+def _test_directory_input(name):
+    rt_util.helper_target(
+        _directory,
+        name = name + "_dir.py",
+    )
+    rt_util.helper_target(
+        py_library,
+        name = name + "_subject",
+        srcs = ["lib.py", name + "_dir.py"],
+        precompile = "enabled",
+    )
+    analysis_test(
+        name = name,
+        impl = _test_directory_input_impl,
+        target = name + "_subject",
+        config_settings = _COMMON_CONFIG_SETTINGS,
+    )
+
+def _test_directory_input_impl(env, target):
+    target = env.expect.that_target(target)
+    target.default_outputs().contains_at_least_predicates([
+        matching.file_path_matches("__pycache__/lib.fakepy-45.pyc"),
+        matching.file_path_matches("/lib.py"),
+        matching.file_path_matches("/" + env.ctx.label.name + "_dir.py"),
+    ])
+    py_info = target.provider(PyInfo, factory = py_info_subject)
+    py_info.direct_pyc_files().contains_exactly([
+        "{package}/__pycache__/lib.fakepy-45.pyc",
+    ])
+    py_info.transitive_pyc_files().contains_exactly([
+        "{package}/__pycache__/lib.fakepy-45.pyc",
+    ])
+
+_tests.append(_test_directory_input)
+
+# buildifier: disable=function-docstring-header
+def _test_directory_input_succeeds(name):
+    """Verify that a `py_test` target with a directory input in srcs builds
+    and runs when precompiling is enabled.
+    """
+    _directory(
+        name = name + "_dir.py",
+    )
+    write_file(
+        name = name + "_main",
+        out = name + "_main.py",
+        content = [
+            "print('Hello from directory input test')",
+            "",
+        ],
+    )
+    py_test(
+        name = name,
+        srcs = [name + "_main.py", name + "_dir.py"],
+        main = name + "_main.py",
+        precompile = "enabled",
+        tags = ["no-pyrefly"],
+    )
+
+_tests.append(_test_directory_input_succeeds)
+
+def _test_pyc_source_input(name):
+    rt_util.helper_target(
+        write_file,
+        name = name + "_pyc",
+        out = name + "_foo.pyc",
+        content = [""],
+    )
+    rt_util.helper_target(
+        py_library,
+        name = name + "_subject",
+        srcs = ["lib.py", name + "_pyc"],
+        precompile = "enabled",
+    )
+    analysis_test(
+        name = name,
+        impl = _test_pyc_source_input_impl,
+        target = name + "_subject",
+        config_settings = _COMMON_CONFIG_SETTINGS,
+    )
+
+def _test_pyc_source_input_impl(env, target):
+    target = env.expect.that_target(target)
+    target.default_outputs().contains_at_least_predicates([
+        matching.file_path_matches("__pycache__/lib.fakepy-45.pyc"),
+        matching.file_path_matches("/lib.py"),
+        matching.file_path_matches("/" + env.ctx.label.name + "_foo.pyc"),
+    ])
+    py_info = target.provider(PyInfo, factory = py_info_subject)
+    py_info.direct_pyc_files().contains_exactly([
+        "{package}/__pycache__/lib.fakepy-45.pyc",
+    ])
+    py_info.transitive_pyc_files().contains_exactly([
+        "{package}/__pycache__/lib.fakepy-45.pyc",
+    ])
+
+_tests.append(_test_pyc_source_input)
+
+# buildifier: disable=function-docstring-header
+def _test_pyc_source_input_succeeds(name):
+    """Verify that a `py_test` target with a pyc input in srcs builds
+    and runs when precompiling is enabled.
+    """
+    write_file(
+        name = name + "_pyc",
+        out = name + "_foo.pyc",
+        content = [""],
+    )
+    write_file(
+        name = name + "_main",
+        out = name + "_main.py",
+        content = [
+            "print('Hello from pyc input test')",
+            "",
+        ],
+    )
+    py_test(
+        name = name,
+        srcs = [name + "_main.py", name + "_pyc"],
+        main = name + "_main.py",
+        precompile = "enabled",
+        tags = ["no-pyrefly"],
+    )
+
+_tests.append(_test_pyc_source_input_succeeds)
+
 def runfiles_contains_at_least_predicates(runfiles, predicates):
     for predicate in predicates:
         runfiles.contains_predicate(predicate)

tests/base_rules/py_library/py_library_tests.bzl

@@ -134,7 +134,9 @@ def _test_srcs_can_contain_tree_artifact(name, config):
     )
 
 def _test_srcs_can_contain_tree_artifact_impl(env, target):
-    env.expect.that_target(target).default_outputs().contains_exactly([])
+    env.expect.that_target(target).default_outputs().contains_exactly([
+        "{package}/{test_name}_tree.dir",
+    ])
 
 _tests.append(_test_srcs_can_contain_tree_artifact)