diff --git a/news/4114.fixed.md b/news/4114.fixed.md new file mode 100644 index 0000000000..b68e75bf91 --- /dev/null +++ b/news/4114.fixed.md @@ -0,0 +1,3 @@ +(pypi) Fixed wheel RECORD file generation on Windows so that rewritten shebang +scripts correctly include the `.bat` extension +([#4114](https://github.com/bazel-contrib/rules_python/pull/4114)). diff --git a/python/private/pypi/gen_wheel_record.bzl b/python/private/pypi/gen_wheel_record.bzl index 6dd211f776..e85f6b98fa 100644 --- a/python/private/pypi/gen_wheel_record.bzl +++ b/python/private/pypi/gen_wheel_record.bzl @@ -47,6 +47,7 @@ def _gen_wheel_record_impl(ctx): action_args.add(out_file) action_args.add("windows" if is_windows else "unix") action_args.add(data_dir_basename) + action_args.add_all(ctx.attr.rewritten_scripts) ctx.actions.run( inputs = inputs, @@ -65,6 +66,10 @@ def _gen_wheel_record_impl(ctx): gen_wheel_record = rule( implementation = _gen_wheel_record_impl, attrs = WINDOWS_CONSTRAINTS_PLAIN_ATTRS | { + "rewritten_scripts": attr.string_list( + doc = "List of script names that had shebang rewriting applied.", + default = [], + ), "srcs": attr.label_list( doc = "The original RECORD files to rewrite.", mandatory = True, diff --git a/python/private/pypi/wheel_record_rewriter.ps1 b/python/private/pypi/wheel_record_rewriter.ps1 index 60dd0eca11..6b6f0de289 100644 --- a/python/private/pypi/wheel_record_rewriter.ps1 +++ b/python/private/pypi/wheel_record_rewriter.ps1 @@ -10,7 +10,10 @@ param( [string]$TargetOs, [Parameter(Position=3, Mandatory=$true)] - [string]$DataDirBasename + [string]$DataDirBasename, + + [Parameter(Position=4, ValueFromRemainingArguments=$true)] + [string[]]$RewrittenScripts ) $ErrorActionPreference = "Stop" @@ -32,6 +35,13 @@ if ($TargetOs -eq "windows") { $scriptsRepl = "../../../bin/" } +$rewrittenSet = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::Ordinal) +if ($RewrittenScripts) { + foreach ($s in $RewrittenScripts) { + $null = $rewrittenSet.Add($s) + } +} + $lines = Get-Content -Path $InFile $outLines = [System.Collections.Generic.List[string]]::new() $Utf8NoBom = New-Object System.Text.UTF8Encoding $False @@ -53,7 +63,24 @@ foreach ($line in $lines) { } elseif ($rest.StartsWith("platlib/")) { $outLines.Add($quote + $platlibRepl + $rest.Substring(8)) } elseif ($rest.StartsWith("scripts/")) { - $outLines.Add($quote + $scriptsRepl + $rest.Substring(8)) + $entry = $rest.Substring(8) + if ($TargetOs -eq "windows") { + if ($quote -eq "`"") { + $idx = $entry.IndexOf("`"") + $spath = $entry.Substring(0, $idx) + $suffix = $entry.Substring($idx) + } else { + $idx = $entry.IndexOf(",") + $spath = $entry.Substring(0, $idx) + $suffix = $entry.Substring($idx) + } + if ($rewrittenSet.Contains($spath)) { + $spath = "$spath.bat" + } + $outLines.Add($quote + $scriptsRepl + $spath + $suffix) + } else { + $outLines.Add($quote + $scriptsRepl + $entry) + } } elseif ($rest.StartsWith("headers/")) { $outLines.Add($quote + $headersRepl + $rest.Substring(8)) } elseif ($rest.StartsWith("data/")) { diff --git a/python/private/pypi/wheel_record_rewriter.sh b/python/private/pypi/wheel_record_rewriter.sh index 10189435e1..4d93f2a3ae 100755 --- a/python/private/pypi/wheel_record_rewriter.sh +++ b/python/private/pypi/wheel_record_rewriter.sh @@ -5,6 +5,7 @@ IN="$1" OUT="$2" TARGET_OS="$3" DATA_DIR_BASENAME="$4" +shift 4 DATA_PREFIX="${DATA_DIR_BASENAME}/" QUOTED_DATA_PREFIX="\"${DATA_DIR_BASENAME}/" @@ -29,7 +30,14 @@ awk -v data_prefix="$DATA_PREFIX" \ -v headers_repl="$HEADERS_REPL" \ -v platlib_repl="$PLATLIB_REPL" \ -v purelib_repl="$PURELIB_REPL" \ - -v scripts_repl="$SCRIPTS_REPL" ' + -v scripts_repl="$SCRIPTS_REPL" \ + -v target_os="$TARGET_OS" ' +BEGIN { + for (i = 2; i < ARGC; i++) { + rewritten[ARGV[i]] = 1 + } + ARGC = 2 +} { line = $0 quote = "" @@ -48,7 +56,24 @@ awk -v data_prefix="$DATA_PREFIX" \ } else if (substr(rest, 1, 8) == "platlib/") { print quote platlib_repl substr(rest, 9) } else if (substr(rest, 1, 8) == "scripts/") { - print quote scripts_repl substr(rest, 9) + entry = substr(rest, 9) + if (target_os == "windows") { + if (quote == "\"") { + idx = index(entry, "\"") + spath = substr(entry, 1, idx - 1) + suffix = substr(entry, idx) + } else { + idx = index(entry, ",") + spath = substr(entry, 1, idx - 1) + suffix = substr(entry, idx) + } + if (spath in rewritten) { + spath = spath ".bat" + } + print quote scripts_repl spath suffix + } else { + print quote scripts_repl entry + } } else if (substr(rest, 1, 8) == "headers/") { print quote headers_repl substr(rest, 9) } else if (substr(rest, 1, 5) == "data/") { @@ -57,4 +82,4 @@ awk -v data_prefix="$DATA_PREFIX" \ print line } } -' "$IN" > "$OUT" +' "$IN" "$@" > "$OUT" diff --git a/python/private/pypi/whl_library_srcs.bzl b/python/private/pypi/whl_library_srcs.bzl index 80a0d8a6dc..128c46e128 100644 --- a/python/private/pypi/whl_library_srcs.bzl +++ b/python/private/pypi/whl_library_srcs.bzl @@ -100,6 +100,7 @@ def whl_library_srcs( for p in native.glob(["bin/*"], allow_empty = True): existing_bin_names[p[len("bin/"):].lower()] = None + rewritten_script_names = [] for src_path in native.glob(["rewrite-bin/*"], allow_empty = True): script_name = src_path[len("rewrite-bin/"):] if script_name.lower() in existing_bin_names: @@ -112,6 +113,7 @@ def whl_library_srcs( ) bins_for_data_label.append(rewrite_target_name) data.append(rewrite_target_name) + rewritten_script_names.append(script_name) record_srcs = native.glob(["rewrite-record/*/RECORD"], allow_empty = True) record_target_name = "record" @@ -119,6 +121,7 @@ def whl_library_srcs( rules.gen_wheel_record( name = record_target_name, srcs = record_srcs, + rewritten_scripts = rewritten_script_names, tags = ["manual"], ) data.append(record_target_name) diff --git a/tests/pypi/whl_extract/wheel_record_rewriter_test.sh b/tests/pypi/whl_extract/wheel_record_rewriter_test.sh index c5880520ad..0076ec0c80 100755 --- a/tests/pypi/whl_extract/wheel_record_rewriter_test.sh +++ b/tests/pypi/whl_extract/wheel_record_rewriter_test.sh @@ -12,11 +12,12 @@ run_rewriter() { out_file="$2" platform_type="$3" data_dir="$4" + shift 4 if command -v cygpath >/dev/null 2>&1; then in_file="$(cygpath -w "$in_file")" out_file="$(cygpath -w "$out_file")" fi - powershell.exe -ExecutionPolicy Bypass -NoProfile -File "$REWRITER" "$in_file" "$out_file" "$platform_type" "$data_dir" + powershell.exe -ExecutionPolicy Bypass -NoProfile -File "$REWRITER" "$in_file" "$out_file" "$platform_type" "$data_dir" "$@" ;; *) "$REWRITER" "$@" @@ -31,7 +32,10 @@ foo-1.0.data/purelib/pkg/module.py,sha256=def,200 foo-1.0.data/platlib/pkg/_ext.so,sha256=ghi,300 foo-1.0.data/data/pkg/data.txt,sha256=111,10 foo-1.0.data/headers/pkg/header.h,sha256=222,20 +foo-1.0.data/scripts/foo,sha256=444,40 foo-1.0.data/scripts/my_script.sh,sha256=333,30 +foo-1.0.data/scripts/plain_tool,sha256=555,50 +foo-1.0.data/scripts/my_py_script.py,sha256=666,60 "foo-1.0.data/purelib/pkg/my file.py",sha256=abc,100 "foo-1.0.data/scripts/my tool",sha256=def,200 "foo-1.0.data/headers/my header.h",sha256=ghi,300 @@ -44,7 +48,7 @@ EOF # Test Unix rewrite UNIX_OUT="$TMP_DIR/unix_RECORD" -run_rewriter "$INPUT" "$UNIX_OUT" "unix" "foo-1.0.data" +run_rewriter "$INPUT" "$UNIX_OUT" "unix" "foo-1.0.data" "foo" "my tool" "my_py_script.py" EXPECTED_UNIX="$TMP_DIR/expected_unix" cat <<'EOF' > "$EXPECTED_UNIX" @@ -53,7 +57,10 @@ pkg/module.py,sha256=def,200 pkg/_ext.so,sha256=ghi,300 ../../../pkg/data.txt,sha256=111,10 ../../../include/pkg/header.h,sha256=222,20 +../../../bin/foo,sha256=444,40 ../../../bin/my_script.sh,sha256=333,30 +../../../bin/plain_tool,sha256=555,50 +../../../bin/my_py_script.py,sha256=666,60 "pkg/my file.py",sha256=abc,100 "../../../bin/my tool",sha256=def,200 "../../../include/my header.h",sha256=ghi,300 @@ -66,9 +73,9 @@ EOF diff -u --strip-trailing-cr "$EXPECTED_UNIX" "$UNIX_OUT" -# Test Windows rewrite +# Test Windows rewrite with shebang-rewritten scripts specified WIN_OUT="$TMP_DIR/win_RECORD" -run_rewriter "$INPUT" "$WIN_OUT" "windows" "foo-1.0.data" +run_rewriter "$INPUT" "$WIN_OUT" "windows" "foo-1.0.data" "foo" "my tool" "my_py_script.py" EXPECTED_WIN="$TMP_DIR/expected_win" cat <<'EOF' > "$EXPECTED_WIN" @@ -77,9 +84,12 @@ pkg/module.py,sha256=def,200 pkg/_ext.so,sha256=ghi,300 ../../pkg/data.txt,sha256=111,10 ../../Include/pkg/header.h,sha256=222,20 +../../Scripts/foo.bat,sha256=444,40 ../../Scripts/my_script.sh,sha256=333,30 +../../Scripts/plain_tool,sha256=555,50 +../../Scripts/my_py_script.py.bat,sha256=666,60 "pkg/my file.py",sha256=abc,100 -"../../Scripts/my tool",sha256=def,200 +"../../Scripts/my tool.bat",sha256=def,200 "../../Include/my header.h",sha256=ghi,300 "../../my data.txt",sha256=jkl,400 foo-1.0.data/custom_dir/custom.txt,sha256=xyz,123 @@ -89,3 +99,30 @@ foo-1.0.dist-info/RECORD,, EOF diff -u --strip-trailing-cr "$EXPECTED_WIN" "$WIN_OUT" + +# Test Windows rewrite with NO rewritten scripts +WIN_OUT_NO_REWRITE="$TMP_DIR/win_no_rewrite_RECORD" +run_rewriter "$INPUT" "$WIN_OUT_NO_REWRITE" "windows" "foo-1.0.data" + +EXPECTED_WIN_NO_REWRITE="$TMP_DIR/expected_win_no_rewrite" +cat <<'EOF' > "$EXPECTED_WIN_NO_REWRITE" +pkg/__init__.py,sha256=abc,100 +pkg/module.py,sha256=def,200 +pkg/_ext.so,sha256=ghi,300 +../../pkg/data.txt,sha256=111,10 +../../Include/pkg/header.h,sha256=222,20 +../../Scripts/foo,sha256=444,40 +../../Scripts/my_script.sh,sha256=333,30 +../../Scripts/plain_tool,sha256=555,50 +../../Scripts/my_py_script.py,sha256=666,60 +"pkg/my file.py",sha256=abc,100 +"../../Scripts/my tool",sha256=def,200 +"../../Include/my header.h",sha256=ghi,300 +"../../my data.txt",sha256=jkl,400 +foo-1.0.data/custom_dir/custom.txt,sha256=xyz,123 +top_level/__init__.py,sha256=aaa,50 +foo-1.0.dist-info/METADATA,sha256=bbb,60 +foo-1.0.dist-info/RECORD,, +EOF + +diff -u --strip-trailing-cr "$EXPECTED_WIN_NO_REWRITE" "$WIN_OUT_NO_REWRITE" diff --git a/tests/pypi/whl_extract/whl_extract_tests.bzl b/tests/pypi/whl_extract/whl_extract_tests.bzl index f7e1e6d2cf..6f3d8e0277 100644 --- a/tests/pypi/whl_extract/whl_extract_tests.bzl +++ b/tests/pypi/whl_extract/whl_extract_tests.bzl @@ -110,6 +110,34 @@ def _test_gen_wheel_record_multiple_srcs_impl(env, target): any(["site-packages/delta-2.0.dist-info/RECORD" in p for p in paths]), ).equals(True) +def _test_gen_wheel_record_rewritten_scripts(name): + rt_util.helper_target( + native.genrule, + name = name + "_src", + outs = [name + "_orig/epsilon-1.0.dist-info/RECORD"], + cmd = "echo 'epsilon-1.0.data/scripts/foo.sh' > $@", + ) + rt_util.helper_target( + gen_wheel_record, + name = name + "_subject", + srcs = [":" + name + "_src"], + rewritten_scripts = ["foo", "my tool"], + ) + analysis_test( + name = name, + target = name + "_subject", + impl = _test_gen_wheel_record_rewritten_scripts_impl, + ) + +_tests.append(_test_gen_wheel_record_rewritten_scripts) + +def _test_gen_wheel_record_rewritten_scripts_impl(env, target): + files = target[DefaultInfo].files.to_list() + env.expect.that_collection(files).has_size(1) + action = env.expect.that_target(target).action_generating(files[0].short_path) + action.argv().contains("foo") + action.argv().contains("my tool") + def whl_extract_test_suite(name): """Create the test suite. diff --git a/tests/repos/whl_with_data1/whl_with_data1-1.0.data/scripts/whl_shell_tool b/tests/repos/whl_with_data1/whl_with_data1-1.0.data/scripts/whl_shell_tool new file mode 100755 index 0000000000..057e62127f --- /dev/null +++ b/tests/repos/whl_with_data1/whl_with_data1-1.0.data/scripts/whl_shell_tool @@ -0,0 +1,2 @@ +#!/bin/sh +echo "hello from whl_shell_tool" diff --git a/tests/repos/whl_with_data1/whl_with_data1-1.0.dist-info/RECORD b/tests/repos/whl_with_data1/whl_with_data1-1.0.dist-info/RECORD index 10307c76a0..c0a6470149 100644 --- a/tests/repos/whl_with_data1/whl_with_data1-1.0.dist-info/RECORD +++ b/tests/repos/whl_with_data1/whl_with_data1-1.0.dist-info/RECORD @@ -1,5 +1,6 @@ whl_with_data1-1.0.data/platlib/whl_with_data1/platlib_file.txt,sha256=123,123 whl_with_data1-1.0.data/scripts/whl_with_data1_script,sha256=123,123 +whl_with_data1-1.0.data/scripts/whl_shell_tool,sha256=123,123 whl_with_data1-1.0.data/scripts/whl_script.sh,sha256=123,123 whl_with_data1-1.0.data/headers/whl_with_data1/header_file.h,sha256=123,123 whl_with_data1-1.0.data/purelib/whl_with_data1/data_file.txt,sha256=123,123 diff --git a/tests/venv_site_packages_libs/BUILD.bazel b/tests/venv_site_packages_libs/BUILD.bazel index 6a7b3b9e12..d70fd6cdf4 100644 --- a/tests/venv_site_packages_libs/BUILD.bazel +++ b/tests/venv_site_packages_libs/BUILD.bazel @@ -1,6 +1,8 @@ load("@rules_shell//shell:sh_test.bzl", "sh_test") load("//python:py_library.bzl", "py_library") load("//tests/support:py_reconfig.bzl", "py_reconfig_test") +load("//tests/support:support.bzl", "SUPPORTS_BZLMOD") +load("//tests/support/pytest_test:pytest_test.bzl", "pytest_test") py_library( name = "user_lib", @@ -86,15 +88,27 @@ py_reconfig_test( ], ) -py_reconfig_test( +pytest_test( name = "importlib_metadata_test", srcs = ["importlib_metadata_test.py"], - bootstrap_impl = select({ - "@platforms//os:windows": "system_python", - "//conditions:default": "script", + config_settings = select({ + "@platforms//os:windows": { + "@rules_python//python/config_settings:bootstrap_impl": "system_python", + "@rules_python//python/config_settings:venvs_site_packages": "yes", + }, + "//conditions:default": { + "@rules_python//python/config_settings:bootstrap_impl": "script", + "@rules_python//python/config_settings:venvs_site_packages": "yes", + }, }), - main = "importlib_metadata_test.py", - venvs_site_packages = "yes", + python_versions = [ + "3.10", + "3.11", + "3.12", + "3.13", + "3.14", + ], + target_compatible_with = SUPPORTS_BZLMOD, deps = [ "@whl_with_data1//:pkg", ], diff --git a/tests/venv_site_packages_libs/importlib_metadata_test.py b/tests/venv_site_packages_libs/importlib_metadata_test.py index eb16a6b6af..93f7964f86 100644 --- a/tests/venv_site_packages_libs/importlib_metadata_test.py +++ b/tests/venv_site_packages_libs/importlib_metadata_test.py @@ -1,91 +1,73 @@ import importlib.metadata import pathlib import sys -import unittest -class ImportlibMetadataTest(unittest.TestCase): - def test_importlib_metadata_files(self): - files = importlib.metadata.files("whl-with-data1") - self.assertIsNotNone(files, "importlib.metadata.files returned None") - self.assertGreater( - len(files), 0, "importlib.metadata.files returned empty list" - ) +def test_importlib_metadata_files(): + files = importlib.metadata.files("whl-with-data1") + assert files is not None, "importlib.metadata.files returned None" + assert len(files) > 0, "importlib.metadata.files returned empty list" - # Verify it contains expected files. - # The RECORD file lists paths relative to the installation root - # (site-packages). - # Per PEP 376 and PEP 427: - # - purelib and platlib files are installed directly under - # site-packages: - # whl_with_data1-1.0.data/purelib/data_overlap.py should be - # installed as data_overlap.py, and - # whl_with_data1-1.0.data/platlib/whl_with_data1/platlib_file.txt - # should be whl_with_data1/platlib_file.txt. - # - scripts, headers, and data files installed outside site-packages - # are recorded relative to site-packages traversing up to the venv - # root (e.g. ../../../bin/ on POSIX, ../../Scripts/ on Windows). - if sys.platform == "win32": - scripts_prefix = "../../Scripts/" - headers_prefix = "../../Include/" - data_prefix = "../../" - else: - scripts_prefix = "../../../bin/" - headers_prefix = "../../../include/" - data_prefix = "../../../" + # Verify it contains expected files. + # The RECORD file lists paths relative to the installation root + # (site-packages). + # Per PEP 376 and PEP 427: + # - purelib and platlib files are installed directly under + # site-packages: + # whl_with_data1-1.0.data/purelib/data_overlap.py should be + # installed as data_overlap.py, and + # whl_with_data1-1.0.data/platlib/whl_with_data1/platlib_file.txt + # should be whl_with_data1/platlib_file.txt. + # - scripts, headers, and data files installed outside site-packages + # are recorded relative to site-packages traversing up to the venv + # root (e.g. ../../../bin/ on POSIX, ../../Scripts/ on Windows). + # - On Windows, venv bin scripts have a .bat extension appended. + if sys.platform == "win32": + scripts_prefix = "../../Scripts/" + headers_prefix = "../../Include/" + data_prefix = "../../" + shebang_script_ext = ".bat" + else: + scripts_prefix = "../../../bin/" + headers_prefix = "../../../include/" + data_prefix = "../../../" + shebang_script_ext = "" - expected_paths = sorted( - [ - scripts_prefix + "data_overlap.sh", - data_prefix + "bin/data_overlap.sh", - scripts_prefix + "overlap/both.sh", - scripts_prefix + "overlap/script1.sh", - scripts_prefix + "whl_script.sh", - scripts_prefix + "whl_with_data1_script", - headers_prefix + "data_overlap.h", - data_prefix + "include/data_overlap.h", - headers_prefix + "overlap/both.h", - headers_prefix + "overlap/header1.h", - headers_prefix + "whl_with_data1/header_file.h", - data_prefix + "overlap/both.txt", - data_prefix + "overlap/data1.txt", - data_prefix + "site-packages/data_overlap.py", - data_prefix + "whl_with_data1/data_data_file.txt", - data_prefix + "whl_with_data1/data_data_file.txt", - "data_overlap.py", - "whl_with_data1/data_file.txt", - "whl_with_data1/platlib_file.txt", - ] - ) - file_paths = sorted(str(f).replace("\\", "/") for f in files) - self.assertEqual(file_paths, expected_paths) + expected_paths = sorted( + [ + scripts_prefix + "data_overlap.sh", + data_prefix + "bin/data_overlap.sh", + scripts_prefix + "overlap/both.sh", + scripts_prefix + "overlap/script1.sh", + scripts_prefix + "whl_script.sh", + scripts_prefix + "whl_shell_tool", + scripts_prefix + "whl_with_data1_script" + shebang_script_ext, + headers_prefix + "data_overlap.h", + data_prefix + "include/data_overlap.h", + headers_prefix + "overlap/both.h", + headers_prefix + "overlap/header1.h", + headers_prefix + "whl_with_data1/header_file.h", + data_prefix + "overlap/both.txt", + data_prefix + "overlap/data1.txt", + data_prefix + "site-packages/data_overlap.py", + data_prefix + "whl_with_data1/data_data_file.txt", + data_prefix + "whl_with_data1/data_data_file.txt", + "data_overlap.py", + "whl_with_data1/data_file.txt", + "whl_with_data1/platlib_file.txt", + ] + ) + file_paths = sorted(str(f).replace("\\", "/") for f in files) + assert file_paths == expected_paths - for f in files: - resolved = pathlib.Path(f.locate()) - if resolved.exists(): - self.assertTrue( - resolved.is_file(), - f"Expected {resolved} to be a regular file", - ) + for f in files: + resolved = pathlib.Path(f.locate()) + assert resolved.exists(), f"Expected file {f} (resolved to {resolved}) to exist" + assert resolved.is_file(), f"Expected {resolved} to be a regular file" - # Verify file content can be read both as binary and as text - content = f.read_binary() - self.assertIsNotNone(content) + # Verify file content can be read both as binary and as text + content = f.read_binary() + assert content is not None - text = f.read_text(encoding="utf-8") - self.assertIsNotNone(text) - else: - # On Windows, venv bin scripts have a .bat extension appended. - bat_resolved = resolved.parent / (resolved.name + ".bat") - self.assertTrue( - bat_resolved.exists(), - f"Expected file {f} (resolved to {resolved} or {bat_resolved}) to exist", - ) - self.assertTrue( - bat_resolved.is_file(), - f"Expected {bat_resolved} to be a regular file", - ) - - -if __name__ == "__main__": - unittest.main() + text = f.read_text(encoding="utf-8") + assert text is not None