Conversation
zsol
force-pushed
the
zsol/jj-kxyvzzmqmzrk
branch
2 times, most recently
from
ac76385 to
604bf7a
Compare
zsol
force-pushed
the
zsol/jj-kxyvzzmqmzrk
branch
from
604bf7a to
854af2c
Compare
|
I don't think using this flag should cause a sync to happen implicitly, that seems confusing. |
| "module_owners": { | ||
| "gpu": [ | ||
| "gpu-a", | ||
| "gpu-b" | ||
| ], | ||
| "gpu.a": [ | ||
| "gpu-a" | ||
| ], | ||
| "gpu.b": [ | ||
| "gpu-b" | ||
| ], | ||
| "typing_extensions": [ | ||
| "typing-extensions" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
I think I'd expect this to just be "modules" and I think I'd expect to use package ids not names in the mapping?
Why can a module have multiple packages? Should we report an owner for gpu?
There was a problem hiding this comment.
- namespace packages let a namespace be defined multiple times
- legacy namespace packages let an actual module be defined multiple times
- packages can both define the same module and there will be a deterministic preference based on search-path ordering and the kind of module (
foo.py,foo/__init__.pyandfoo/(namespace)).
| /// This adds a mapping from importable module names to the package names that provide | ||
| /// them. To do this, the venv will be synced in "inexact" mode. | ||
| #[arg(long)] | ||
| pub module_owners: bool, |
There was a problem hiding this comment.
I might prefer --report-modules, but not sure if I feel strongly yet. I'll mull it over.
| /// A mapping from importable module names to the distributions that provide them | ||
| #[serde(skip_serializing_if = "BTreeMap::is_empty", default)] | ||
| module_owners: BTreeMap<ModuleName, Vec<PackageName>>, |
There was a problem hiding this comment.
I'd probably reverse key and value in this map and make it package centric, in the sense that all metadata is attached to package node(s), rather than the other thing pointing towards a package.
| use std::fmt::Display; | ||
|
|
||
| /// The name of an importable Python module. | ||
| type ModuleName = String; |
There was a problem hiding this comment.
What about uv_pypi_types::identifier::Identifier?
| /// Include module ownership metadata in the output. | ||
| /// | ||
| /// This adds a mapping from importable module names to the package names that provide | ||
| /// them. To do this, the venv will be synced in "inexact" mode. |
There was a problem hiding this comment.
| /// them. To do this, the venv will be synced in "inexact" mode. | |
| /// them. To do this, the venv will be synced in inexact mode. |
| .collect()) | ||
| } | ||
|
|
||
| fn inspect_installed_modules(dist_info: &Path) -> Result<BTreeSet<String>> { |
There was a problem hiding this comment.
I'd move the inspection logic out of the uv crate, as the uv crate is already overly large.
| if !has_extension(dist_info, "dist-info") { | ||
| return Ok(BTreeSet::new()); | ||
| } | ||
|
|
||
| let mut modules = BTreeSet::new(); | ||
|
|
||
| let top_level = dist_info.join("top_level.txt"); | ||
| if let Ok(contents) = fs_err::read_to_string(top_level) { | ||
| for line in contents.lines() { | ||
| add_module_name(line.trim(), &mut modules); | ||
| } | ||
| } | ||
|
|
||
| let record_path = dist_info.join("RECORD"); | ||
| let record = read_record(fs_err::File::open(&record_path)?)?; | ||
| for entry in record { | ||
| add_record_module(&entry.path, &mut modules); | ||
| } | ||
|
|
||
| Ok(modules) |
There was a problem hiding this comment.
Where's that logic from?
top_level.txt is an old setuptools artifact, we shouldn't parse it.
| Ok(modules) | ||
| } | ||
|
|
||
| fn add_record_module(path: &str, modules: &mut BTreeSet<String>) { |
There was a problem hiding this comment.
The RECORD parsing logic should also handle the RECORD edge cases we handle in the existing RECORD logic
| let mut module_components = parents.to_vec(); | ||
| if *file_name == "__init__.py" { | ||
| // The parent path is the package. | ||
| } else if let Some(stem) = file_name.strip_suffix(".py") { |
| } else if let Some(stem) = file_name.strip_suffix(".py") { | ||
| module_components.push(stem); | ||
| } else if let Some(stem) = extension_module_stem(file_name) { | ||
| if stem != "__init__" { |
| fn is_identifier(component: &str) -> bool { | ||
| let mut chars = component.chars(); | ||
| let Some(first) = chars.next() else { | ||
| return false; | ||
| }; | ||
| if !(first == '_' || first.is_ascii_alphabetic()) { | ||
| return false; | ||
| } | ||
| chars.all(|char| char == '_' || char.is_ascii_alphanumeric()) | ||
| } |
There was a problem hiding this comment.
We should follow python's rules here, which includes unicode support.
4 participants
Summary
This adds a
--module-ownersflag touv workspace metadatathat makes uv collect a module -> package mapping and emit it in the metadata.Passing this flag means the command will now do a
sync --impreciseoperation with all extras and dependency groups.