Skip to content

Commit

Permalink
fix(windows): symlink bootstrap script when not building zip (bazelbu…
Browse files Browse the repository at this point in the history 
…ild#2015)

This fixes bazelbuild#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 <[email protected]>
  • Loading branch information
@KoltesDigital @rickeylev
KoltesDigital and rickeylev authored Jul 4, 2024
1 parent e6b9cff commit 2cfbe73
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 16 deletions.
54 changes: 39 additions & 15 deletions python/config_settings/transition.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<name>.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)
Expand All @@ -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,
Expand Down Expand Up @@ -169,13 +185,15 @@ _transition_py_binary = rule(
attrs = _COMMON_ATTRS | _PY_TEST_ATTRS,
cfg = _transition_python_version,
executable = True,
fragments = ["py"],
)

_transition_py_test = rule(
_transition_py_impl,
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):
Expand Down Expand Up @@ -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
77 changes: 76 additions & 1 deletion tests/config_settings/transition/multi_version_tests.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 2cfbe73

Please sign in to comment.