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

Allow custom stub template for py_binary's #137

Closed
kwatts opened this issue Apr 17, 2015 · 21 comments
Closed

Allow custom stub template for py_binary's #137

kwatts opened this issue Apr 17, 2015 · 21 comments
Assignees
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-Python Native rules for Python type: feature request

Comments

@kwatts
Copy link
Contributor

kwatts commented Apr 17, 2015

Feature request: My team would like to use a custom stub template for our py_binary executables (setting up a customization points, etc). Is there any way to do this with Bazel, or should we write a "custom_py_binary" rule?

@damienmg
Copy link
Contributor

@laszlocsomor
Copy link
Contributor

My two cents: in this case I think updating the existing py_binary rules is better than implementing a custom one. The feature sounds reasonable and could be useful to others too.

@laszlocsomor laszlocsomor added type: feature request under investigation P2 We'll consider working on this in future. (Assignee optional) labels Apr 21, 2015
@laurentlb laurentlb removed their assignment May 13, 2015
@ulfjack ulfjack added this to the 1.0 milestone Jun 15, 2016
@ulfjack ulfjack added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jun 15, 2016
@benley
Copy link
Contributor

benley commented Jul 18, 2017

My team could use this as well - we have been wanting to tweak the python stub to exclude local system site-packages by adding -S to the python commandline args. Unless there's a better way of accomplishing that...?

@fahhem
Copy link
Contributor

fahhem commented Nov 8, 2018

I took @damienmg's suggestions and added a stub_template attribute and a test. Hopefully that solves your (and my) problem!

@brandjon
Copy link
Member

brandjon commented Nov 8, 2018

Thanks a lot for the PR Fahrzin!

It doesn't surprise me that people would want to customize the stub script, but I do wonder what all the use cases are and whether they can be achieved with a more narrow parameterization. For instance, we could add a place to insert custom PYTHONPATHs and custom interpreter flags.

I have two main concerns with this proposal. First, it seems to me that stub customization should be per-interpreter, rather than per-binary. For example, if the same binary is built on different developer machines that have different system interpreters (which is possible in the future via a py_toolchain rule), the stub scripts might need to differ. (Since currently all py_binarys in a build use --python_top as their interpreter, this means they'd all share the same stub, but that restriction will go away in the future.)

This could be implemented by adding the attribute to py_runtime, adding an Artifact field to BazelPyRuntimeProvider, and consuming that in BazelPythonSemantics#createExecutable.

My second concern is that if we expose the template, it becomes an API that we have to worry about breaking. In particular, we need to worry about the availability and meaning of the substitution placeholders. One way around this might be to guard the feature with an experimental flag.

In any case, I think this raises enough questions that I should put the above points into a design doc.

@laszlocsomor
Copy link
Contributor

Please consider also that py_* rules on Windows do not use a stub script but a pre-built binary preamble. I suggest pointing out this difference in documentation, i.e. customizing the stub script would have no effect on Windows.

@brandjon
Copy link
Member

brandjon commented Nov 9, 2018

@kwatts, @benley, @fahhem: What are the exact kinds of customization people want for the stub template? It's hard for me to imagine that users should be able to manipulate the runfiles logic for instance. I understand -S for excluding site packages, but that can also be done by adding a one-off boolean attribute, or a list of strings for additional flags to pass to the interpreter before the script name.

@brandjon
Copy link
Member

brandjon commented Nov 9, 2018

@laszlocsomor: Does that mean the windows-specific logic in python_stub_template.txt is dead code and can be removed?

@laszlocsomor
Copy link
Contributor

Sorry, forget almost everything I said, I got confused.
The Python stub is used on Windows, it's not dead code, so customizing it would also have an effect on Windows. It's also true that we use a launcher, which ultimately executes this stub script. Calling @meteorcloudy to verify what I said.

I got confused over the fact that for Java we also have a stub script, which is a Bash script, and its logic is fully implemented in the launcher. (We use the same launcher preamble for Python and Java binaries, but with different launch parameters, so one starts python, the other starts the JVM.)

@brandjon
Copy link
Member

brandjon commented Nov 9, 2018

Thanks for the clarification. So if we go ahead with this, I won't distinguish the case of windows vs unix.

@brandjon
Copy link
Member

brandjon commented Nov 9, 2018

Please see design doc posted here, and in particular the open questions.

@meteorcloudy
Copy link
Member

Yes, I can confirm what @laszlocsomor said, we do use the stub template on Windows. The native python launcher is just used to run the stub template.

@fahhem
Copy link
Contributor

fahhem commented Nov 13, 2018

My current use case is that I'd like to insert code that runs in the stub for a certain subset of rules, such as enabling (and extracting) coverage instrumentation for py_test() rules (the stub_template attribute is available in py_test() rules too).

Other use cases in the past that would have been easier with this:

  • subpar parses the current post-substitution stub for a py_binary() rule to extract the data it needs. If it was a custom stub_template, then subpar could have created a custom stub_template that exposed the python imports as needed.
  • I also needed -S-equivalent behavior in the past, I ended up with re-doing what the stub does (execute Python with new args) at the beginning of a Python binary's lifetime. So now the stub template runs some Python, then executes Python, then we run some Python and then exec Python again. Not great. I actually needed -B as well as a mechanism to control LD_LIBRARY_PATH before Python is executed. All things that should be done in the stub, but since I don't have control over that, I do it in the equivalent to google's import google3 setup.

@brown
Copy link
Contributor

brown commented Nov 14, 2018

I'm aware of two common use cases that require changes to the
Python stub.

  1. Developers want an executable that has the PYTHON_PATH of a
    given py_binary or py_library but behaves just like the standard
    Python interpreter. This executable would be used for
    experimenting in the REPL or for deploying an interpreter with
    pre-loaded libraries.

  2. Augmenting LD_LIBRARY_PATH.

@fahhem
Copy link
Contributor

fahhem commented Dec 26, 2019

Any update here @brandjon? I can update my PR again if we're okay allowing custom stubs on the py_binary rules itself. Otherwise, we will have to continue using a fork of bazel with the custom stub enabled.

If you want to pull the stub from the toolchain, then I'm happy to switch to that tactic if I have a chance of actually getting it merged in.

@ulfjack
Copy link
Contributor

ulfjack commented Jan 27, 2020

I merged b01c859, with which I was able to get coverage from python using a patched pycoverage. I filed #10660 as a standalone issue about coverage, and also provided instructions for how to make it work with a Bazel binary containing said patch.

@meisterT meisterT removed this from the 1.0 milestone May 12, 2020
@lberki lberki added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Nov 18, 2020
fahhem added a commit to AppliedIntuition/bazel that referenced this issue 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 added a commit to AppliedIntuition/bazel that referenced this issue 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 issue 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 issue 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
Copy link
Contributor

fahhem commented Dec 1, 2022

FYI, I've put out a new PR with changes to the runtime (that I think is close to getting in, but @rickeylev decides that): #16806

Once that's in, a new attribute bootstrap_template will be available in py_runtime() that will be used instead of the current python_stub_template.txt.

@arrdem
Copy link

arrdem commented Dec 14, 2022

Is there something I can do to help push #16806 forwards? Looks like there's an outstanding review but it's pretty close

@fahhem
Copy link
Contributor

fahhem commented Dec 21, 2022

Not other than making the changes necessary :D I'm coming back to this now so I hope it's ready this week

fahhem added a commit to AppliedIntuition/bazel that referenced this issue 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
@brandjon
Copy link
Member

I'm not working on Python rules anymore. Ccing @rickeylev for any decisions regarding prioritization of this.

@rickeylev rickeylev self-assigned this Jan 4, 2023
@rickeylev
Copy link
Contributor

That PR is approved and just needs to be imported. I got back from vacation today; it's one of changes I'll be looking at first.

fahhem added a commit to AppliedIntuition/bazel that referenced this issue 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 issue 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 issue 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 issue 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
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-Python Native rules for Python type: feature request
Projects
None yet