Skip to content

Conversation

@arrdem
Copy link
Collaborator

@arrdem arrdem commented Nov 5, 2025

Vendor rules_python's determine_main and remove the need for a separate determine_main target which complicates exec properties.

Replaces #641 with thanks to Keith.
Fixes #605
Fixes #621

Note that py_venv_* already mandates that a main= label be provided, so this is for legacy py_binary only. Should be removed in 2.0.

Changes are visible to end-users: no

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: no

Test plan

  • Covered by existing test cases

@arrdem arrdem requested review from alexeagle and dzbarsky November 5, 2025 00:36
@arrdem arrdem added the breaking Requires a semver-major release due to breaking public API changes label Nov 5, 2025
@arrdem arrdem force-pushed the arrdem/remove-determine-main branch from 59f9875 to bf5852e Compare November 5, 2025 00:51
@aspect-workflows
Copy link

aspect-workflows bot commented Nov 5, 2025

test-os:linux-bzl:8 (Test)

All tests were cache hits

38 tests (100.0%) were fully cached saving 51s.


test-os:linux-bzl:latest (Test)

All tests were cache hits

38 tests (100.0%) were fully cached saving 51s.


test-os:linux-bzl:8 (Test)

e2e

All tests were cache hits

14 tests (100.0%) were fully cached saving 10s.


test-os:linux-bzl:latest (Test)

e2e

All tests were cache hits

14 tests (100.0%) were fully cached saving 10s.


test-os:linux-bzl:8 (Test)

examples/uv_pip_compile

All tests were cache hits

1 test (100.0%) was fully cached saving 335ms.


test-os:linux-bzl:latest (Test)

examples/uv_pip_compile

All tests were cache hits

1 test (100.0%) was fully cached saving 335ms.

@alexeagle
Copy link
Member

note that this is a subtle compatibility break with rules_python, at least last time I dug into this code, rules_python users couldn't adopt rules_py without changing their code.

Perhaps we can make the case that we no longer aim for drop-in compatibility with rules_python, but if we give that up we ought to be clear about it in the readme or other communication with users.

@dzbarsky
Copy link
Collaborator

dzbarsky commented Nov 5, 2025

note that this is a subtle compatibility break with rules_python, at least last time I dug into this code, rules_python users couldn't adopt rules_py without changing their code.

Perhaps we can make the case that we no longer aim for drop-in compatibility with rules_python, but if we give that up we ought to be clear about it in the readme or other communication with users.

This is true; rules_python has already swapped determine_main to be a function instead of a standalone target, so let's at least do that if we don't want to make the bigger change. (Though I do think the new semantics are clearer)

@arrdem arrdem removed the breaking Requires a semver-major release due to breaking public API changes label Nov 7, 2025
@arrdem
Copy link
Collaborator Author

arrdem commented Nov 7, 2025

Gonna just redo this by vendoring the determine_main code and keeping compat for now.

@arrdem arrdem force-pushed the arrdem/remove-determine-main branch from 6342788 to edd42e1 Compare November 7, 2025 21:12
@arrdem arrdem merged commit 87d3dc3 into main Nov 7, 2025
2 checks passed
@arrdem arrdem deleted the arrdem/remove-determine-main branch November 7, 2025 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants