-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
move lock: in-place dependency graph updates #16788
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ use std::{ | |
collections::{btree_map::Entry, BTreeMap, BTreeSet, VecDeque}, | ||
fmt, | ||
fs::File, | ||
io::{BufWriter, Read, Write}, | ||
io::{Read, Write}, | ||
path::{Path, PathBuf}, | ||
process::Command, | ||
}; | ||
|
@@ -1214,12 +1214,8 @@ impl DependencyGraph { | |
/// This operation fails, writing nothing, if the graph contains a cycle, and can fail with an | ||
/// undefined output if it cannot be represented in a TOML file. | ||
pub fn write_to_lock(&self, install_dir: PathBuf) -> Result<LockFile> { | ||
let lock = LockFile::new( | ||
install_dir, | ||
self.manifest_digest.clone(), | ||
self.deps_digest.clone(), | ||
)?; | ||
let mut writer = BufWriter::new(&*lock); | ||
use fmt::Write; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would personally just put the trait There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For sure! In this case, there is already a |
||
let mut writer = String::new(); | ||
|
||
self.write_dependencies_to_lock(self.root_package_id, &mut writer)?; | ||
|
||
|
@@ -1235,15 +1231,47 @@ impl DependencyGraph { | |
self.write_dependencies_to_lock(*id, &mut writer)?; | ||
} | ||
|
||
writer.flush()?; | ||
std::mem::drop(writer); | ||
let mut dependencies = None; | ||
let mut dev_dependencies = None; | ||
let mut packages = None; | ||
if !writer.is_empty() { | ||
let toml = writer.parse::<toml_edit::Document>()?; | ||
if let Some(value) = toml.get("dependencies").and_then(|v| v.as_value()) { | ||
dependencies = Some(value.clone()); | ||
} | ||
if let Some(value) = toml.get("dev-dependencies").and_then(|v| v.as_value()) { | ||
dev_dependencies = Some(value.clone()); | ||
} | ||
packages = toml | ||
.get("move") | ||
.and_then(|m| m.as_table()) | ||
.and_then(|move_table| move_table.get("package")) | ||
.and_then(|v| v.as_array_of_tables().cloned()); | ||
} | ||
|
||
use std::io::Seek; | ||
let mut lock = LockFile::new( | ||
install_dir, | ||
self.manifest_digest.clone(), | ||
self.deps_digest.clone(), | ||
)?; | ||
lock.flush()?; | ||
lock.rewind()?; | ||
Comment on lines
+1252
to
+1259
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still unconditionally create a We rewind the lock file cursor so that it's ready to be read by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to just work with strings (or some other in-memory representation) everywhere instead of the file that you need to keep rewinding, then at the end, you can put it back into the file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd like something like that, but it's tricky. In effect we'd need to thread that string along wherever we'd like to avoid a write to the file, which is intimidating--mostly because the control flow is tricky, and the points at which various things care to commit or write to the file is spread out over that control flow (meaning the callers will then read/write anyway). I'm happy to try rework this later, but consistent updates to the lock file path this way abstracts away from the callers and avoids passing a string around. Because one thing I think doesn't work is to have, e.g., Again I may revisit and have these return strings, it might make sense, though there's also a chance that change will clutter up callers with needing to write to disk at various places. |
||
|
||
schema::update_dependency_graph( | ||
&mut lock, | ||
self.manifest_digest.clone(), | ||
self.deps_digest.clone(), | ||
dependencies, | ||
dev_dependencies, | ||
packages, | ||
)?; | ||
Ok(lock) | ||
} | ||
|
||
/// Helper function to output the dependencies and dev-dependencies of `name` from this | ||
/// dependency graph, to the lock file under `writer`. | ||
fn write_dependencies_to_lock<W: Write>( | ||
fn write_dependencies_to_lock<W: fmt::Write>( | ||
&self, | ||
id: PackageIdentifier, | ||
writer: &mut W, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates the relevant TOML values for the dependency graph in
file
without affecting any other part of the contents.Currently,
file
passed here is always generated from scratch (as before). But in upcoming changes,file
will be an existing lock file, e.g., containing automated address management info, that will now remain intact when the dependency graph is updated via this function.