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] Add internal ability to generate lockfiles with PEX #14278

Merged
merged 4 commits into from
Jan 27, 2022

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Jan 27, 2022

We can't consume PEX lockfiles until pex-tool/pex#1583, but the generation is now stable. Part 1 of #13964.

Why land this? It lets us finish up the PEX generation code. For example, a follow-up PR can wire up [python-repos]. We can frontload some PEX-related work without blocking on pex-tool/pex#1583.

Note that we generate in PEX's JSON format and do not call pex3 lock export, as the export is lossy and does not attempt to handle things like environment markers.

[ci skip-rust]
[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]
@Eric-Arellano
Copy link
Contributor Author

I'm happy to close this if it's too premature, @jsirois .

I tried turning it on for our --internal lockfiles and it resulted in some issues like black showing up in the Pytest lockfile, and importlib-metadata no longer having an env marker to not install with newer Python versions. I suspect that is expected, though, that pex3 lock export intentionally is not doing things like env markers (importlib-metadata) and extras (Black). So, I'm not sure reason 2 of dogfooding is valuable at all.

Reason 1 of landing the code is helpful to me, but there's other stuff I can do.

@Eric-Arellano
Copy link
Contributor Author

If we don't want to expose this option to users, another approach is to add a boolean field to GeneratePythonLockfile, which only be set by tests.

That will let me get the first benefit of incremental progress on the code, without exposing this to premature dogfooding.

@jsirois
Copy link
Contributor

jsirois commented Jan 27, 2022

Yeah, not appropriate. The lock export for universal will probably never support universal export since it requires rather insane env marker fabrication gymnastics. Neither Pants, nor anyone else, can use Pex locks until Pex supports resolving its own locks.

# 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 Eric-Arellano changed the title Add [python].experimental_generate_lockfiles_with_pex [internal] Add internal ability to generate lockfiles with PEX Jan 27, 2022
# 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]
lock_content = digest_contents[0].content.decode()
header = dedent(
"""\
# This lockfile was autogenerated by Pants. To regenerate, run:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Pants pre-strip this stuff before handing to the tool to resolve with later? With a json lock file it will have to. With any lock file really. It's probably actually an error to be adding an extension to the file since Pants should not even know what the contents of those files are - and thus can't know if the comment style is corrupting of the file or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not currently -- we'll have to strip when hooking up Pex consumption, I suppose.

Would you be willing to consider changing PEX to use TOML? We recently changed Pants's proprietary JVM lock format to use TOML, in part to accommodate comments and in part because it's arguably easier to read (less nesting). I imagine a major turnoff would be needing to vendor toml or tomli.

Copy link
Contributor

@jsirois jsirois Jan 27, 2022

Choose a reason for hiding this comment

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

Yeah, Pants can't expect to strong arm the world here aside from Pex being willing to do this. I strongly suggest what Pants is doing today is not right - it should not tamper with a - supposedly - opaque file format, which lock files should be considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #14281. Feedback welcomed :)

@Eric-Arellano Eric-Arellano merged commit 3de554d into pantsbuild:main Jan 27, 2022
@Eric-Arellano Eric-Arellano deleted the pex-lockfile-generation branch January 27, 2022 16:02
@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.

2 participants