adex tool was introduced

[rozhkovdmitrii]

DO NOT FORGET TO SQUASH COMMITTS ON MERGE

adex command line utility was introduced that supplies

Commands: init, start, stop, status

[rozhkovdmitrii]

@shamardy @ozkanonur @borngraced
Guys, I'm happy to introduce the version 0.1.0

[onur-ozkan]

Great implementation :)

I have a few suggestions/notes from my first review iteration:

[shamardy]

Great work!
I have a few comments/questions for my first review :)

[borngraced]

Great work! I have couple questions and some questions

[DeckerSU]

Hm, I see the difference in original Cargo.lock, of coarse it should be solved

Yes, I mentioned exactly these differences, nothing else. Sorry, maybe I wasn't so clear.

[rozhkovdmitrii]

@DeckerSU
rolled back

[DeckerSU]

Seems you are missed / did not pay attention to few recommendations in #1729 (review) , related to the "Matching with Fork::Parent should not be considered as a successful launch".

Let's imagine the following case:

1. mm2 binary exists, but we accidentally did chmod -x mm2 on it.
2. After ./adex-cli start user will see the following message:

Successfully started: "mm2", forked pid: 579956
thread 'main' panicked at 'Failed to execute process: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', src/scenarios/mm2_proc_mng.rs:117:30
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Errors and reports about a successful start at the same time mislead.

In all other aspects it looks good to me.

[onur-ozkan]

You just have avoid writing Successfully started: "mm2", forked pid: $id when mm2 is not an executable. @rozhkovdmitrii

[rozhkovdmitrii]

You just have avoid writing Successfully started: "mm2", forked pid: $id when mm2 is not an executable. @rozhkovdmitrii

Sure, I know, thank you )
The matter is right after fork the program releases terminal even if that is the main process and I want to save the output straight

[rozhkovdmitrii]

Seems you are missed / did not pay attention to few recommendations in #1729 (review) , related to the "Matching with Fork::Parent should not be considered as a successful launch".

Let's imagine the following case:

1. `mm2` binary exists, but we accidentally did `chmod -x mm2` on it.

2. After `./adex-cli start` user will see the following message:

...

@DeckerSU

done

cc: @ozkanonur

P.S: if I used something like this the output would make the output messy

    if let Ok(Fork::Child) = daemon(true, true) {
        command.output().expect("Failed to start: {program:?}");
        exit(1);
    };
    info!("Successfully started: {program:?}");

[rozhkovdmitrii]

Found more deliberate decision using fork and setsid together instead of daemon. Works well, no ugly things in source code
to: @DeckerSU
cc: @ozkanonur

Diff from last review

[DeckerSU]

The look of it has improved, but there is still a potential for a report of a “successful start” even if the outcome was a failure:

1. Let's imagine user already have started instance of mm2 from some other folder.
2. In folder with adex-cli he have mm2 file without rights on execution (chmod -x mm2).
3. After ./adex-cli start user will see the message Successfully started: "mm2", but in real - nothing is started.

Also, can you explain why you chose to use fork() to launch the mm2, rather than simple spawn new process and "detach"? What was your thought process behind this decision? Just curious to know.

use std::process::Command;

let child_process = Command::new("my_child_process_binary")
    .args(&["arg1", "arg2"])
    .spawn()
    .expect("Failed to start child process");

// detach the child process
let pid = child_process.id();
std::mem::forget(child_process);

println!("Started child process with PID {}", pid);

[rozhkovdmitrii]

...
std::mem::forget(child_process);

println!("Started child process with PID {}", pid);

@DeckerSU, to tell you the truth, using std::mem::forget was not obvious. Thank you for great decision ), this one is the best of coarse!

[shamardy]

Thanks for the fixes @rozhkovdmitrii! Changes since my last review looks good.

[rozhkovdmitrii]

If we want CLI to work with remote nodes, then yes it should work over HTTP

Should be HTTPS, else the rpc password is sent in cleartext over internet. There is an issue about this #1324 and i think it's an easy task.

#1873