Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(py_runtime): Allow py_runtime to take an executable target as the interpreter #1621

Merged

Conversation

mishazharov
Copy link
Contributor

@mishazharov mishazharov commented Dec 16, 2023

This PR allows py_runtime to accept an executable (e.g. sh_binary).

This makes it easier to customize the interpreter binary used, as it allows
intercepting invocation of the interpreter. For example, it can be used to
change how the interpreter searches for dynamic libraries.

Related to #1612

python/private/common/py_runtime_rule.bzl Outdated Show resolved Hide resolved
python/private/common/py_runtime_rule.bzl Outdated Show resolved Hide resolved
python/private/common/py_runtime_rule.bzl Outdated Show resolved Hide resolved
python/private/common/py_runtime_rule.bzl Outdated Show resolved Hide resolved
python/private/common/py_runtime_rule.bzl Show resolved Hide resolved
@mishazharov
Copy link
Contributor Author

Thanks for the review, I've also added a bare bones unit test

@mishazharov mishazharov force-pushed the py-runtime-allow-executable-targets branch from d593067 to c60b21f Compare December 24, 2023 18:38
@mishazharov mishazharov force-pushed the py-runtime-allow-executable-targets branch from 7cf394f to c60b21f Compare December 31, 2023 02:24
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.

This looks pretty good; almost there. Most is just cosmetic changes. Only real potential bug is the order of checks for the single-file vs executable cases.

I got a bit of time now, so I think I can resolve most of the requested changes and push them into this PR.

python/private/common/py_runtime_rule.bzl Outdated Show resolved Hide resolved
tests/py_runtime/pretend_binary Outdated Show resolved Hide resolved
tests/py_runtime/py_runtime_tests.bzl Outdated Show resolved Hide resolved
tests/py_runtime/py_runtime_tests.bzl Outdated Show resolved Hide resolved
* Wrap changelog text
* Make comment about allow_files more descriptive
* Improve doc about interpreter attribute behavior
* use helper_target()
* add tests for binaries with multiple and single outputs
* switch to custom rule instead of using sh_binary
* fix bug where single-output binary rules wouldn't have runfiles
  merged in.
* puts the tests in a more alphabetical location
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.

I think this is in good shape and can be merged.

The only thing I can think to add is an integration test to verify that using an sh_binary as described actually works, but that can be done in a separate PR if necessary.

@rickeylev rickeylev enabled auto-merge January 8, 2024 01:48
@rickeylev rickeylev added this pull request to the merge queue Jan 8, 2024
Merged via the queue into bazelbuild:main with commit 69edec8 Jan 8, 2024
4 checks passed
@mishazharov
Copy link
Contributor Author

Awesome, thank you for getting this over the line!

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