Conversation
7e873f5 to
7408852
Compare
src/bin/pixi-pack.rs
Outdated
| } | ||
|
|
||
| fn default_output_file(platform: Platform, create_executable: bool) -> PathBuf { | ||
| fn default_output_file(platform: Platform, create_executable: bool, no_tar: bool) -> PathBuf { |
There was a problem hiding this comment.
i think we can make an enum
enum OutputMode {
Default,
CreateExecutable,
NoTar,
}and then match here.
There was a problem hiding this comment.
enum OutputMode {
Default,
CreateExecutable,
DirectoryOnly,
}How about DirectoryOnly? Discussed with @pavelzw and we found this more intuitive. (Applies to CLI flags as well.)
There was a problem hiding this comment.
It seems like "mutually exclusive flags mapped to members of an enum" isn't directly supported by clap. clap-rs/clap#2621
Should I work around this by building the enum from if (flag is set) logic?
There was a problem hiding this comment.
i would still use the flags as boolean but just convert them at the beginning to this enum then
src/pack.rs
Outdated
| fn open_output_file( | ||
| target: &Path, | ||
| ext: Option<&str>, | ||
| ) -> Result<Either<Counter<std::io::Stdout>, Counter<std::fs::File>>> { |
There was a problem hiding this comment.
let's just use an enum instead of introducing another dependency (that basically mimics what an enum in rust does)
enum Output {
Stdout(std::io::Stdout),
File(std::fs::File),
}There was a problem hiding this comment.
The Either type propagates the Write trait through to its member types -- basically I'm using it to avoid having to either (a) re-implement the Write trait via for the enum, or (b) explicitly resolving the enum at every point of use. It seemed like the least evil solution to import a crate that already had the code for (a).
Perhaps I'm missing something though? Is there a cleaner way?
There was a problem hiding this comment.
in how many places are we using that it's Write? I would say it's easier to impl Write for Output and just implement it there instead of pulling in either as a dependency. if it's only few places, i'm also fine with just doing a match directly (i.e. option b you proposed)
There was a problem hiding this comment.
About a dozen; final_executable.write_all() gets a fair amount of traffic. Re (a), at least from my vantage point, the trickier bit is that I don't know which calls on Write are performance sensitive to tar::Builder, so there's a risk that a minimal impl Write causes slowdowns; a full implementation is probably the way to go to give the compiler the best information.
I'll pull the relevant bits from either in here, then.
src/pack.rs
Outdated
| fn create_tarball(input_dir: &Path, archive_target: &Path) -> Result<()> { | ||
| let outfile = std::fs::File::create(archive_target).map_err(|e| { | ||
| fn open_output_file( | ||
| target: &Path, |
There was a problem hiding this comment.
the target can be made an enum as well
enum Target {
Stdout,
File(PathBuf)
}There was a problem hiding this comment.
Do you mean for this to be a library API change in PackOptions? Given that the PathBuf->Writer mapping is resolved in only two locations in pack, it's true we could do a PathBuf -> Target -> Write approach with the "stdout" special case identified early, but I'm not sure if the extra formalism will buy much inside pack.rs vs doing the text analysis later in open_output_file.
| #[cfg(not(target_os = "windows"))] | ||
| use std::os::unix::fs::PermissionsExt as _; | ||
|
|
||
| use countio::Counter; |
There was a problem hiding this comment.
This is used to count output bytes when written since we can't check the file size of stdout after packing is completed. The intent was to provide a consistent internal interface to minimize the number of if special cases.
|
Sorry to bother, what am I missing to move this PR forward? Thanks! |
|
sorry for the delay, this pr slipped out of my inbox. i'll look into it next week! |
Motivation
Add some additional options for pack output/unpack input, as discussed in #245.
Changes
Notes