-
-
Notifications
You must be signed in to change notification settings - Fork 686
plumb through more of Pex's --scie flags #22996
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
Conversation
* --scie-pbs-free-threaded / --scie-pbs-debug which are new since when I started pantsbuild#22866 * --scie-load-dotenv https://github.com/pex-tool/pex/releases/tag/v2.75.0 * revised entrypoint https://github.com/pex-tool/pex/releases/tag/v2.76.0 Since there has not been a relase with these yet, I've changed the name without doing a depreciation cycle
| args = [] | ||
| if self.scie.value is not None: | ||
| args.append(f"--scie={self.scie.value}") | ||
| if self.scie_load_dotenv.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 think this nested conditional is more verbose, but means we only pass something to the underlying tool when explicit instead of mirroring defaults. I know Pex isn't going to break, but I mean this as a general 'get out of the way' point.
| default_version = _PEX_VERSION | ||
| default_url_template = "https://github.com/pex-tool/pex/releases/download/{version}/pex" | ||
| version_constraints = ">=2.13.0,<3.0" | ||
| version_constraints = ">=2.76.0,<3.0" |
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 having a bit of a blank, would this prevent users from modifying this via pants.toml (e.g. limit them?) - or does this just act as a default and is overridable?
As in, if someone is using a newer pants, would they be able to downgrade to earlier pex?
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 prevents using older versions of Pex. As in this would error out:
[pex-cli]
version = "v2.0.1"
The alternative would be that pants some-goal some-target errors out when using some feature that isn't present, which is worse IMO and I think we are doing the normal thing with lower version bounds.
You can't -- today -- override version_constraints, but I think there are few ways to add an override if one wanted.
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.
Hmm, not sure how I feel about this. A user with a pinned pex-cli (2.75.0) upgrades Pants and this breaks...?
Does Pants have (or has it ever had) some concept of something like this in Swift @available(iOS 10.0, *)?
So, like, the functionality is only exposed to the user under some condition @available(pex-cli > 2.76) sorta idea... At the target level. I'm digging through the codebase, as I thought I'd seen something like that before, but it might have been in a specific backend
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 thinking of these... So, the opposite.
class Target:
...
removal_version: ClassVar[str | None] = None
removal_hint: ClassVar[str | None] = None
I think we should chat briefly in the meeting, because summarily changing the lower bound when adding a target/field feels ... wrong... I'd rather we have a mechanism by which a user with old requirement gets notified at runtime they can't use a target or field (as this should be a one-time thing - could even be a lint).
Can also pull that information out and add it into the docs as well.
I'm happy to take this on, fyi
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.
You can't -- today -- override version_constraints, but I think there are few ways to add an override if one wanted.
Well I am silly, we already had use_unsupported_version since the beginning (#12332) to cover this case
| use_unsupported_version = EnumOption( |
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 have never noticed that before. I guess that'd be something to call out in the release notes as well
|
Meta comment: I think the train left the station a lonnnnng time ago - but we probably shouldn't be exposing each and every API field for each and every tool. Feels like we would want the minimal set of flags, and then let Or like, we would setup the default/reasonable setup - and then the user can extend or overwrite that. Unless we need to do something in pants for a given arg (like how in typecheckers, we might overwrite where the venvs are). Anyways, not a blocker for this ticket - just something we should take note of for future changes/backends. |
I think historically backends often follow a pattern like: expose a few things to start --> expose a few more --> whack-a-mole --> add an But absent some future alternative design, I think eventually plumbing them for BUILD files is useful. I know I have a lot of macros/defaults around these target types. |
😆 Yeah, me too. I'd seen Anyways, yeah, in many of these tools - if they ever have a breaking CLI change, either we break, or maintain backports - both of which seem silly given the givens. |
Since there has not been a relase with these yet, I've changed the name without doing a depreciation cycle