From b0bda9706fd8ab9ebf6b9dd4fd4b5daad53dadae Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 24 Jun 2024 11:18:30 -0700 Subject: [PATCH 1/2] fix: make first default output the executable again This fixes a small change in behavior identified by some Google regression tests. When precompiling was introduced, the target's executable was no longer the first file in the default outputs depset. While that behavior isn't a strong contract, it is the convention with many other rules, and the existing historical behavior. To fix, put the executable as the first value in the default outputs list. Also adds a test for this behavior. --- CHANGELOG.md | 1 + python/private/common/py_executable.bzl | 5 +++-- tests/base_rules/py_executable_base_tests.bzl | 9 +++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b259e51fbe..6ae6508f21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ A brief description of the categories of changes: `interpreter_version_info` arg. * (bzlmod) Correctly pass `isolated`, `quiet` and `timeout` values to `whl_library` and drop the defaults from the lock file. +* (rules) The first element of the default outputs is now the executable again. ### Removed * (pip): Removes the `entrypoint` macro that was replaced by `py_console_script_binary` in 0.26.0. diff --git a/python/private/common/py_executable.bzl b/python/private/common/py_executable.bzl index 6b75b4139a..2b4a9397c8 100644 --- a/python/private/common/py_executable.bzl +++ b/python/private/common/py_executable.bzl @@ -166,9 +166,10 @@ def py_executable_base_impl(ctx, *, semantics, is_test, inherited_environment = main_py = precompile_result.py_to_pyc_map[main_py] direct_pyc_files = depset(precompile_result.pyc_files) - default_outputs = precompile_result.keep_srcs + precompile_result.pyc_files executable = _declare_executable_file(ctx) - default_outputs.append(executable) + default_outputs = [executable] + default_outputs.extend(precompile_result.keep_srcs) + default_outputs.extend(precompile_result.pyc_files) imports = collect_imports(ctx, semantics) diff --git a/tests/base_rules/py_executable_base_tests.bzl b/tests/base_rules/py_executable_base_tests.bzl index c96ec4e108..8e8bebcbb0 100644 --- a/tests/base_rules/py_executable_base_tests.bzl +++ b/tests/base_rules/py_executable_base_tests.bzl @@ -297,6 +297,15 @@ def _test_files_to_build_impl(env, target): "{package}/{test_name}_subject.py", ]) + # Convention and historical behavior is that the first default output is + # the executable, so verify that is the case. + # rules_testing DepsetFileSubject.contains_exactly doesn't provide an + # in_order() call, nor access to the underlying depset, so we have to + # do things manually. + first_default_output = target[DefaultInfo].files.to_list()[0] + executable = target[DefaultInfo].files_to_run.executable + env.expect.that_file(first_default_output).equals(executable) + def _test_name_cannot_end_in_py(name, config): # Bazel 5 will crash with a Java stacktrace when the native Python # rules have an error. From 40149b266acd83ddae09eb7ac0af8806821a9645 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 24 Jun 2024 11:41:04 -0700 Subject: [PATCH 2/2] fix ci: behavior is bazel 7+ --- tests/base_rules/py_executable_base_tests.bzl | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/base_rules/py_executable_base_tests.bzl b/tests/base_rules/py_executable_base_tests.bzl index 8e8bebcbb0..eb1a1b6c07 100644 --- a/tests/base_rules/py_executable_base_tests.bzl +++ b/tests/base_rules/py_executable_base_tests.bzl @@ -18,6 +18,7 @@ load("@rules_python_internal//:rules_python_config.bzl", rp_config = "config") load("@rules_testing//lib:analysis_test.bzl", "analysis_test") load("@rules_testing//lib:truth.bzl", "matching") load("@rules_testing//lib:util.bzl", rt_util = "util") +load("//python/private:util.bzl", "IS_BAZEL_7_OR_HIGHER") # buildifier: disable=bzl-visibility load("//tests/base_rules:base_tests.bzl", "create_base_tests") load("//tests/base_rules:util.bzl", "WINDOWS_ATTR", pt_util = "util") load("//tests/support:support.bzl", "LINUX_X86_64", "WINDOWS_X86_64") @@ -297,14 +298,15 @@ def _test_files_to_build_impl(env, target): "{package}/{test_name}_subject.py", ]) - # Convention and historical behavior is that the first default output is - # the executable, so verify that is the case. - # rules_testing DepsetFileSubject.contains_exactly doesn't provide an - # in_order() call, nor access to the underlying depset, so we have to - # do things manually. - first_default_output = target[DefaultInfo].files.to_list()[0] - executable = target[DefaultInfo].files_to_run.executable - env.expect.that_file(first_default_output).equals(executable) + if IS_BAZEL_7_OR_HIGHER: + # As of Bazel 7, the first default output is the executable, so + # verify that is the case. rules_testing + # DepsetFileSubject.contains_exactly doesn't provide an in_order() + # call, nor access to the underlying depset, so we have to do things + # manually. + first_default_output = target[DefaultInfo].files.to_list()[0] + executable = target[DefaultInfo].files_to_run.executable + env.expect.that_file(first_default_output).equals(executable) def _test_name_cannot_end_in_py(name, config): # Bazel 5 will crash with a Java stacktrace when the native Python