Build source distributions in the resolver #138
Conversation
|
Thanks, for this one let's do a proper review cycle (I'll try to read it today). |
|
Please wait with proper reviewing, it's very much not ready yet |
|
Sounds good, mark it as ready when you think it's time. I'm guessing the serial building is just due to the resolution order? But not sure. |
konstin
force-pushed
the
resolve-sdist
branch
from
3700120 to
55fa8af
Compare
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
konstin
force-pushed
the
resolve-sdist
branch
4 times, most recently
from
0a4db66 to
60d4783
Compare
konstin
force-pushed
the
resolve-sdist
branch
from
60d4783 to
3f540b7
Compare
|
I tested this with |
konstin
force-pushed
the
resolve-sdist
branch
2 times, most recently
from
f8a1890 to
6fa4c77
Compare
konstin
force-pushed
the
resolve-sdist
branch
from
6fa4c77 to
0cc1f52
Compare
| // TODO(konstin): Check wheel filename | ||
| Ok(wheel) | ||
| Ok(dist_wheel.file_name().to_string_lossy().to_string()) |
There was a problem hiding this comment.
Why change the return type here? It seems strictly less flexible.
There was a problem hiding this comment.
We need to parse the WheelFilename, this avoids two unwraps later
| @@ -51,14 +51,16 @@ pub(crate) async fn pip_compile( | |||
| // Detect the current Python interpreter. | |||
| let platform = Platform::current()?; | |||
| let python = PythonExecutable::from_env(platform, cache)?; | |||
|
|
|||
| // Determine the current environment markers. | |||
| let markers = python.markers().clone(); | |||
There was a problem hiding this comment.
We need to clone either the python or the markers as the build context needs an owned PythonExecutable (it's not worth the trait async lifetime problems we get otherwise).
crates/puffin-dispatch/src/lib.rs
Outdated
| @@ -0,0 +1,198 @@ | |||
| #![allow(unstable_name_collisions)] // intersperse | |||
There was a problem hiding this comment.
What is this? Why is it necessary?
There was a problem hiding this comment.
clippy complains that's there's an unstable Iterator::intersperse that would shadow Itertools::intersperse if stabilized, but there's no activity of that happening and also it would be nice because it would just move the itertools features to std. (extended the comment.)
| let Some(file) = self | ||
| // Try to find a wheel. If there isn't any, to a find a source distribution. If there | ||
| // isn't any either, short circuit and fail the resolution. | ||
| // TODO: Group files by version, then check for each version first for compatible wheels |
There was a problem hiding this comment.
Nit: please write TODOs as TODO(konsti): Group files by version....
| client: &RegistryClient, | ||
| puffin_ctx: &impl PuffinCtx, | ||
| sdist_filename: &SourceDistributionFilename, | ||
| ) -> anyhow::Result<Metadata21> { |
There was a problem hiding this comment.
Here and elsewhere, can we use the direct Result type without the anyhow::, for consistency with the other crates?
| pub fn version(&self, name: &PackageName, version: &Version) -> PathBuf { | ||
| self.0.join(name.to_string()).join(version.to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
I find this struct strange because it doesn't actually do any cache operations, i.e., it's not itself a cache. Hmm... Could we instead just put a method like id on SourceDistributionFilename?
There was a problem hiding this comment.
I wanted a central abstraction that know the structure of the built wheel cache. It's half baked because i want to move towards a more centralized cache abstraction in the future anyway, for know i put the find_wheel part on it also so it at least acts as a cache abstraction.
| operator: Operator::Equal, | ||
| version, | ||
| } | ||
| } |
There was a problem hiding this comment.
Tempted to say we should just inline this since it's only used in one place.
There was a problem hiding this comment.
We can't allow construction arbitrary VersionSpecifier instances because e.g. foo!=1.2.* is banned, this avoids unwrapping from VersionSpecifier::new.
crates/puffin-dispatch/src/lib.rs
Outdated
| self.python.markers(), | ||
| &tags, | ||
| &self.client, | ||
| // TODO: nested builds are not supported yet |
There was a problem hiding this comment.
I don't quite understand this one. What is the limitation? What does it have to do with passing self here?
There was a problem hiding this comment.
it's done, i forgot to remove the comment
| marker: None, | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
Feels like this should be a From implementation but maybe that's strange when we don't have a wrapper type for Vec<Requirement>.
There was a problem hiding this comment.
Yeah, i hope this will become more clearly a graph query vs. a conversion once get more complex e.g. with root extras.
|
I think i've addressed all comments |
|
Cool, thanks! I'll give this another read later today and will merge it if you're offline at that point. |
charliermarsh
force-pushed
the
resolve-sdist
branch
from
3e69c5e to
a625806
Compare
This is isn't ready, but it can resolve
meine_stadt_transparent==0.2.14.The source distributions are currently being built serially one after the other, i don't know if that is incidentally due to the resolution order, because sdist building is blocking or because of something in the resolver that could be improved.
It's a bit annoying that the thing that was supposed to do http requests now suddenly also has to a whole download/unpack/resolve/install/build routine, it messes up the type hierarchy. The much bigger problem though is avoid recursive crate dependencies, it's the reason for the callback and for splitting the builder into two crates (badly named atm)