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

Adding support to have all command line flags respected via environment variables #2475

Closed
AlexTereshenkov opened this issue Jul 24, 2024 · 6 comments

Comments

@AlexTereshenkov
Copy link

AlexTereshenkov commented Jul 24, 2024

I'd love to see PEX supporting receiving any command line argument by reading a corresponding environment variable. This means that all (or most) command line options listed under pex --help would be having a matching env var. E.g. running

$ pex --seed=verbose --compile

would be equivalent to running

PEX_SEED=verbose PEX_COMPILE=1 pex

PEX does have PEX runtime environment variables, but it only covers some of the flags.

The motivation for this is two fold:

  1. It's quite common from what I've seen for cli tools to make it possible to declare environment variables instead of passing the flags, e.g. https://github.com/buchgr/bazel-remote?tab=readme-ov-file#command-line-flags. This can be done for various purposes such as convenience or security (not exposing the values in the terminal). The pex invocation can become quite lean with most of the complicated logic stored in Shell scripts or elsewhere.

  2. Clients who make calls to Pex do not need to be modified for every new flag that pex expose. Pants is of primary concern.

As an example, in a recent PR, a user added pex_build_extra_args flag option to be able to propagate a PEX command line flag from Pants invocation down the line to pex itself:

    Additional arguments to pass to the `pex` invocation that is used to collect the requirements
    and sources for packaging.
    For example, `pex_build_extra_args=["--exclude=pypi-package-name"]` to force a package called
    `pypi-package-name` isn't included in the artifact.
    Note: Excluding dependencies currently causes Pex to throw an error. You can additionally pass
    the `--ignore-errors` flag.

There was at least one question about this on Pantsbuild Slack.

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

To avoid going the route of raising individual PRs in Pants trying to add support for random PEX flags user may want to have, it would be so much easier to be able to set the environment variables.


I appreciate pex isn't part of Pants so its development might not be driven by Pants users needs, but I hope that use case (1) is strong enough for this enhancement to be considered.

@benjyw
Copy link
Collaborator

benjyw commented Jul 24, 2024

Hi @AlexTereshenkov , doesn't Pants's pex_build_extra_args solve this problem there in practice?

@AlexTereshenkov
Copy link
Author

AlexTereshenkov commented Jul 24, 2024

Hi @AlexTereshenkov , doesn't Pants's pex_build_extra_args solve this problem there in practice?

Hey Benjy! It would, but only for that particular target. I want the PEX args to be passed on any Pants goal invocation, not just when dealing with that particular target type.https://www.pantsbuild.org/2.22/reference/targets/python_aws_lambda_function

I hope this makes sense?

@benjyw
Copy link
Collaborator

benjyw commented Jul 24, 2024

This seems easy to add, at least globally, in the pex-cli subsystem, no?

@jsirois
Copy link
Member

jsirois commented Jul 24, 2024

Thankfully @benjyw stepped in here but @AlexTereshenkov this clearly is a problem for Pants to be handling in its own shed. I happen to know both the Python rule side Process dataclass and the rust side struct have slots for both args and env vars. How is it easier to fill one and not the other? Sounds like a Pants problem to me.

I'll just point out all the things off-putting in this issue filing:

  1. You didn't try too hard to look around: Add support for controlling Pex CLI via env var or arg. #2242

  2. You give a 2-fold motivation of which your only real motivation is item 2. Always lead with your best argument - never pad arguments with weak bullet points. Its disingenuous.

  3. Your motivation for point 1 is crappy:

    It's quite common from what I've seen for cli tools to make it possible to declare environment variables instead of passing the flag

    This much, fine, but see Add support for controlling Pex CLI via env var or arg. #2242 - clearly I'm aware of this.

    This can be done for various purposes such as convenience or security (not exposing the values in the terminal).

    Half OK - convenience, sure?, but security - an irrelevant red herring point since PEX currently accepts 0 command line arguments that need or should contain credentials.

    The pex invocation can become quite lean with most of the complicated logic stored in Shell scripts or elsewhere.

    With a shell script you can seal in options just as well as you can env vars so this point makes 0 sense: alias pex 'pex --my-favorite-setting1 foo ...

  4. You point to a PR (Allow Extra Args to be passed to FaaS Pex Build pantsbuild/pants#20939) that succeeds at plumbing via args, is approved by another Pants maintainer that set the example themselves with these PRs: Allow passing arguments to PEX invocation used for pex_binary pantsbuild/pants#20737 and Include extra_build_args on pex_binaries too pantsbuild/pants#20929 and is specifically plumbing --exclude which is already something Pants supports 1st class for, for example, jvm deps where the ecosystem has support for excludes. IOW the PR you point to is exactly an example of a quick hack to work around Pants not supporting deep excludes integration, which, I think Pants maintainers would agree it should.

@jsirois
Copy link
Member

jsirois commented Jul 24, 2024

I suspect it won't shed favorable light on the OP, but I will note https://pantsbuild.slack.com/archives/C046T6T9U/p1695281564840179 is not accessible to me. I am no longer a member of either the Pantsbuild GitHub org or the Slack. @AlexTereshenkov if you can provide a linen link or copy paste the whole thread you point to, that would be great.

@jsirois
Copy link
Member

jsirois commented Jul 24, 2024

@AlexTereshenkov I'm going to close this in favor of #2242 since it adds nothing over that issue. I filed that quite a while ago and still haven't gotten to it; so thast should make it clear I'm unlikely to be getting to it any time soon. If you want to pitch in and do the work in Pex to make it so, great. If you'd rather put the work into Pants to make it handle controlling the tools it launches more flexibly and more generally, probably better - more bang for your buck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants