Intrinsic to turn FilesContent into Snapshot #7739
Conversation
illicitonion
force-pushed
the
dwagnerhall/filescontent-to-snapshot
branch
from
9de9a53
to
cdcfe63
Compare
| futures::future::done( | ||
| self | ||
| .files_content | ||
| .get(&file) |
There was a problem hiding this comment.
Almost commented that this is unfortunate, then noticed that it is Bytes, huzzah.
There was a problem hiding this comment.
Yay! Should we add some testing to Rust like you did with prefix stripping?
Should you be allowed to set whether a file is executable?
Yes, but I would save this for a followup change. We don't need the functionality at the moment and doesn't seem like a good use of time to properly do this, e.g. to add testing.
This also would mean changing FileContent to keep track of permissions.. Maybe instead we could add a new class FileMetadata? And you could add a rule Snapshot, [FileContent, FileMetadata]? Again, would hold off on this API design and work until we actually need it.
Should you be able to create empty directories?
How would we know what is a directory and what is a file without an extension, e.g. ./pants? Directory if it has a leading /?
Does FilesContent currently keep track of directory paths, or only files? I think this rule should reflect whatever the semantics of FilesContent is.
| @@ -496,6 +498,36 @@ impl Snapshot { | |||
| }) | |||
| .to_boxed() | |||
| } | |||
|
|
|||
| pub fn from_files_content( | |||
| store: Store, | |||
There was a problem hiding this comment.
Solely curiousity - why not use an address &Store? Also I see in the final line we move the store variable into Snapshot::from_path_stats, which means that that function becomes the new owner, right?
There was a problem hiding this comment.
Yep! Store is a tiny type that cheaply implements Clone by wrapping something that is threadsafe and reference counted.
We do a lot of this currently due to Future not making borrowing very ergonomic, but will be able to fix it soon. See http://aturon.github.io/2018/04/24/async-borrowing/
There was a problem hiding this comment.
Store is a tiny type that cheaply implements Clone
Ah cool, just like why you don't use references / borrowing for i32. Thanks!
There was a problem hiding this comment.
Store is a tiny type that cheaply implements Clone
Ah cool, just like why you don't use references / borrowing for
i32. Thanks!
There's a little more to this, actually - ideally we would use references and borrowing here, because cloning this isn't nearly as cheap as copying an i32.
One of the functions we're going to end up passing this Store to is going to end up moving it onto another thread (to use it from a Future). If we borrowed a Store here, we'd need to clone the Store at the point that we move it onto that thread (because you can't borrow across that boundary).
As a rule, if you're going to borrow a value, and then unconditionally clone it, it's better to just take an owned value in the first place, because that's being up-front about the cost of the function. That said, when async/await happens, we can probably get rid of a bunch of those clones, because we should be able to borrow across futures boundaries.
| self.assertEquals( | ||
| snapshot.directory_digest, | ||
| Digest( | ||
| fingerprint="1249edd274db4350ce7d295dc2f40768581197f0e13e74d0206a3c408f1c236f", |
There was a problem hiding this comment.
For the sake of me testing things in the future, how did you figure out this value?
There was a problem hiding this comment.
We have this handy binary called fs_util! Run:
build-support/bin/native/cargo run -p fs_util --manifest-path=src/rust/engine/Cargo.toml -- --help for some helptext :) In particular, build-support/bin/native/cargo run -p fs_util --manifest-path=src/rust/engine/Cargo.toml -- directory save --help
|
Unfortunately, the CI failure looks related to #7710. Made some progress on that one over the weekend. |
Fixes #7718
Couple of open questions: