Skip to content

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Sep 20, 2025

Summary:

  • refactor: use rules_shell runfiles lib
  • refactor: remove watch helpers
  • refactor: remove usage of select helper
  • refactor: make enable_pystar fixed and cleanup code
  • refactor: remove migration tag helper
  • refactor: remove is_bazel_6_or_higher
  • refactor: remove is_bazel_6_4_or_higher
  • refactor: remove is_bazel_7_or_greater
  • remove: is_bazel_7_4_or_greater
  • fix: pipstar env var is now respected
  • chore: drop bazel 5 support code
  • chore: add an override for bzlmod example
  • chore: remove version specific globs, since the supported versions
    support spaces in filenames and the file becomes redundant.

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Wow. We had quite a bit of compatibility code for Bazel 6. Thanks for cleaning house!

# Because this file isn't bundled with Bazel, so we have to conditionally
# check for this flag.
# TODO: Remove this once all supported Balze versions have this flag.
# TODO: Remove this once all supported Blaze versions have this flag.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed.

@aignas aignas marked this pull request as ready for review September 21, 2025 12:42
@aignas aignas requested a review from groodt as a code owner September 21, 2025 12:42
@aignas aignas force-pushed the aignas.chore.drop-bazel-6 branch from 48af9b2 to f2e3791 Compare September 21, 2025 13:02
@aignas
Copy link
Collaborator Author

aignas commented Sep 22, 2025

So it is failing due to runfiles missing from the integration test. I remember seeing this failure, but I don't know how to solve it know.

And the rc is soft failing, I need to check what is going on.

@aignas aignas force-pushed the aignas.chore.drop-bazel-6 branch from 37a384f to 33f7c91 Compare September 27, 2025 13:02
@aignas
Copy link
Collaborator Author

aignas commented Sep 27, 2025

OK, did the scrub again, now much more methodically and incrementally, lets see how the CI behaves now. Locally the failing tests are passing.

@aignas aignas requested a review from rickeylev September 28, 2025 00:15
@aignas
Copy link
Collaborator Author

aignas commented Sep 28, 2025

PTAL, did more cleanup and updated the PR description.


# buildifier: disable=native-python
_py_binary_impl = _starlark_py_binary if config.enable_pystar else native.py_binary
_py_binary_impl = _starlark_py_binary
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind the loaded py_binary symbol to _py_binary_impl

config.enable_pystar or config.BuiltinPyCcLinkParamsProvider == None
) else config.BuiltinPyCcLinkParamsProvider
)
PyCcLinkParamsInfo = _starlark_PyCcLinkParamsInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind loaded symbol name

load("//python/private:reexports.bzl", "BuiltinPyInfo")

PyInfo = _starlark_PyInfo if config.enable_pystar or BuiltinPyInfo == None else BuiltinPyInfo
PyInfo = _starlark_PyInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind loaded symbol name


# buildifier: disable=native-python
_py_library_impl = _starlark_py_library if config.enable_pystar else native.py_library
_py_library_impl = _starlark_py_library
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind loaded symbol name


# buildifier: disable=native-python
_py_runtime_impl = _starlark_py_runtime if IS_BAZEL_6_OR_HIGHER else native.py_runtime
_py_runtime_impl = _starlark_py_runtime
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind loaded symbol name

load("//python/private:reexports.bzl", "BuiltinPyRuntimeInfo")

PyRuntimeInfo = _starlark_PyRuntimeInfo if config.enable_pystar else BuiltinPyRuntimeInfo
PyRuntimeInfo = _starlark_PyRuntimeInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind loaded symbol name

load("//python/private:util.bzl", "IS_BAZEL_6_OR_HIGHER")

_py_runtime_pair = _starlark_impl if IS_BAZEL_6_OR_HIGHER else _bazel_tools_impl
_py_runtime_pair = _starlark_impl
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind loaded symbol name


# buildifier: disable=native-python
_py_test_impl = _starlark_py_test if config.enable_pystar else native.py_test
_py_test_impl = _starlark_py_test
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can rebind loaded symbol name

@rickeylev
Copy link
Collaborator

Just some nits that can be addressed later.

Thanks for this! Feels good to see all that pystar compatibility code die ☠️ 🫗

@rickeylev rickeylev added this pull request to the merge queue Sep 28, 2025
Merged via the queue into main with commit 726ffa2 Sep 28, 2025
8 checks passed
@rickeylev rickeylev deleted the aignas.chore.drop-bazel-6 branch September 28, 2025 01:32
github-merge-queue bot pushed a commit that referenced this pull request Sep 28, 2025
A followup to #3282 to finish up the cleanup and remove the unnecessary
`starlark` usage in naming.
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