From 2cfbe731332426bbbed31f4e02140290bbf86d16 Mon Sep 17 00:00:00 2001 From: "Jonathan Giroux (Koltes)" Date: Fri, 5 Jul 2024 01:50:44 +0200 Subject: [PATCH] fix(windows): symlink bootstrap script when not building zip (#2015) This fixes #1840 by supporting again `--build_python_zip=false` on Windows. When the zip file isn't build, the transition executable looks for the eponymous bootstrap script but the latter doesn't exist. I've just added a symlink, and refactored a bit because logic would have been duplicated. It seems you don't run CICD on Windows. FWIW I've manually tested it, both with `build_python_zip` as `true` and `false`. --------- Co-authored-by: Richard Levasseur --- python/config_settings/transition.bzl | 54 +++++++++---- .../transition/multi_version_tests.bzl | 77 ++++++++++++++++++- 2 files changed, 115 insertions(+), 16 deletions(-) diff --git a/python/config_settings/transition.bzl b/python/config_settings/transition.bzl index 48b0447ed..da48a1fdb 100644 --- a/python/config_settings/transition.bzl +++ b/python/config_settings/transition.bzl @@ -43,24 +43,40 @@ def _transition_py_impl(ctx): output = executable, target_file = target[DefaultInfo].files_to_run.executable, ) - zipfile_symlink = None + default_outputs = [] if target_is_windows: - # Under Windows, the expected ".zip" does not exist, so we have to - # create the symlink ourselves to achieve the same behaviour as in macOS - # and Linux. - zipfile = None - expected_target_path = target[DefaultInfo].files_to_run.executable.short_path[:-4] + ".zip" - for file in target[DefaultInfo].default_runfiles.files.to_list(): - if file.short_path == expected_target_path: - zipfile = file + # NOTE: Bazel 6 + host=linux + target=windows results in the .exe extension missing + inner_bootstrap_path = _strip_suffix(target[DefaultInfo].files_to_run.executable.short_path, ".exe") + inner_bootstrap = None + inner_zip_file_path = inner_bootstrap_path + ".zip" + inner_zip_file = None + for file in target[DefaultInfo].files.to_list(): + if file.short_path == inner_bootstrap_path: + inner_bootstrap = file + elif file.short_path == inner_zip_file_path: + inner_zip_file = file - if zipfile: - zipfile_symlink = ctx.actions.declare_file(ctx.attr.name + ".zip") + # TODO: Use `fragments.py.build_python_zip` once Bazel 6 support is dropped. + # Which file the Windows .exe looks for depends on the --build_python_zip file. + # Bazel 7+ has APIs to know the effective value of that flag, but not Bazel 6. + # To work around this, we treat the existence of a .zip in the default outputs + # to mean --build_python_zip=true. + if inner_zip_file: + suffix = ".zip" + underlying_launched_file = inner_zip_file + else: + suffix = "" + underlying_launched_file = inner_bootstrap + + if underlying_launched_file: + launched_file_symlink = ctx.actions.declare_file(ctx.attr.name + suffix) ctx.actions.symlink( is_executable = True, - output = zipfile_symlink, - target_file = zipfile, + output = launched_file_symlink, + target_file = underlying_launched_file, ) + default_outputs.append(launched_file_symlink) + env = {} for k, v in ctx.attr.env.items(): env[k] = ctx.expand_location(v) @@ -85,8 +101,8 @@ def _transition_py_impl(ctx): providers = [ DefaultInfo( executable = executable, - files = depset([zipfile_symlink] if zipfile_symlink else [], transitive = [target[DefaultInfo].files]), - runfiles = ctx.runfiles([zipfile_symlink] if zipfile_symlink else []).merge(target[DefaultInfo].default_runfiles), + files = depset(default_outputs, transitive = [target[DefaultInfo].files]), + runfiles = ctx.runfiles(default_outputs).merge(target[DefaultInfo].default_runfiles), ), py_info, py_runtime_info, @@ -169,6 +185,7 @@ _transition_py_binary = rule( attrs = _COMMON_ATTRS | _PY_TEST_ATTRS, cfg = _transition_python_version, executable = True, + fragments = ["py"], ) _transition_py_test = rule( @@ -176,6 +193,7 @@ _transition_py_test = rule( attrs = _COMMON_ATTRS | _PY_TEST_ATTRS, cfg = _transition_python_version, test = True, + fragments = ["py"], ) def _py_rule(rule_impl, transition_rule, name, python_version, **kwargs): @@ -263,3 +281,9 @@ def py_binary(name, python_version, **kwargs): def py_test(name, python_version, **kwargs): return _py_rule(_py_test, _transition_py_test, name, python_version, **kwargs) + +def _strip_suffix(s, suffix): + if s.endswith(suffix): + return s[:-len(suffix)] + else: + return s diff --git a/tests/config_settings/transition/multi_version_tests.bzl b/tests/config_settings/transition/multi_version_tests.bzl index f3707dba2..e35590bbb 100644 --- a/tests/config_settings/transition/multi_version_tests.bzl +++ b/tests/config_settings/transition/multi_version_tests.bzl @@ -15,8 +15,9 @@ load("@rules_testing//lib:analysis_test.bzl", "analysis_test") load("@rules_testing//lib:test_suite.bzl", "test_suite") -load("@rules_testing//lib:util.bzl", rt_util = "util") +load("@rules_testing//lib:util.bzl", "TestingAspectInfo", rt_util = "util") load("//python/config_settings:transition.bzl", py_binary_transitioned = "py_binary", py_test_transitioned = "py_test") +load("//python/private:util.bzl", "IS_BAZEL_7_OR_HIGHER") # buildifier: disable=bzl-visibility # NOTE @aignas 2024-06-04: we are using here something that is registered in the MODULE.Bazel # and if you find tests failing, it could be because of the toolchain resolution issues here. @@ -68,6 +69,80 @@ def _test_py_binary_with_transition_impl(env, target): _tests.append(_test_py_binary_with_transition) +def _setup_py_binary_windows(name, *, impl, build_python_zip): + rt_util.helper_target( + py_binary_transitioned, + name = name + "_subject", + srcs = [name + "_subject.py"], + python_version = _PYTHON_VERSION, + ) + + analysis_test( + name = name, + target = name + "_subject", + impl = impl, + config_settings = { + "//command_line_option:build_python_zip": build_python_zip, + "//command_line_option:extra_toolchains": "//tests/cc:all", + "//command_line_option:platforms": str(Label("//tests/support:windows_x86_64")), + }, + ) + +def _test_py_binary_windows_build_python_zip_false(name): + _setup_py_binary_windows( + name, + build_python_zip = "false", + impl = _test_py_binary_windows_build_python_zip_false_impl, + ) + +def _test_py_binary_windows_build_python_zip_false_impl(env, target): + default_outputs = env.expect.that_target(target).default_outputs() + if IS_BAZEL_7_OR_HIGHER: + # TODO: These outputs aren't correct. The outputs shouldn't + # have the "_" prefix on them (those are coming from the underlying + # wrapped binary). + env.expect.that_target(target).default_outputs().contains_exactly([ + "{package}/_{test_name}_subject", + "{package}/_{test_name}_subject.exe", + "{package}/{test_name}_subject", + "{package}/{test_name}_subject.py", + ]) + else: + inner_exe = target[TestingAspectInfo].attrs.target[DefaultInfo].files_to_run.executable + default_outputs.contains_at_least([ + inner_exe.short_path, + ]) + +_tests.append(_test_py_binary_windows_build_python_zip_false) + +def _test_py_binary_windows_build_python_zip_true(name): + _setup_py_binary_windows( + name, + build_python_zip = "true", + impl = _test_py_binary_windows_build_python_zip_true_impl, + ) + +def _test_py_binary_windows_build_python_zip_true_impl(env, target): + default_outputs = env.expect.that_target(target).default_outputs() + if IS_BAZEL_7_OR_HIGHER: + # TODO: These outputs aren't correct. The outputs shouldn't + # have the "_" prefix on them (those are coming from the underlying + # wrapped binary). + default_outputs.contains_exactly([ + "{package}/_{test_name}_subject.exe", + "{package}/_{test_name}_subject.zip", + "{package}/{test_name}_subject.py", + "{package}/{test_name}_subject.zip", + ]) + else: + inner_exe = target[TestingAspectInfo].attrs.target[DefaultInfo].files_to_run.executable + default_outputs.contains_at_least([ + "{package}/{test_name}_subject.zip", + inner_exe.short_path, + ]) + +_tests.append(_test_py_binary_windows_build_python_zip_true) + def multi_version_test_suite(name): test_suite( name = name,