-
-
Notifications
You must be signed in to change notification settings - Fork 628
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
Add plugin support for building PyOxidizer apps #14183
Conversation
@@ -0,0 +1,92 @@ | |||
# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm completely skeptical of this whole file, but I'm not sure what a better approach is. Should there even be a "default" configuration if the user doesn't pass one in? There are a ton of configuration options - and while this is the most reasonable by default (and thus, probably the easiest for a new user to start with) - it feels a bit dicey too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template is quite a bit of boiler plate, so a basic example to get going is reasonable to have, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tricky to say.
One potential razor to help decide whether the plugin should attempt to lean in and provide a default template might be whether we think that we can maintain a template over time that is able to build an artifact from most distributions using a reasonable number of arguments on the target to control behavior.
For example: if we think that by exposing less than a dozen options (unclassified_resources=
, etc) from pyoxidizer_binary
we can build 90% of distributions, then it might be worth trying.
I really don't know. But one thing that would seem to be in favor of that approach is that installing from a distribution seems like a relatively self-contained situation (unlike building from loose sources and dynamically adding requirements).
async def package_pyoxidizer_binary( | ||
pyoxidizer: PyOxidizer, field_set: PyOxidizerFieldSet | ||
) -> BuiltPackage: | ||
logger.info(f"Incoming package_pyoxidizer_binary field set: {field_set}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be making these 'debug' once I get my unit tests running
) | ||
|
||
config_template = None | ||
if field_set.template.value is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't fully able to grok how configs like isort and black were auto-magically picked up, so this is configured in the field_set (also depends on whether there should be a default, programmatic config or not)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For isort
and black
, the files are expected to be located at certain places in the repository, and so the config scraping in those cases is a convenience to avoid needing to specify a config. In this case, requiring a per-binary config seems completely reasonable, since I don't think that any two binaries are likely to share a config.
For the moment, I have a bunch of sample apps here (tested on MacOS only) here: https://github.com/sureshjoshi/pants-pyoxidizer-plugin If this PR gets merged at some point, I'll split the examples off into a separate repo - as there aren't many PyOxidizer examples in the wild at all. Additionally, as per Slack, I can't seem to get my tests running in my example repo, so I'm going to try to write them in the mainline Pants repo and see if I have better luck. Some other issues which I'd like to tackle (maybe in a future PR) related to this functionality are here: https://github.com/sureshjoshi/pants-pyoxidizer-plugin/issues Aside from review, this PR is waiting on my unit tests so that there is some level of coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool!
Looks great so far. Only made a quick cursory review.
@@ -0,0 +1,92 @@ | |||
# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template is quite a bit of boiler plate, so a basic example to get going is reasonable to have, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Let us know how we can help you get it reviewable.
src/python/pants/backend/python/packaging/pyoxidizer/subsystem.py
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,92 @@ | |||
# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tricky to say.
One potential razor to help decide whether the plugin should attempt to lean in and provide a default template might be whether we think that we can maintain a template over time that is able to build an artifact from most distributions using a reasonable number of arguments on the target to control behavior.
For example: if we think that by exposing less than a dozen options (unclassified_resources=
, etc) from pyoxidizer_binary
we can build 90% of distributions, then it might be worth trying.
I really don't know. But one thing that would seem to be in favor of that approach is that installing from a distribution seems like a relatively self-contained situation (unlike building from loose sources and dynamically adding requirements).
# pip_download requires that wheels are available for each dep | ||
# exe.add_python_resources(exe.pip_download($WHEELS)) | ||
exe.add_python_resources(exe.pip_install($WHEELS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll likely want to resolve the relevant wheels via PEX at some point (since that will apply a user's repository settings and etc), but fine as a TODO for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, there will be a lot of re-factoring to be done on this while in experimental :)
|
||
|
||
class PyOxidizerEntryPointField(StringField): | ||
alias = "entry_point" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python_distribution
supports setting entrypoint(s) as well, IIRC. Not sure how complex it would be to default to that (...and for it to be what decides that we fallback to a repl
, if that's going to happen).
Thanks for the reviews @stuhood and @kaos - I'm just trying to grab a night to fiddle with getting some level of testing working (still running into problems as per Slack), and then I'll be fine taking it out of draft. I'm hoping to get a few hours tomorrow night to fiddle around so I can get this ready. I have two more plugins in the works, so need to free up some time for those too! |
…ype help - Re-named ENTRY_POINT to RUN_MODULE in template - Changed logger.info to debug inside rule - Added pyoxidizer to Pants init BUILD file
…eck for the pyoxidizer target
@stuhood @kaos Sorry for the delay on this, was KO'd with food poisoning for about a week 🤦🏽♂️ I resolved a few of the issues, and created some sanity unit tests - I'd happily put it into review. Note: I wasn't ever able to get the mock'd rules working for the tests, error city over here still - and need to dig into why. So, I just used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
I have only nits that aren't worth bugging you with on your first review: when you're ready to land this (and once it goes green), I'll go ahead and apply them.
Thanks a lot for the contribution!
PexProcess( | ||
pyoxidizer_pex, | ||
argv=["build", *pyoxidizer.args], | ||
description="Running PyOxidizer build (...this can take a minute...)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description="Running PyOxidizer build (...this can take a minute...)", | |
description="Running PyOxidizer build for {field_set.address.spec}", |
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
(pushed some fixes so that CI can make some more progress) |
python_tests( | ||
name="config_test", | ||
sources=["config_test.py"], | ||
) | ||
|
||
python_tests( | ||
name="rules_integration_test", | ||
sources=["rules_integration_test.py"], | ||
timeout=240, | ||
interpreter_constraints=[">=3.9"] | ||
) | ||
|
||
python_tests( | ||
name="subsystem_integration_test", | ||
sources=["subsystem_integration_test.py"], | ||
) | ||
|
||
python_tests( | ||
name="target_types_integration_test", | ||
sources=["target_types_integration_test.py"], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fly by comment, a more idiomatic pattern is to use a single python_tests
target with default sources
, along with overrides
for rules_integration_test
. It results in less boilerplate.
Tip that update-build-files
is nice to handle formatting of the BUILD file via Black, overrides
is annoying to manually format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Eric-Arellano! I was actually copying from another plugin - in my own projects, I only ever have a single python_test
per repo, but I assumed there was some reason for this layout regarding the pants test infra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I assumed there was some reason for this layout regarding the pants test infra.
Only that we haven't finished updating all our BUILD files to use the best idiom. "Generated targets" have been a thing since Pants 2.0, but they were in the shadows -- you could only explicitly create a python_test
target + use overrides
starting in Pants 2.8. Before then, the BUILD file you have here is roughly the best we could get.
I have a WIP PR to update them that I sometimes work on when I need a trivial thing to do as a break from other things 😀
I've requested an outside opinion on this, but if we don't get it we can land tomorrow. Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably room for a feature in PyOxidizer that can streamline the production of a binary given narrowly constrained inputs (such as the name of a package to pip install
or a requirements file). But in the absence of said feature, templating the pyoxidizer.bzl
file is a viable solution and possibly the path of least resistance.
If you can think of the design of a PyOxidizer command to streamline this, feel free to make noise in PyOxidizer's issue tracker. My GitHub issue response time can be high though. Twitter pings tend to get my attention though :)
Woot! |
This is super cool! I look forward to soon building Pants itself with PyOxidizer... |
Adding a new experimental plugin for creating executable binaries using PyOxidizer.
The plugin's current functionality is proof-of-concept, with minimal configuration capabilities, and minimal edge-case checking (until a maintainer reviews draft PR for suggestions).
The new
pyoxidizer_binary
target expects a wheel-based python distribution dependency, and optionally supports a PyOxidizer configuration template (with the extension.bzlt
). If no template is specified, it will use a default configuration with reasonable defaults. The complexity of PyOxidizer's configuration, however, would suggest custom configurations will be the norm. Additionally, afilesystem_resources
target field exists to handle certain edge cases listed in PyOxidizer's documentation (unclassified resource dependencies). Theentry_point
field is optional, however, if it is not included - the PyOxidizer binary will default to a REPL.Recent PyOxidizer's require Python 3.8 or greater - which is setup as a default interpreter_constraint.
The only other option is to set command-line args (e.g. setting release mode, or setting the target tuple).
Example pants.toml option:
Example BUILD file:
./pants package helloworld:
(Closes issue #14144)