Skip to content
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

fix(eas): allow path/to/output & add clean-up #115

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ZeroEkkusu
Copy link

@ZeroEkkusu ZeroEkkusu commented Jan 27, 2023

Gm! ETK is an amazing toolkit and I wanted to contribute.

This is my fist time working with Rust - feel free to make any changes and merge/close the PR, because I'll be AFK.

Motivation

  1. eas errors if a directory on the path to the output file doesn't exist.
  2. An empty output file is created if eas fails. In case of an existing file, that one will be overwritten.

Solution

  1. Create any directories specified in the out path that don't exist.
  2. Do not create an output file or overwrite the existing file if eas fails.

Thanks!

Comment on lines +69 to +72
match std::fs::write(&opt.out.unwrap(), &out_file_content) {
Ok(_) => (),
Err(e) => panic!("couldn't restore existing file.\n{}", e),
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to only modify the output if the compilation succeeds, but leave the file untouched otherwise?

This particular approach will fail to restore the file if, say, there's a panic.

What would you think of using something like tempfile to make a temporary file in the same directory as the intended output, then moving the file into the intended output?

So to write to a file a/b/test.foo, you'd do:

  1. Create a/b/test.foo.tmp using tempfile.
  2. ingest_file(...).
  3. std::fs::rename from a/b/test.foo.tmp to a/b/test.foo.

Copy link
Author

@ZeroEkkusu ZeroEkkusu Jan 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @SamWilsn. Yes, that was the idea - don’t create a file or overwrite the existing one with the same name if eas fails.

tempfile sounds like a good option.

Sorry I won’t be able to make the changes - I won’t have a computer with me for a while.

I’d recommend the following approach:

  1. Create a temporary file in the current directory
  2. Ingest
  3. If successful, create directories on the out path that don’t exist, and move + rename the file

This way, neither new directories nor an output file will be created if the compilation fails, which is cleaner.

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants