diff --git a/news/4113.fixed.md b/news/4113.fixed.md new file mode 100644 index 0000000000..e852e1ac96 --- /dev/null +++ b/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)). diff --git a/python/private/BUILD.bazel b/python/private/BUILD.bazel index 0b0dc97f66..72bf7138f8 100644 --- a/python/private/BUILD.bazel +++ b/python/private/BUILD.bazel @@ -469,6 +469,7 @@ bzl_library( srcs = ["precompile.bzl"], deps = [ ":attributes", + ":common", ":flags", ":py_interpreter_program", ":toolchain_types", diff --git a/python/private/attributes.bzl b/python/private/attributes.bzl index e1e77cba03..5b842f85bc 100644 --- a/python/private/attributes.bzl +++ b/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( diff --git a/python/private/common.bzl b/python/private/common.bzl index 7e8c6decf3..5cff7f8723 100644 --- a/python/private/common.bzl +++ b/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: diff --git a/python/private/precompile.bzl b/python/private/precompile.bzl index 898dc3ee2a..ceaa76cf88 100644 --- a/python/private/precompile.bzl +++ b/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,7 +98,7 @@ 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 @@ -106,6 +106,12 @@ def _precompile(ctx, src, *, use_pycache): 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 diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index 0e9c315a73..368a785686 100644 --- a/python/private/py_executable.bzl +++ b/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) diff --git a/python/private/py_library.bzl b/python/private/py_library.bzl index 3118d3c9a1..6282b71ee1 100644 --- a/python/private/py_library.bzl +++ b/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 diff --git a/tests/base_rules/precompile/precompile_tests.bzl b/tests/base_rules/precompile/precompile_tests.bzl index d2c1da6b8f..63865a30e6 100644 --- a/tests/base_rules/precompile/precompile_tests.bzl +++ b/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) diff --git a/tests/base_rules/py_library/py_library_tests.bzl b/tests/base_rules/py_library/py_library_tests.bzl index a3be7c4bcf..80e3e1ba22 100644 --- a/tests/base_rules/py_library/py_library_tests.bzl +++ b/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)