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

Enable custom py_binary stub_template attribute (via py_runtime) #16806

Closed

Conversation

fahhem
Copy link
Contributor

@fahhem fahhem commented Nov 19, 2022

Fixes #137, but unlike my approach in #6632, this adds an attribute to py_runtime rather than to py_binary

Open to suggestions on the attribute name and documentation

@fahhem fahhem marked this pull request as draft November 19, 2022 22:58
@lberki lberki requested review from rickeylev and removed request for lberki November 21, 2022 08:07
allow_single_file = True,
default = DEFAULT_STUB_SCRIPT_TEMPLATE,
doc = """
The stub script template file to use. Should have %python_binary%,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like for the variables to be more formally documented, but I think this is OK for now.

Can you please add 3 pieces of doc, though?

This template, after expansion, becomes the executable file used to start the process, so it is responsible for initial bootstrapping actions such as
finding the Python interpreter, runfiles, and constructing an environment
to run the intended Python application.

And also:

While this attribute is currently optional, it will become required when the Python rules are moved out of Bazel itself.

And also:

The exact variable names expanded is an unstable API and is subject to change. The API will become more stable when the Python rules are moved out of Bazel itself.

(also, you can omit such doc in the java impl; I'll be deleting it in a couple weeks. I've just been waiting for the Starlark version to be released within Google to serve as a spot-check before deleting the Java impl)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would the attribute become required? Is the plan to remove the bootstrap template that I'm moving to bazel_tools?

Added the comments verbatim for now

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to remove the bootstrap template that I'm moving to bazel_tools?

Yep! The plan is to remove all this py rule stuff from Bazel itself and move it into rules_python instead. The pieces to start this are almost all in place.

@fahhem fahhem marked this pull request as ready for review November 22, 2022 07:50
@fahhem fahhem requested review from rickeylev and removed request for comius November 22, 2022 07:51
@sgowroji sgowroji added team-Rules-Python Native rules for Python awaiting-review PR is awaiting review from an assigned reviewer labels Nov 22, 2022
src/main/starlark/builtins_bzl/common/python/providers.bzl Outdated Show resolved Hide resolved
@@ -39,6 +39,7 @@ public void setup(MockToolsConfig config) throws IOException {
addTool(config, "tools/python/toolchain.bzl");
addTool(config, "tools/python/utils.bzl");
addTool(config, "tools/python/private/defs.bzl");
addTool(config, "tools/python/python_bootstrap_template.txt");
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self: this will probably need to also be added to the internal MockPythonSupport version of this class when importing

@fahhem
Copy link
Contributor Author

fahhem commented Nov 22, 2022

Thanks Richard! I replied to the questions, and I'll follow up on the actionable comments today

@fahhem
Copy link
Contributor Author

fahhem commented Nov 23, 2022

Done with the review, thank you @rickeylev !

@fahhem
Copy link
Contributor Author

fahhem commented Nov 30, 2022

Seems like the main change needed was just to switch to ctx.file and then fix the two places I had workarounds to "support" ctx.attr being not a FileApi? I've done the above and it looks like the tests are passing, are we good to merge?

src/main/starlark/builtins_bzl/common/python/providers.bzl Outdated Show resolved Hide resolved
Comment on lines 220 to 222
allowedTypes = {
@ParamType(type = FileApi.class),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this permit None being passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added NoneType since I think we should allow None to be passed in

@@ -46,6 +47,7 @@
public interface PyRuntimeInfoApi<FileT extends FileApi> extends StarlarkValue {

static final String DEFAULT_STUB_SHEBANG = "#!/usr/bin/env python3";
static final String DEFAULT_BOOTSTRAP_TEMPLATE = "//tools/python:python_bootstrap_template.txt";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have "@bazel_tools"? Or is that implicit in this context for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was, but it's not used here anymore so deleted it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually added it back, it's used (but via the subclass PyRuntimeInfo, that's why I didn't find it via rg). I added a comment that it should be used with getToolsLabel() to use it correctly

@rickeylev
Copy link
Contributor

@fahhem just bumping this in case it got lost

@fahhem
Copy link
Contributor Author

fahhem commented Dec 21, 2022

Sorry, needed to get something else done for work, but coming back to this now.

Fixes bazelbuild#137, but unlike my approach in bazelbuild#6632, this adds an attribute to
`py_runtime` rather than to `py_binary`

Open to suggestions on the attribute name and documentation
@rickeylev
Copy link
Contributor

LGTM.

@rickeylev rickeylev added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 23, 2022
@fahhem
Copy link
Contributor Author

fahhem commented Jan 6, 2023

Thank you @rickeylev! I assume this will be merged by copybara or something in the coming weeks? Do I need to do anything to get it into the next 6.x release?

@rickeylev
Copy link
Contributor

Heyas,

Just got back from vacation the other day. The Bazel-pr-importer-team (that's who the "awaiting-pr-merge" label notifies) starting importing it, but some failures popped up so I had to fix those first. I'm now re-importing it and it's looking good; I expect to submit it tomorrow or Monday, and it'll show up on github automatically shortly thereafter.

copybara-service bot pushed a commit that referenced this pull request Jan 7, 2023
This is just a small quality of life improvement.

This is originally from PR #16806; it was split off to make submitting the
main change of that PR easier.

PiperOrigin-RevId: 500222350
Change-Id: I274f527340086a131d81f11a123bde240c87d34c
@copybara-service copybara-service bot closed this in 0696ba3 Jan 7, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 9, 2023
@fahhem fahhem deleted the custom_stub_runtime branch January 21, 2023 01:59
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
This is just a small quality of life improvement.

This is originally from PR #16806; it was split off to make submitting the
main change of that PR easier.

PiperOrigin-RevId: 500222350
Change-Id: I274f527340086a131d81f11a123bde240c87d34c
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
Fixes #137, but unlike my approach in #6632, this adds an attribute to `py_runtime` rather than to `py_binary`

Closes #16806.

PiperOrigin-RevId: 500248760
Change-Id: Ic39b0e9f8ae8063c3dedd1f67feece2e69de6306
copybara-service bot pushed a commit that referenced this pull request Apr 18, 2023
…y_runtime on bazel latest

This PR fixes a small bug where the bootstrap template runtime file was not being passed to Py Binary. This broke the bootstrap template feature on bazel latest authored in #16806

Closes #18103.

PiperOrigin-RevId: 525042380
Change-Id: I8b8a200634eb98e156b4d8ba6b2e5baef5d06c73
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
…y_runtime on bazel latest

This PR fixes a small bug where the bootstrap template runtime file was not being passed to Py Binary. This broke the bootstrap template feature on bazel latest authored in bazelbuild#16806

Closes bazelbuild#18103.

PiperOrigin-RevId: 525042380
Change-Id: I8b8a200634eb98e156b4d8ba6b2e5baef5d06c73
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-Python Native rules for Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow custom stub template for py_binary's
3 participants