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 #6632

Closed
wants to merge 3 commits into from

Conversation

fahhem
Copy link
Contributor

@fahhem fahhem commented Nov 8, 2018

Fixes #137

Wasn't too hard after all!

One note: the fact that the TemplateExpansionAction constructors switch the order of the template and the output depending on the type of the template (an Artifact vs a Template) took the longest amount of time in debugging for this PR.

Lastly, I'm aware I didn't document the attribute very well. I'm open to suggestions!

@jin jin added the team-Rules-Python Native rules for Python label Nov 8, 2018
@brandjon
Copy link
Member

brandjon commented Nov 8, 2018

Thanks for the PR! Please see my comments in #137. I'd consider this PR blocked pending a design doc. I'll see if I can write that doc tomorrow.

@laurentlb
Copy link
Contributor

What's the current state, @brandjon?

@aiuto
Copy link
Contributor

aiuto commented Apr 23, 2020

ping everyone.

  • tests need to be fixed
  • to we still want this or is it obsolete?

@meisterT
Copy link
Member

ping @brandjon

@danmx
Copy link

danmx commented Oct 14, 2020

Any updates on this PR?

@meisterT meisterT added the P1 I'll work on this now. (Assignee required) label Oct 21, 2020
@hrfuller
Copy link

I'm running into some issues debugging an os error in the stub template and having a custom script would be useful.

@sgowroji
Copy link
Member

Hello @fahhem, The above PR has been stale from long time. Could you please check buildkite failures and address the above comments. Thanks

@sgowroji sgowroji added the awaiting-user-response Awaiting a response from the author label Apr 26, 2022
@fahhem
Copy link
Contributor Author

fahhem commented Apr 26, 2022

I have this working on the bazel 4.0.0 tag internally. However, I don't want to spend time on this PR until I have a signal that someone from the bazel team will actually consider it. Last I checked, the issue was 'under design review' a couple years ago and no updates since. It's marked a P4 (though this PR is a P1??).

If there's an actual chance of getting this in I'd be happy to update the branch and get it in

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Apr 27, 2022
@sventiffe
Copy link
Contributor

@brandjon can you have a look/comment, please?

@meteorcloudy
Copy link
Member

@rickeylev is currently working on Python rules, he can probably take a look ;)

@fahhem
Copy link
Contributor Author

fahhem commented Nov 8, 2022

I'm actively working on the CI failures, will post again when it's ready

@fahhem
Copy link
Contributor Author

fahhem commented Nov 8, 2022

Unfortunately I can't seem to figure out why @bazel_tools//tools/python:python_stub_template.txt doesn't exist in CI when it's there, checked in, and mentioned in BUILD.tools like the other files. Anyone have an idea on what I'm missing in adding a file to @bazel_tools?

@rickeylev
Copy link
Contributor

I somewhat echo Jon's thinking in #137 from back in Nov 2018.

This is more appropriate as an attribute of py_runtime rather than py_binary. Most of what the stub template does is find the interpreter, runfiles, and setup the runtime environment for a binary. The majority of this doesn't vary on a per-binary bases. It does vary on a per-platform basis, though, which is better handled by the toolchain lookup process. A good example of this is windows vs mac vs linux differences; e.g. windows might use a batch file (or powershell or etc), while mac might use an old version of bash, and linux uses a newer version. There's other stuff, too, like coverage setup and execution. Handling those sort of differences on a per-binary basis is a hassle.

I would accept adding an optional {something}_template[1] attribute to py_runtime, which is treated as a file. If py_binary finds this, it uses it as it would the builtin template. The API for this (i.e the set of keywords substituted and their values) is considered experimental and unstable (at least until the py rule implementation is moved out of Bazel itself -- then we can revisit).

[1] not "stub_template"; the startup code is fairly complicated and does real things; "executable_template", "binary_template", etc are all more descriptive.

If you want to experiment with something to allow a greater degree of customization, then we can try adding a callback somewhere on the py_runtime_pair or PyRuntimeInfo values to give the toolchain greater control over how the executable is created (basically allowing the toolchain to directly do what PythonSemantics.createExecutable is able to do).

All that said, there's definitely still some binary-level customizations to propagate into the startup process. e.g., a binary might want to have certain interpreter flags set at startup that are difficult/impossible to customize post-interpreter startup (-b, -u, and other flags come to mind). So I'm +1 on exposing some attribute to pass along that information.

@fahhem
Copy link
Contributor Author

fahhem commented Nov 13, 2022

@rickeylev Thank you for the comment! py_runtime came out years after I opened this PR, and now that it exists I'd also love to move this there.

However, seeing as I can't make progress due to the @bazel_tools files not being exposed/updated the way they used to be, I can't make any useful changes here. I'm currently leaning to close this PR out, if anyone is interested in fixing this PR or getting a py_runtime version over the line, please do so. In the meantime, we'll keep updating our fork every few months, as we have for 4 years

fahhem added a commit to AppliedIntuition/bazel that referenced this pull request Nov 19, 2022
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
@fahhem
Copy link
Contributor Author

fahhem commented Nov 22, 2022

Somehow I got it to work in #16806, so going to close this in favor of that PR getting in.

@fahhem fahhem closed this Nov 22, 2022
@fahhem fahhem deleted the custom_stub branch November 22, 2022 07:52
fahhem added a commit to AppliedIntuition/bazel that referenced this pull request Nov 23, 2022
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
fahhem added a commit to AppliedIntuition/bazel that referenced this pull request Nov 23, 2022
Fixes bazelbuild#137, but unlike my approach in bazelbuild#6632, this adds an attribute to
`py_runtime` rather than to `py_binary`
fahhem added a commit to AppliedIntuition/bazel that referenced this pull request Nov 30, 2022
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
fahhem added a commit to AppliedIntuition/bazel that referenced this pull request Dec 21, 2022
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
copybara-service bot pushed a commit that referenced this pull request Jan 7, 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
fahhem added a commit to AppliedIntuition/bazel that referenced this pull request Jan 20, 2023
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
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
jack-zhang-ai pushed a commit to AppliedIntuition/bazel that referenced this pull request Jan 31, 2024
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
jamison-lahman-ai pushed a commit to AppliedIntuition/bazel that referenced this pull request May 22, 2024
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

Patched cc_shared_library to fix cc shared library transitive dependencies taking in upb protos

Edit bootstrap template.

Windows related updates.

Use our stub template directly.

Remove host version warning emission.

Update builtins_bzl with better support for cc_shared_library's

Transitive debug and run files.

Repair transitive debug files.

Some more fixes.

Update python_bootstrap_template.txt

Fix error prone ComparisonOutOfRange in PersistentStringIndexer

PiperOrigin-RevId: 544294153
Change-Id: Ia89d02a025dfafa213d68ea2110a79e5adccf79c

fix changed var name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer cla: yes P1 I'll work on this now. (Assignee required) 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