A --resolve flag, to specify the resolves to export. #17416
Conversation
|
I'll try and review this this week, works been crazy. One question: why py-resolve? I assume that's because it's coming from the python plugin and otherwise we'd be squatting all of "resolve"? 🤔 |
|
Just confirming that as a user, the proposed new behavior makes much more sense to me than the way things work now. |
Exactly. If/when we implement export for JVM resolves for example... And it's not |
|
Should this be a union instead, so it matches |
I think Josh has this right. If |
Oh, yeah, that is a good idea! |
|
There would be a slight quirk in that you could then specify that option even if resolves weren't a relevant concept in any backend you're using, but those backends do have other "export" semantics. But that seems unlikely to be a problem in practice. |
|
Unions are very oddly powerful in the engine, let's keep leaning 😈
Do you have an example in mind? |
|
There are some differences between this and generate-lockfiles. With the latter it makes sense to have a generic-level In this case we'd be adding a |
|
I think the area of "dynamic help" can certainly be explored (and should!). Additionally, |
I don't understand what you mean by either of these? But in any case, I think the latest iteration solves the problem. |
benjyw
changed the title
There was a problem hiding this comment.
The code lgtm. I'll leave review of this problem of export claiming to be agnostic of what exactly is exported vs lift the --resolve option up into its purview to others. I think the option is in the right place for end users and with the right name; so any future work to get the option pushed there by some plugin mechanism / unions should be non-end-user-thrashing afaict.
|
Yeah, I think this is an acceptable concession to the fact that while export is agnostic to what backends do in theory, in practice it's a very common (and currently ~only) use case |
There was a problem hiding this comment.
Only looked at the high level interface. I'm happy to take a look at the code tomorrow.
I strongly agree with calling this --resolve and having it generic, rather than --py-resolve. That is good alignment with generate-lockfiles. I also generally am in favor of only supporting export when you use resolves. NB that that will still work even if you use manually generated lockfiles, like via poetry export! (A good thing). Given the security importance, I'm generally more and more in favor of how we can push people to use resolves.
My only uncertainty is what ./pants export should do with no args. Right now, I think it no-ops? Should we instead be like generate-lockfiles that we export every resolve? If we didn't have the CLI specs legacy, would that change the answer?
| @@ -34,29 +34,25 @@ See [Use of the PYTHONPATH variable](https://code.visualstudio.com/docs/python/e | |||
| Python third-party dependencies and tools | |||
| ----------------------------------------- | |||
There was a problem hiding this comment.
🔥 Excellent rewrite! Thanks so much for all the improvements you've made to export.
Good question. I think it should no-op, because resolves are only one of the things you might want to export (this is where the symmetry with generate-lockfiles breaks down). E.g., I want to move export-codegen under export. So why would we assume that no args means "all the resolves"? Many other goals no-op when given no arguments, e.g., |
Currently, export takes cmd-line specs, so the python export implementation, which exports virtualenvs, exports the requirements of those targets. This has a couple of issues: A) User intent is almost certainly that export should emit the entire lockfile, whereas we will omit anything that isn't actively depended on by some target in the specs (which could be the case even with `::`). B) Tools don't relate to specs, so we export every tool every time, no matter the specs, which is obviously clunky. This change re-envisions how export should work. Instead of passing specs, you pass the `--resolve` flag one or more times, and we export a venv for each resolve, whether it's a user resolve or a tool resolve. The old codepath still works, but we can consider deprecating it in a followup. Closes pantsbuild#17398
…17416) (#17461) Currently, export takes cmd-line specs, so the python export implementation, which exports virtualenvs, exports the requirements of those targets. This has a couple of issues: A) User intent is almost certainly that export should emit the entire lockfile, whereas we will omit anything that isn't actively depended on by some target in the specs (which could be the case even with `::`). B) Tools don't relate to specs, so we export every tool every time, no matter the specs, which is obviously clunky. This change re-envisions how export should work. Instead of passing specs, you pass the `--resolve` flag one or more times, and we export a venv for each resolve, whether it's a user resolve or a tool resolve. The old codepath still works, but we can consider deprecating it in a followup. Closes #17398
|
Good call on it doing nothing. I didn't look closely at the change, but we'll want to make sure we have a good warning if no args are given so users know what to do. Thanks for making this improvement! |
Currently, export takes cmd-line specs, so the python export implementation, which
exports virtualenvs, exports the requirements of those targets.
This has a couple of issues:
A) User intent is almost certainly that export should emit the entire lockfile, whereas we will omit anything
that isn't actively depended on by some target in the specs (which could be the case even with
::).B) Tools don't relate to specs, so we export every tool every time, no matter the specs, which is obviously clunky.
This change re-envisions how export should work. Instead of passing specs, you pass the
--resolveflag one or more times, and we export a venv for each resolve, whether it's a user resolve or a tool resolve.The old codepath still works, but we can consider deprecating it in a followup.
Closes #17398