Allow setting Fork behavior in ENV variable

[9mm]

Is your feature request related to a problem? Please describe.

#2147

Will assist with the dock version of the application until default forking behavior is decided.

Describe the solution you'd like

Add NEOVIDE_FORK=0/1

[9mm]

@fredizzimo so I started doing this but one confusing bit of this is... i thought forking was the default, however in main.rs it has .arg("--no-fork")?

[fredizzimo]

That’s actually the arguments it passes to the forked process, which should not fork itself again…

[9mm]

I started doing this thinking it would be simple but it's actually quite confusing given that app development isn't where my specialty lies 😅

This was my current version I was working on...

    /// Instead of spawning a child process and leaking it, be "blocking" and let the shell persist
    /// as parent process
    #[arg(long = "no-fork", env = "NEOVIDE_FORK", action = ArgAction::SetFalse, default_value = "1", value_parser = FalseyValueParser::new())]
    pub no_fork: bool,

But then I realized after that I'm not exactly sure how the Neovide.app behavior relates to the binary behavior, given that that's actually what we're trying to fix, and at least on OSX they behave very differently. That's further compounded in that I don't actually know how to test the .app version easily given that it comes from the CI build process.

I don't expect other ppl to do this for me just wanted to write some notes. I'll attempt to come back to this later I think when I have more brainpower

[fredizzimo]

For the app creation, maybe you need to install the bundle tool?

neovide/.github/workflows/build.yml

Lines 121 to 123 in ce0174a

	rustup target add x86_64-apple-darwin
	rustup target add aarch64-apple-darwin
	if ! which cargo-bundle; then cargo install cargo-bundle; fi

[9mm]

Ok, this may be a dumb question but this is definitely like a "boolean hell"...

    /// Instead of spawning a child process and leaking it, be "blocking" and let the shell persist
    /// as parent process
    #[arg(long = "no-fork", env = "NEOVIDE_FORK", default_value = "1", value_parser = FalseyValueParser::new())]
    pub no_fork: bool,

This is the closest I can get, but its actually inverted behavior because:

NEOVIDE_FORK=1 cargo run --release will not fork while NEOVIDE_FORK=0 cargo run --release forks

Aside from this, it works perfectly. I thought this would be simple at first given it is just booleans, but after messing with this for some time it's almost like its not possible unless the env variable is renamed NO_FORK

Am i missing something obvious?

[fredizzimo]

Take a look at how vsync is done. The clap libarary that we are using does not support --no- options very well, so it has to be done manually.

But fork should work the exact same way, both defaults to true, and supports turning it off.

[9mm]

Ah i see, ok I will try that. I'm literally sitting here laughing out loud because I didn't imagine it can get so crazy with boolean values. "This doesnt seem so bad" <- famous last words

[fredizzimo]

That has happened to me many times, especially when you use things like no_fork=true instead of fork=false and start to do test like !no_fork

Edit and some languages that needs a double negation to convert to a boolean !!no_fork...

[9mm]

ok.. that was a very humbling experience. I'm scared to put my code here as I feel absolutely stupid after that, and it might be embarrasing, but so far this behavior seems to work... I would have guessed so many odd "negations" were not required, but this was the only way I could get it to work.

I also noticed errors saying --no-fork can't be sent twice because of main.rs. I did leave that unmodified however, and the error no longer appears with this method. I can't tell if thats because:

1. this is the correct way to do it
2. this is the incorrect way to do it, and it's only working as an unintended side-effect

    /// Instead of spawning a child process and leaking it, be "blocking" and let the shell persist
    /// as parent process
    #[arg(long = "fork", env = "NEOVIDE_FORK", action = ArgAction::SetTrue, default_value = "1", value_parser = FalseyValueParser::new())]
    _fork: bool,

    /// Instead of spawning a child process and leaking it, be "blocking" and let the shell persist
    /// as parent process
    #[arg(long = "no-fork", action = ArgAction::SetTrue, value_parser = FalseyValueParser::new())]
    pub no_fork: bool,

And down in the handle_command_line_arguments function:

    if !cmdline._fork {
        cmdline.no_fork = true;
    }

    if cmdline._no_srgb {
        cmdline.srgb = false;
    }

    if cmdline._no_vsync {
        cmdline.vsync = false;
    }

3 instances I tested:

1. Properly forks:

➜  neovide git:(nofork-env) ✗ cargo run --release
    Finished release [optimized] target(s) in 0.11s
     Running `target/release/neovide`
➜  neovide git:(nofork-env) ✗

2. Properly doesn't fork

➜  neovide git:(nofork-env) ✗ NEOVIDE_FORK=0 cargo run --release
    Finished release [optimized] target(s) in 0.11s
     Running `target/release/neovide`
[flexi_logger][ERRCODE::Time] flexi_logger has to work with UTC rather than with local time, caused by IndeterminateOffset
    See https://docs.rs/flexi_logger/latest/flexi_logger/error_info/index.html#time

3. Properly forks

➜  neovide git:(nofork-env) ✗ NEOVIDE_FORK=1 cargo run --release
    Finished release [optimized] target(s) in 0.11s
     Running `target/release/neovide`
➜  neovide git:(nofork-env) ✗

[fredizzimo]

I would change the main argument from _fork to fork, and no_fork to _no_fork. It will require some more modifcations to the rest of the code, but it becomes more natural that way.

[9mm]

OK awesome... im glad to see this is the right track then 😅

[9mm]

@MultisampledNight I dont know if you have a procedure for closing things but I think this one can go too

[MultisampledNight]

You're right! Yeehaw! Closed by #2189!