Resolves #53 #111

thedanvail · 2022-06-03T14:53:18Z

Adds a more graceful exit when the directory does not exist.

If the desired behavior is any different (for example, printing the absolute path of the nonexistent directory), I can update with the absolute path.

Adds a more graceful exit when the directory does not exist.

aviramha

Thanks for your contribution!
I have left few notes in the code.
Feel free to ask for clarifications/guidance if needed.
Also, you are more than welcome to consult with us in our Discord server.

CHANGELOG.md

aviramha · 2022-06-03T14:57:56Z

mirrord-cli/src/main.rs

-    let mut file = File::create(file_path.clone()).unwrap();
+    let mut file = match File::create(&file_path) {
+        Ok(file) => file,
+        Err(_) => {


I think it'd be better to bind the error here and add it to the message (as it can fail for many reasons, one reason can be directory not existing)

aviramha · 2022-06-03T14:58:49Z

mirrord-cli/src/main.rs

+        Err(_) => {
+            file_path.pop();
+            eprintln!("Directiory \"{}\" does not exist.", file_path.display());
+            std::process::exit(1);


It'd be better if we changed the signature of the function to Result<String> then just return the err here.

aviramha · 2022-06-03T14:59:22Z

mirrord-cli/src/main.rs

+        Ok(file) => file,
+        Err(_) => {
+            file_path.pop();
+            eprintln!("Directiory \"{}\" does not exist.", file_path.display());


I would suggest to use trace library macros (error).

@aviramha

Decided to utilize `anyhow` at the recommendation of @aviramha - went with the `with_context` option as that seemed the most elegant solution.

thedanvail · 2022-06-03T16:28:41Z

@aviramha I tried to take all of your comments into consideration as I updated using anyhow to handle an invalid path.
Additional meta-information:

This is my first open source PR, so I apologize if I'm not quite following best practices
I found this issue through the This Week In Rust call for collaboration - just so you know!

aviramha · 2022-06-03T16:31:41Z

@thedanvail

Great! Thanks for choosing us to be your first contribution. Beware, OSS contributing can be addictive!
Cool! Thanks for letting us know :)

aviramha

The context can be great, but a few more tweaks are needed to merge this.
Thanks!

aviramha · 2022-06-03T16:37:01Z

mirrord-cli/src/main.rs

@@ -117,7 +120,7 @@ fn exec(args: &ExecArgs) -> Result<()> {
    if args.accept_invalid_certificates {
        std::env::set_var("MIRRORD_ACCEPT_INVALID_CERTIFICATES", "true");
    }
-    let library_path = extract_library(None);
+    let library_path = extract_library(None).unwrap();


you can use ? here instead of unwrap

aviramha · 2022-06-03T16:37:16Z

mirrord-cli/src/main.rs

@@ -138,7 +141,7 @@ fn main() {
    match cli.commands {
        Commands::Exec(args) => exec(&args).unwrap(),
        Commands::Extract { path } => {
-            extract_library(Some(path));
+            extract_library(Some(path)).unwrap();


Change fn main to return Result<()> then you can use ? here

If I change the main function signature to return Result<()>, then we'd have to remove the unwrap()s/? to return the Result - or we can leave in the unwrap()s/? and leave out Result from the return type - which behavior would be preferred?

? is a syntatic sugar that returns when a Result/Option has Err/None else it returns the inner value.
So the change would to add ? and change the return type to Result
https://doc.rust-lang.org/rust-by-example/std/result/question_mark.html

I understand that, but the issue is, if I run cargo check with the following code

fn main() -> Result<()> { registry() .with(fmt::layer()) .with(EnvFilter::from_default_env()) .init(); prompt_outdated_version(); let cli = Cli::parse(); match cli.commands { Commands::Exec(args) => exec(&args)?, Commands::Extract { path } => { extract_library(Some(path))?; } } }

I receive the following error:

Sorry but the image isn't available.. you can upload directly to GitHub by pasting the image here.
Considering the snippet, I think I know what the issue is. You can just drop the ? in both arms and it'd work because both exec and extract_library return Result, so no need to unwrap it and you can just return their return value.

Added in your changes - since extract_library returns a Result<String> instead of Result<()>, I just return a normal Ok(()). I think that should cover everything!

aviramha

Few small tweaks left :)
Sorry for being hard, but we want to make sure everything is great.
Also please fix conflicts with the main branch so we GH can run our automated tests.

aviramha · 2022-06-05T20:23:18Z

CHANGELOG.md

@@ -6,6 +6,8 @@ Previous versions had CHANGELOG per component, we decided to combine all reposit
 Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how to structure this file.

 ## [Unreleased]
+### Changed
+- Added a graceful exit for system paths that do not exist.


Reword - "Added graceful exit for library extraction logic in case of error"

aviramha · 2022-06-05T20:24:09Z

mirrord-cli/src/main.rs

    let library_file = env!("CARGO_CDYLIB_FILE_MIRRORD_LAYER");
    let library_path = std::path::Path::new(library_file);
    let file_name = library_path.components().last().unwrap();
    let file_path = match dest_dir {
        Some(dest_dir) => std::path::Path::new(&dest_dir).join(file_name),
        None => temp_dir().as_path().join(file_name),
    };
-    let mut file = File::create(file_path.clone()).unwrap();
+    let mut file = File::create(&file_path)


We can't know for sure the reason it failed is due to path not existing, so I'd just add using context the file path ("Path .. creation failed")

aviramha · 2022-06-05T20:25:05Z

mirrord-cli/src/main.rs

@@ -136,9 +139,10 @@ fn main() {

    let cli = Cli::parse();
    match cli.commands {
-        Commands::Exec(args) => exec(&args).unwrap(),
+        Commands::Exec(args) => exec(&args),
        Commands::Extract { path } => {
            extract_library(Some(path));


I think you would have warning here if you wont add ?

Nay - the function returns Result<()>, and because we added that to the main() -> Result<()> as well, this compiles as is. If I add the ? operator - Commands::Exec(args) => exec(&args)?, - cargo check returns the following:

error[E0308]: `?` operator has incompatible types --> mirrord-cli/src/main.rs:142:33 | 142 | Commands::Exec(args) => exec(&args)?, | ^^^^^^^^^^^^ expected enum `Result`, found `()` | = note: `?` operator cannot convert from `()` to `Result<(), anyhow::Error>` = note: expected enum `Result<(), anyhow::Error>` found unit type `()` help: try removing this `?` | 142 - Commands::Exec(args) => exec(&args)?, 142 + Commands::Exec(args) => exec(&args), | help: try wrapping the expression in `Ok` | 142 | Commands::Exec(args) => Ok(exec(&args)?), | +++ + For more information about this error, try `rustc --explain E0308`. error: could not compile `mirrord` due to previous error

I think it'd be best to have ? in the end of all arm branches (on exec + Extract) then outside the match branch (in the end of function) just to have Ok(()) which means that if it got there, we're all good.

…extual failure

Refactor CI (#107)

thedanvail · 2022-06-05T21:11:34Z

Few small tweaks left :) Sorry for being hard, but we want to make sure everything is great. Also please fix conflicts with the main branch so we GH can run our automated tests.

No, please be as hard as you'd like! I'm trying to take as many notes as I can to get better at Rust and so I can be better at my next OSS PR. I sincerely appreciate your patience with me during this process!

I believe I have updated to reflect your suggestions and updated to fix the merge conflicts. I'm currently between a couple flights, so I didn't have time to check the status before the plane took off, but should be good now!

aviramha · 2022-06-06T05:14:01Z

Few small tweaks left :) Sorry for being hard, but we want to make sure everything is great. Also please fix conflicts with the main branch so we GH can run our automated tests.

No, please be as hard as you'd like! I'm trying to take as many notes as I can to get better at Rust and so I can be better at my next OSS PR. I sincerely appreciate your patience with me during this process!

I believe I have updated to reflect your suggestions and updated to fix the merge conflicts. I'm currently between a couple flights, so I didn't have time to check the status before the plane took off, but should be good now!

Awesome, just see my last comment in the conversation about the ?.

Merge

merge

thedanvail · 2022-06-06T19:34:03Z

Done and done!

thedanvail · 2022-06-06T22:00:53Z

(Just updated with cargo fmt)

thedanvail · 2022-06-07T05:19:26Z

I looked at the last CI run, and it looks like the cargo clippy issues are in mirrord-layer, which I did not touch in this PR. Let me know if I should do anything further.

aviramha · 2022-06-07T08:10:30Z

Yes you're right. We're not binding clippy to a specific version so such cases can happen :)
Thanks for the contribution!

Resolves #53

522bfe2

Adds a more graceful exit when the directory does not exist.

aviramha requested changes Jun 3, 2022

View reviewed changes

Refactored for comments

9be1bff

Decided to utilize `anyhow` at the recommendation of @aviramha - went with the `with_context` option as that seemed the most elegant solution.

aviramha requested changes Jun 3, 2022

View reviewed changes

removed unwraps, added Result to main return time

90a4747

aviramha requested changes Jun 5, 2022

View reviewed changes

thedanvail and others added 4 commits June 5, 2022 16:05

Clarified CHANGELOG.md, reworded extract_library's file creation cont…

9f07ce2

…extual failure

Merge pull request #1 from metalbear-co/main

1278d00

Refactor CI (#107)

Adding missed changelog

14eace0

Merge branch 'main' into 53-graceful-exit

89e247f

thedanvail and others added 3 commits June 6, 2022 12:27

Merge pull request #2 from metalbear-co/main

de3bed2

Merge

Merge pull request #3 from thedanvail/main

5423d27

merge

Ending all arms with '?'

6a11a88

Formatting

ca66ffb

aviramha merged commit 604ad8f into metalbear-co:main Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolves #53 #111

Resolves #53 #111

thedanvail commented Jun 3, 2022

aviramha left a comment

aviramha Jun 3, 2022

aviramha Jun 3, 2022

aviramha Jun 3, 2022

thedanvail commented Jun 3, 2022

aviramha commented Jun 3, 2022

aviramha left a comment

aviramha Jun 3, 2022

aviramha Jun 3, 2022

thedanvail Jun 3, 2022

aviramha Jun 3, 2022

thedanvail Jun 3, 2022

aviramha Jun 4, 2022

thedanvail Jun 5, 2022

aviramha left a comment

aviramha Jun 5, 2022

aviramha Jun 5, 2022

aviramha Jun 5, 2022

thedanvail Jun 5, 2022 •

edited

aviramha Jun 6, 2022

thedanvail commented Jun 5, 2022

aviramha commented Jun 6, 2022

thedanvail commented Jun 6, 2022

thedanvail commented Jun 6, 2022

thedanvail commented Jun 7, 2022

aviramha commented Jun 7, 2022

Resolves #53 #111

Resolves #53 #111

Conversation

thedanvail commented Jun 3, 2022

aviramha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedanvail commented Jun 3, 2022

aviramha commented Jun 3, 2022

aviramha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aviramha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedanvail Jun 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedanvail commented Jun 5, 2022

aviramha commented Jun 6, 2022

thedanvail commented Jun 6, 2022

thedanvail commented Jun 6, 2022

thedanvail commented Jun 7, 2022

aviramha commented Jun 7, 2022

thedanvail Jun 5, 2022 •

edited