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 passing arguments to PEX invocation used for pex_binary #20737

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Apr 2, 2024

This adds a new field extra_build_args to pex_binary that just faithfully passes through arbitrary arguments to the pex ... invocation used to build the PEX. This gives users more control and flexibility for new PEX features that Pants might not (yet) expose natively.

For instance, the functionality requested in #20227 can now be solved by passing extra_build_args=["--no-compress"], and similarly people can now start taking advantage of the --no-pre-install-wheels functionality for their own PEXes (relates to #20562).

@huonw
Copy link
Contributor Author

huonw commented Apr 2, 2024

In #20227, I indicated that maybe this sort of functionality could solve that issue, i.e. we expect someone who wants to pass --no-compress to use pex_binary(..., extra_build_args=["--no-compress"]) and that's the only way.

If that's the way we want to go, we could potentially deprecate all the random fields that just set an argument (with no fancy pants functionality attached), e.g. sh_boot, include_tools, etc. We're then just expecting the user to know more about the tool they're using (PEX), and Pants is a "thinner" command runner.

However, this would mean something like __defaults__ doesn't work as well: for instance, someone might usually want no compression and sh-booting, but some binaries should be compressed. With only extra_build_args, a lot more things need to repeat.

# with extra_build_args only:
__defaults__({pex_binary: dict(extra_build_args=["--no-compress", "--sh-boot"])})

pex_binary(name="all-defaults", entry_point="foo.py")
pex_binary(name="half-defaults", entry_point="bar.py", extra_build_args=["--compress", "--sh-boot"]) # have to remember to pass the other defaults

# with dedicated fields per flag:
__defaults__({pex_binary: dict(compress=False, sh_boot=True)})

pex_binary(name="all-defaults", entry_point="foo.py")
pex_binary(name="half-defaults", entry_point="bar.py", compress=True) # sh_boot default still applies

Thus, I'm now thinking that it would actually be good to add fields for each flag.

Thoughts?

I don't think this should block this PR though, having this extra_build_args functionality will give people more power for new PEX functionality, even if we do eventually expose all the flags in a dedicated way.

@huonw huonw marked this pull request as ready for review April 2, 2024 23:42
@benjyw
Copy link
Sponsor Contributor

benjyw commented Apr 3, 2024

Thoughts?

I don't know that it's necessary to deprecate, but in the planned new Python backend it will make sense to learn from this and be "thinner".

@benjyw
Copy link
Sponsor Contributor

benjyw commented Apr 3, 2024

Nice!

@cburroughs
Copy link
Contributor

I thin this PR is good and this should exist even if we think of it as "just" an escape hatch.

In general I think it's really hard to layer "thick abstractions" successfully and one commonly ends up where you need to be an expert in both the underlying tool and the one that is purporting to wrap it. I'm certainly disposed toward "thinner" abstractions in general. (There are a lot of geologic strata in Python packaging-land though so that might not be the best example, it's more like a 500% problem.)

I think the typing and composability of fields is pretty valuable though and I'd still lean towards adding fields for most everything the underlying tool exposes. They are usually pretty cheap. If the interface became effective just args orsubprocess.run I think everyone would end up re-writing a ton of ad-hoc macro code to deal with that. Maybe thinking of the job of target and fields as "be the best abstraction for composing args" might lead in a different direction, but I'm not sure quiet what that would look like.

@huonw
Copy link
Contributor Author

huonw commented Apr 5, 2024

There are a lot of geologic strata in Python packaging-land though so that might not be the best example, it's more like a 500% problem

😂

I think the typing and composability of fields is pretty valuable though and I'd still lean towards adding fields for most everything the underlying tool exposes. They are usually pretty cheap. If the interface became effective just args orsubprocess.run I think everyone would end up re-writing a ton of ad-hoc macro code to deal with that

Yeah, that's fair. I think that's a better distillation of what I was attempting to reach for with the __defaults__-focused line of thinking, but hadn't crystallised as precisely, thank you!

@huonw huonw merged commit ff0f561 into main Apr 5, 2024
24 checks passed
@huonw huonw deleted the huonw/arbitrary-pex-args branch April 5, 2024 03:28
huonw added a commit that referenced this pull request May 15, 2024
This moves the `extra_build_args` from being a field on `pex_binary`
only to being a common field, so it appears on both `pex_binary` and the
`pex_binaries` target generator.

I'd missed this in #20737.
AlexTereshenkov added a commit that referenced this pull request Aug 13, 2024
…ss globally (#21202)

A new option `[pex-cli].global_args` has been added to be able to pass
arbitrary arguments to the `pex` tool as part of any Pants goal
invocation. This should make it a lot easier to modify behavior of `pex`
tool without needing to make changes in the Pants codebase.

Not having this ability to pass arbitrary arguments to pex makes it
really hard to start taking advantage of new features that come with
newer versions of pex. For instance, the new `--exclude` flag added
recently would require making lots of changes in the codebase to be able
to pass those extra arguments. This is because the pex invocations in
the Pants codebase are numerous and it's really hard to make sure a
particular argument is respected (by keeping the chain of calls correct
making sure it does reach the final subprocess spawn). And if it's
relevant for multiple goals, this becomes even harder.

We would still need to make changes to pass arguments to individual
targets, see #20737 or
#20939 - this makes sense as
those arguments apply only to those targets. However, some options would
need to apply for any `pex` invocation (e.g. ignoring all 3rd party
dependencies).

I've looked into having environment variables support for all flags that
PEX has (pex-tool/pex#2242) first (so that no
changes are needed in Pants, one would just export a bunch of env vars
as needed), but this is not going to happen any time soon, so doing it
in the Pants codebase instead is the only path forward, I reckon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants