Add plugin support for building PyOxidizer apps by sureshjoshi · Pull Request #14183 · pantsbuild/pants

sureshjoshi · 2022-01-18T15:49:09Z

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, a filesystem_resources target field exists to handle certain edge cases listed in PyOxidizer's documentation (unclassified resource dependencies). The entry_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:

[pyoxidizer]
interpreter_constraints = [">=3.9"]
args = ["--release"]

Example BUILD file:

python_sources(name="libhelloworld", sources=["**/*.py"])

python_distribution(
    name="helloworld-dist",
    dependencies=[":libhelloworld"],
    wheel=True,
    sdist=False,
    provides=python_artifact(
        name="helloworld-dist",
        version="0.0.1",
        description="A distribution for the hello world library.",
    ),
)

pyoxidizer_binary(
    name="helloworld-bin",
    entry_point="helloworld.main", # Optional
    template="pyoxidizer.bzlt", # Optional
    filesystem_resources=["numpy"], # Optional
    dependencies=[":helloworld-dist"],
)

./pants package helloworld:

(Closes issue #14144)

…port

sureshjoshi · 2022-01-18T15:51:58Z

@@ -0,0 +1,92 @@
+# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).


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.

The template is quite a bit of boiler plate, so a basic example to get going is reasonable to have, I think.

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).

sureshjoshi · 2022-01-18T15:52:57Z

+async def package_pyoxidizer_binary(
+    pyoxidizer: PyOxidizer, field_set: PyOxidizerFieldSet
+) -> BuiltPackage:
+    logger.info(f"Incoming package_pyoxidizer_binary field set: {field_set}")


I'll be making these 'debug' once I get my unit tests running

sureshjoshi · 2022-01-18T15:55:20Z

+    )
+
+    config_template = None
+    if field_set.template.value is not None:


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)

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.

sureshjoshi · 2022-01-18T15:57:54Z

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.

kaos

Really cool!

Looks great so far. Only made a quick cursory review.

kaos · 2022-01-18T16:16:06Z

@@ -0,0 +1,92 @@
+# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).


The template is quite a bit of boiler plate, so a basic example to get going is reasonable to have, I think.

stuhood

This looks great! Let us know how we can help you get it reviewable.

stuhood · 2022-01-20T00:23:00Z

@@ -0,0 +1,92 @@
+# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).


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).

stuhood · 2022-01-20T00:25:36Z

+    # 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))


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.

I agree, there will be a lot of re-factoring to be done on this while in experimental :)

stuhood · 2022-01-20T00:31:29Z

+
+
+class PyOxidizerEntryPointField(StringField):
+    alias = "entry_point"


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).

sureshjoshi · 2022-01-20T00:56:03Z

This looks great! Let us know how we can help you get it reviewable.

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

sureshjoshi · 2022-01-28T19:46:38Z

@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 run_pants, as it happily works. But, to note, the pyoxidizer running on anything takes at least 1 minute on my machine, so this test is sloooowwww

stuhood

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!

stuhood · 2022-02-02T04:13:48Z

+        PexProcess(
+            pyoxidizer_pex,
+            argv=["build", *pyoxidizer.args],
+            description="Running PyOxidizer build (...this can take a minute...)",


Suggested change

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]

stuhood · 2022-02-02T04:43:48Z

(pushed some fixes so that CI can make some more progress)

Eric-Arellano · 2022-02-02T05:21:17Z

+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"],
+)


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

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.

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 😀

stuhood · 2022-02-02T23:00:07Z

I've requested an outside opinion on this, but if we don't get it we can land tomorrow. Thanks again!

indygreg

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 :)

sureshjoshi · 2022-02-03T13:48:14Z

Woot!

benjyw · 2022-02-09T01:10:07Z

This is super cool! I look forward to soon building Pants itself with PyOxidizer...

See issue pantsbuild#14144: Adding experimental PyOxidizer plugin sup…

bbc3db1

…port

sureshjoshi commented Jan 18, 2022

View reviewed changes

Comment thread pants.toml

sureshjoshi commented Jan 18, 2022

View reviewed changes

Comment thread src/python/pants/backend/python/packaging/pyoxidizer/config.py Outdated

kaos reviewed Jan 18, 2022

View reviewed changes

stuhood reviewed Jan 20, 2022

View reviewed changes

sureshjoshi added 2 commits January 28, 2022 14:22

See issue pantsbuild#14144: Added sanity tests for subsystem/target_t…

f17c87d

…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

See issue pantsbuild#14144: Created a run_pants integration sanity ch…

eb9e17a

…eck for the pyoxidizer target

See issue pantsbuild#14144: Ran fmt over code

977a4e0

sureshjoshi changed the title ~~[WIP] Add plugin support for building PyOxidizer apps~~ Add plugin support for building PyOxidizer apps Feb 1, 2022

sureshjoshi marked this pull request as ready for review February 1, 2022 13:46

stuhood approved these changes Feb 2, 2022

View reviewed changes

Format and type fixes.

06ab0d1

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

Eric-Arellano reviewed Feb 2, 2022

View reviewed changes

Bump timeout

4e59f48

indygreg reviewed Feb 3, 2022

View reviewed changes

stuhood merged commit 0143171 into pantsbuild:main Feb 3, 2022

sureshjoshi deleted the 14144-pyoxidizer-support branch February 3, 2022 15:01

		@@ -0,0 +1,92 @@
		# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).



		class PyOxidizerEntryPointField(StringField):
		alias = "entry_point"

	description="Running PyOxidizer build (...this can take a minute...)",
	description="Running PyOxidizer build for {field_set.address.spec}",

Uh oh!

Conversation

sureshjoshi commented Jan 18, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sureshjoshi commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sureshjoshi commented Jan 20, 2022

Uh oh!

sureshjoshi commented Jan 28, 2022

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuhood commented Feb 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuhood commented Feb 2, 2022

Uh oh!

indygreg left a comment

Choose a reason for hiding this comment

Uh oh!

sureshjoshi commented Feb 3, 2022

Uh oh!

benjyw commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sureshjoshi commented Jan 18, 2022 •

edited

Loading