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

Cannot use py_runtime with the output of a sh_binary #1612

Closed
snakethatlovesstaticlibs opened this issue Dec 14, 2023 · 5 comments
Closed

Cannot use py_runtime with the output of a sh_binary #1612

snakethatlovesstaticlibs opened this issue Dec 14, 2023 · 5 comments

Comments

@snakethatlovesstaticlibs
Copy link

snakethatlovesstaticlibs commented Dec 14, 2023

🐞 bug report

Affected Rule

The issue is caused by the rule: py_runtime

Is this a regression?

No

Description

We're not able to use py_runtime with the output of a sh_binary. This is useful because we want to customize how python searches for dynamic libraries at runtime.

🔬 Minimal Reproduction

https://github.com/snakethatlovesstaticlibs/bazel-py-runtime-repro

🔥 Exception or Error


BUILD:11:11: in interpreter attribute of py_runtime rule //:python3_runtime: '//:wrapped_python' must produce a single file. Since this rule was created by the macro 'py_runtime', the error might have been caused by the macro implementation

🌍 Your Environment

Operating System:

  
  Linux 6.5.11-linuxkit #1 SMP PREEMPT Mon Dec  4 11:30:00 UTC 2023 aarch64 aarch64 aarch64 GNU/Linux and
  Darwin 22.6.0 Darwin Kernel Version 22.6.0
  

Output of bazel version:

  
bazel 7.0.0
  

Rules_python version:

  
latest main / b6ba317b8227c6d182d5e41d2571a217ecdf11a9d3ebc2cdef3bc5503fca2e76
  

Anything else relevant?

Related to bazelbuild/bazel#4286

@rickeylev
Copy link
Contributor

A typical binary rule should just have one output (the executable). Is sh_binary one of those old, funny, rules (like py_binary 🙈 ) where it has two outputs: the executable and the main file?

Looking at the py_runtime.interpreter attribute, it is a bit funny that it's defined as a single file, rather than an executable. (I don't see how it could be an arbitrary file that isn't executable)

I don't see a problem with changing it to allow_files=True and modifying the logic to take the executable (if it is an executable), otherwise require it to be a single file. I've had to do similar for other rules because of py_binary.

@snakethatlovesstaticlibs
Copy link
Author

Thanks for the quick response. I'm still pretty new to bazel, so I could potentially make the changes with guidance. I had some questions about this approach:

  • Do you have any examples for other rules that modify the logic based on whether the input is executable?
  • Which changes would be required to the consumers of the py_runtime?
  • Would we need to make changes to any providers to get this working?

@rickeylev
Copy link
Contributor

Do you have any examples for other rules that modify the logic based on whether the input is executable?

You can look at the py_runtime.coverage_tool attribute for an example of how a label attribute can be treated as an executable or a singleton file.

Which changes would be required to the consumers of the py_runtime?

None. A single-file target is expressed the same way as a multiple-file target (they're both just //foo:bar.

Would we need to make changes to any providers to get this working?

Maybe? That depends on what the target being passed as the interpreter needs at runtime.

The only issue I can think of is that py_runtime and PyRuntimeInfo don't handle the runfiles of the in-build interpreter, they just take a plain set of files (via the py_runtime.files attribute). So long as the runfiles are just plain files (i.e. only runfiles.files has data, and the other runfiles.<xxx> attributes are empty), then there isn't a problem (the two sets are basically the same).

I suggest first making py_runtime.interpreter accept a sh_binary, and then seeing what fails next. You'll likely need to merge ctx.attr.interpreter.default_runfiles.files into the PyRuntimeInfo.files value. Ideally it would more directly propagate the entire runfiles data, but making that work would require some changes to py_binary, which has to wait until the starlark-based implementation is fully enabled (almost there, with bazel 7 out we can now finish this process).

@snakethatlovesstaticlibs
Copy link
Author

Awesome, made some changes which allow py_runtime to accept a sh_binary and that part seems to work

So long as the runfiles are just plain files (i.e. only runfiles.files has data, and the other runfiles. attributes are empty), then there isn't a problem (the two sets are basically the same).

Unfortunately I instantly ran into this issue. My sh_binary's DefaultInfo has a non empty default_runfiles which causes missing runtime dependencies I think. I'll post a PR with my current progress soon

github-merge-queue bot pushed a commit that referenced this issue Jan 8, 2024
…the interpreter (#1621)

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

---------

Co-authored-by: Richard Levasseur <rlevasseur@google.com>
@snakethatlovesstaticlibs
Copy link
Author

Unfortunately I instantly ran into this issue. My sh_binary's DefaultInfo has a non empty default_runfiles which causes missing runtime dependencies I think.

This was actually inaccurate on my part. I was getting confused by the fact that runfiles are all present in one directory, and not always in the same directory as the binary that they are for.

The latest PR fixed our use case, thank you!

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

No branches or pull requests

2 participants