Skip to content

chore: make pipstar non-switchable#3737

Draft
aignas wants to merge 10 commits intobazel-contrib:mainfrom
aignas:aignas.chore.cleanup-legacy-pip-code
Draft

chore: make pipstar non-switchable#3737
aignas wants to merge 10 commits intobazel-contrib:mainfrom
aignas:aignas.chore.cleanup-legacy-pip-code

Conversation

@aignas
Copy link
Copy Markdown
Collaborator

@aignas aignas commented Apr 24, 2026

The pipstar code has been there for some time and might be stable enough to not
have a fallback anymore.

Related #2949

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the RULES_PYTHON_ENABLE_PIPSTAR and RULES_PYTHON_ENABLE_PYSTAR environment variables and their associated logic, effectively making the Starlark implementations the default. It also removes the legacy entry point generation mechanism in favor of py_console_script_binary. Feedback was provided regarding a potential breaking change in the whl_library_targets macro signature, where retaining the entry_points parameter is recommended for backward compatibility with vendored files. Additionally, a null check was suggested for the whl_metadata result in whl_library.bzl to prevent potential crashes if the metadata file is missing.

Comment on lines 119 to 124
data = [],
copy_files = {},
copy_executables = {},
entry_points = {},
native = native,
enable_implicit_namespace_pkgs = False,
namespace_package_files = [],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Removing the entry_points argument from the whl_library_targets macro is a breaking change for users who have vendored requirements.bzl files or are using custom BUILD file templates that still pass this argument. To maintain backward compatibility and avoid a TypeError in Starlark, it is recommended to keep the argument in the signature but ignore its value.

        data = [],
        copy_files = {},
        copy_executables = {},
        entry_points = {},
        native = native,
        enable_implicit_namespace_pkgs = False,
        namespace_package_files = [],

Comment on lines +430 to +434
metadata = whl_metadata(
install_dir = install_dir_path,
read_fn = rctx.read,
logger = logger,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The whl_metadata function can return None if it fails to find or parse the METADATA file (as seen in whl_metadata.bzl). If it returns None, the subsequent access to metadata.name at line 444 will cause a crash with a potentially confusing error message. Although logger.fail is called inside whl_metadata, adding an explicit check here prevents a secondary crash and provides a cleaner exit from the repository rule.

Suggested change
metadata = whl_metadata(
install_dir = install_dir_path,
read_fn = rctx.read,
logger = logger,
)
metadata = whl_metadata(
install_dir = install_dir_path,
read_fn = rctx.read,
logger = logger,
)
if not metadata:
return None

At the same time cleanup pystar env var docs. Also cleanup dead code.
@aignas aignas force-pushed the aignas.chore.cleanup-legacy-pip-code branch from 116019e to 4f78a8c Compare April 24, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants