flowey: support directories in typed artifacts #1277
Conversation
Most artifacts contain an explicit list of files and are produced and consumed as such. But some artifacts, such as the output of rustdoc, are produced and consumed as an opaque directory. Today, only untyped artifacts support this. Support this in typed artifacts by also storing a marker file indicating that this is an opaque directory.
| pub trait Artifact: Serialize + DeserializeOwned { | ||
| /// If present, the artifact should consist of a tar.gz file containing the | ||
| /// contents of the artifact. This is used when the artifact contents are | ||
| /// too large for Azure DevOps to handle directly. |
There was a problem hiding this comment.
Careful about writing docs that elevate one backend over any other. This feature is - presumably - functional across all backends (gh actions, and local included). If so - having a clearer delineation in the docs between "here is what this does", and "here is why you may want to use, depending on what backend you're targeting" is good
| use std::path::Path; | ||
|
|
||
| /// Copies the contents of `src` into the directory `dst`. | ||
| pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> std::io::Result<()> { |
There was a problem hiding this comment.
If you're going to do this, my suggestion is to have these functions accept a _rt: &mut RustRuntimeServices<'_> parameter, to act as a "proof" that the function is only called at runtime.
The long-term alternative would be to invest in some more interesting sandboxing during pipeline resolution time (e.g: seccomp filters to fail pipeline compilation if the code attempts to read/write the fs at compile time), but in the meantime, having some guard rails via type-system proof would be good.
There was a problem hiding this comment.
(note that other utils in the mod you pulled this from already follow this pattern, so this might've just been an oversight on my part)
There was a problem hiding this comment.
I dunno, man, we already can't stop people from reaching for fs_err::copy for single files.
I think the fact that you just don't have the paths to copy to/from is probably enough, in practice.
There was a problem hiding this comment.
I was approaching it more from the lens of API consistency (wrt. some of the other helper APIs that surround it), but sure, these particular filesystem manipulation APIs are probably not going to be misused
|
Does this resolve all outstanding uses of the untyped artifact APIs? |
Almost. The "set of IGVM files" artifact is still untyped. That one will probably require some manual serde impls, because it wants each IGVM file to be named according to its recipe. I am still thinking about the best way to implement that. |
Most artifacts contain an explicit list of files and are produced and consumed as such. But some artifacts, such as the output of rustdoc, are produced and consumed as an opaque directory. Today, only untyped artifacts support this.
Support this in typed artifacts by also storing a marker file indicating that this is an opaque directory.