Intermediate source <(mcfly init *sh) during initialisation. Fixes #254, #219, #239

[tombh]

As requested in #254.

I don't know anything about Fish, but I assume this issue didn't affect it?

[cantino]

Thanks @tombh! Merged. Are you able to check if this has fixed the bug for you?

[cantino]

Actually, I'm getting panics when I run bash now and I wasn't before.

~ RUST_BACKTRACE=1 bash
thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1187:9
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: std::io::stdio::_print
   3: mcfly::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

This is launching bash from zsh, my default shell.

[tombh]

I'm primarily a zsh user too, so I may have overlooked something for bash. I just assumed they were similar enough that the both needed the same solution. But I might be wrong. Anyway, I went as far as testing like this from with a bash shell:

[tombh@tombox mcfly]$ source <(cargo run -- init bash --print_full_init)
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/mcfly init bash --print_full_init`
[tombh@tombox mcfly]$

And CTRL+r worked fine for me. How are you testing?

[cantino]

I built and installed it locally: cargo build --release && cp ./target/release/mcfly ~/bin/
My .zshrc includes eval "$(mcfly init zsh)" and my .bashrc has eval "$(mcfly init bash)".
And then from zsh, I ran bash.

[tombh]

I just repeated that exactly, and can't reproduce the bug. Can you paste the contents of your .bashrc? And do you think there's anything unusual in your env?

[cantino]

That's very odd. I tried adding a newline to your outputs too. Are you on a Mac? Maybe related to this? nix-community/nix-index#69 Backing up a second, what was the reasoning around not just changing the init scripts to be wrapped in a conditional if `if ! [[ "$__MCFLY_LOADED" == "loaded" ]]; then`?

[tombh]

I'm on Arch (by the way haha). What are the newlines for? To replicate the difference between println! and writeln! as mentioned in that nix-index bug?

what was the reasoning around not just changing the init scripts to be wrapped in a conditional if if ! [[ "$__MCFLY_LOADED" == "loaded" ]]; then?

That's certainly one approach. But the advantage of using process substitution, namely source <(...), is that you never have to worry ever again about what gets put in the init scripts, because there's no possible way they can contaminate the user's own .zshrc or .bashrc. That just seems like the sanest, safest and most respectful approach.

[tombh]

Oh, are you on Mac? Because there is indeed some issues mentioned with this kind of init in the Starship code, have a look: https://github.com/starship/starship/blob/17453929091b1d9463f33e0b291436c9213c54d2/src/init/mod.rs#L104

[cantino]

Oh interesting, that’s probably it. I guess we should just do the conditional around the scripts? Seems easiest.

[tombh]

If Starship can do it, so can we! Look, this is what they use for lower Bash versions:
source /dev/stdin <<<"$(mcfly init bash --print-full-init)"

Does it help you? Are you Mac?

[cantino]

I am on a Mac, yes. GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin20)

No, that isn't working for me. Running source /dev/stdin <<<"$(mcfly init bash --print_full_init)" returns no errors, but also doesn't register mcfly with the shell.

I'm leaning toward just conditionalizing the scripts for now.

[tombh]

There's an interesting comment at the top of that Starship file:

/*
We use a two-phase init here: the first phase gives a simple command to the
shell. This command evaluates a more complicated script using `source` and
process substitution.

Directly using `eval` on a shell script causes it to be evaluated in
a single line, which sucks because things like comments will comment out the
rest of the script, and you have to spam semicolons everywhere. By using
source and process substitutions, we make it possible to comment and debug
the init scripts.

In the future, this may be changed to just directly evaluating the initscript
using whatever mechanism is available in the host shell--this two-phase solution
has been developed as a compatibility measure with `eval $(starship init X)`
*/

So wrapping the init scripts in a big conditional won't solve the bigger issue. Directly eval()'ing is just asking for untold trouble, whether now or later when you or any future contributors are unaware of the various restrictions they need to ensure.

I think we're close. Starship is Rust, and they've already successfully solved all these pieces. We've found the cause of your bug, and so just need to get over that final hurdle. If you don't have time to figure it out, then I think the better compromise is to only fallback for Bash <v4.1 users, something like this:

local major="${{BASH_VERSINFO[0]}}"
local minor="${{BASH_VERSINFO[1]}}"
if ((major > 4)) || {{ ((major == 4)) && ((minor >= 1)); }}; then
	source <(mcfly init bash --print_full_init)
else
	mcfly init bash --print_full_init
fi

[cantino]

Starship's ${{BASH_VERSINFO[0]}} stuff isn't working for me, unfortunately. I just created #259 as a temporary solution. Mind giving it a look? I unfortunately don't have much time to put into mcfly anymore. I should post looking for co-maintainers.

[tombh]

I think you misunderstood my advice in that previous comment. The ${BASH_VERSINFO[0]} isn't to fix your issue, it's to disable the fix in this PR just for your version of Bash. That way everybody else gets the safer source <(mcfly init) behaviour. Bash v4.1 is 12 years old after all 🫤

Also, I didn't realise that Rust is escaping brackets with another bracket {{, so that wouldn't have worked as is anyway. I've pushed a commit that actually implements it correctly. And there's no reason why you can't have both this PR and your #259 approach. At least yours will help whilst the ${BASH_VERSINFO[0]} conditional is serving the bad behaved version.

[tombh]

Oh, because this PR is closed the commit isn't automatically showing up. It's here: tombh@54984f8 I'll have to make another PR for it, if you want it.

[cantino]

Right, but the ${BASH_VERSINFO[0]} stuff simply isn't working when I try it.

[tombh]

Oh I made a typo, it should be ${BASH_VERSIONFO[0]} not ${BASH_VERSINFO[0]}

[cantino]

If you want to remake the PR with the correct code, I'm happy to give it another try locally.