[anneal][v2] Initial commit#3376
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3376 +/- ##
=======================================
Coverage 91.88% 91.88%
=======================================
Files 20 20
Lines 6076 6076
=======================================
Hits 5583 5583
Misses 493 493 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joshlf
changed the title
joshlf
force-pushed
the
G34qjom3lz7cc6hd57tgp44t4pzlstdhj
branch
4 times, most recently
from
1652fd0 to
af503ff
Compare
mdittmer
left a comment
mdittmer
left a comment
There was a problem hiding this comment.
Presumably the name foo is a pointer for us to finish bikeshedding on toolchain/toolchain-config/setup/install?
| let entries: Vec<_> = fs::read_dir(temp.path()) | ||
| .unwrap() | ||
| .map(|e| e.unwrap().file_name()) | ||
| .filter(|n| n != ".dirlock") | ||
| .collect(); | ||
| assert!(entries.is_empty()); |
There was a problem hiding this comment.
I don't understand the interaction between guard and these expectations. AFAICT, guard has not be dropped, so it's not clear to me why we would assert that it contains no lock file.
| populate: impl FnOnce(&Path) -> IoResult<()>, | ||
| ) -> IoResult<()> { | ||
| let parent = guard.path(); | ||
| assert_eq!(parent, dst.parent().expect("dst must have at least one ancestor")); |
There was a problem hiding this comment.
This seems like a strange thing to assert. If I'm reading correctly, we require that the parameters be (guard, dst) = (Guard("/foo/bar"), Path("/foo/bar/baz")). There are a couple considerations:
- Is it not sufficient that
IsAncestor(guard.dir, dst)(rather thanIsParent(guard.dir, dst)? - If we insist on
IsParent(guard.dir, dst), then would it be more ergonomic to accept(guard, dst) = (Guard("/foo/bar"), Path("baz"))and havefully_qualified_dst = guard.dir.join(dst)? I suppose that would replace this check with some kind ofIsSingleComponentPath(dst).
| let dst = Path::new("/"); | ||
| let _ = install(&mut guard, dst, |_| Ok(())); | ||
| } | ||
| } |
There was a problem hiding this comment.
If we're going to bother with multi-threaded and multi-process directory guards, we should probably test it! Could we have one or two stress tests that "fight over" the same path and ensure there's one clear winner?
| pub struct Config { | ||
| pub os: &'static str, | ||
| pub arch: &'static str, | ||
| pub url: &'static str, | ||
| pub sha256: [u8; 32], |
There was a problem hiding this comment.
Please document pub items. In particular, there are details and relationships worth calling out:
osandarchcorrespond to rust stdlibconstssha256is the expected digest for file fetched aturl
|
|
||
| /// Extracts the `.tar.zst` from `reader` and installs it at `dst`, optionally | ||
| /// validating its hash. | ||
| pub(crate) fn install( |
There was a problem hiding this comment.
What's the purpose of pub(crate) here? Is the intent that a pub wrapper will manage guard and dst (and use a Config for the rest of the parameters)? (Possibly said wrapper was going to be the commented-out associated functions on Config?)
Exactly |
Co-authored-by: Mark Dittmer <markdittmer@google.com> gherrit-pr-id: G34qjom3lz7cc6hd57tgp44t4pzlstdhj
joshlf
force-pushed
the
G34qjom3lz7cc6hd57tgp44t4pzlstdhj
branch
from
af503ff to
1f95236
Compare
3 participants
Co-authored-by: Mark Dittmer markdittmer@google.com
Latest Update: v6 — Compare vs v5
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/G34qjom3lz7cc6hd57tgp44t4pzlstdhj && git checkout -b pr-G34qjom3lz7cc6hd57tgp44t4pzlstdhj FETCH_HEADCheckout
git fetch origin refs/heads/G34qjom3lz7cc6hd57tgp44t4pzlstdhj && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/G34qjom3lz7cc6hd57tgp44t4pzlstdhj && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.