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

[internal] Hook up [python-repos] and manylinux to Pex lockfile generation #14282

Merged
merged 2 commits into from
Jan 27, 2022

Conversation

Eric-Arellano
Copy link
Contributor

[ci skip-rust]

…eneration

# 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]
# 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]
Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah! Thanks @jsirois for your work on Pex lockfiles! It's exciting to close the loop here from when you first pointed out Poetry's insufficiencies with [python-repos].

extra_args=(
"--output=lock.json",
"--no-emit-warnings",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use this when building a PEX, so I figure we should use it here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I wonder why...

Given that we only render the output when we fail, having warnings rendered on failure might be useful...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're of course free to add args piecemeal, and maybe you have ~all the arguments Pants passes to Pex wired now (not sure from my phone) but lock create does accept all resolve options, including --platform, etc. Those will need to all get passed to be able to create the same PEX with the lock that you would without the lock.

@Eric-Arellano
Copy link
Contributor Author

including --platform

With platforms, I'd appreciate your feedback on #12612 (comment) when you have a chance. I think I'm still confused on locks & platforms.

and maybe you have ~all the arguments Pants passes to Pex wired now

Divergences from our create_pex rule:

  • no --python-repository and --pex-path
  • no --sources-directory
  • no --main
  • no --layout, which isn't available
  • no --constraints, given that we (at least for now) are having that future be mutually exclusive with lockfiles given that we abused constraints to act like locks
  • no --platform
  • no --python, which we use for internal_only to discover eagerly a single interpreter to use. I think the resolve should probably consider all valid interpreters? Installation-time would still only use one, though.

So, --platform and --python are the only ones I'm not sure about.

@Eric-Arellano Eric-Arellano enabled auto-merge (squash) January 27, 2022 17:25
@Eric-Arellano Eric-Arellano merged commit 0eda46f into pantsbuild:main Jan 27, 2022
@Eric-Arellano Eric-Arellano deleted the pex-generation-options branch January 27, 2022 17:53
@jsirois
Copy link
Contributor

jsirois commented Jan 27, 2022

  • --platform: Pex handles this 1st class for all but universal mode where it rejects --python and --platform; so today you'd have to accept delayed failure with universal (i.e.: not pass --platform) or else generate a seperate lock file using all the --platforms to get fail fast. I commented on Lockfiles: implement --platform handling #12612 though that Pex could support --platform for universal mode to get fail-fast as an added feature.
  • --python: Since Pants is buying in to universal and has taken the burden of explaining its choice to security conscious users of our tool lockfiles, this will have to be considered fine.
  • --constraints: At some point Pants should probably allow constraints back in their intended use. Its completely reasonable to want to lock but need to also constrain due to a bad actor. Without being able to constrain you take away the only tool to solve some resolves.

@Eric-Arellano
Copy link
Contributor Author

Thank you, that's helpful!

--constraints: At some point Pants should probably allow constraints back in their intended use.

I agree. And possibly we don't get rid of it during this awkward in-between period. I'm still thinking about what the migration path is going to look like.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

extra_args=(
"--output=lock.json",
"--no-emit-warnings",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I wonder why...

Given that we only render the output when we fail, having warnings rendered on failure might be useful...?

@wisechengyi wisechengyi mentioned this pull request Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants