muvm-guest: Find actual path for muvm-server before trying to execute it

[sbrivio-rh]

Otherwise this depends on where you run things from. In my case, I
would get:

  $ ./target/debug/muvm -- /bin/false
  Error: Failed to execute `muvm-server` as child process

  Caused by:
      No such file or directory (os error 2)

Signed-off-by: Stefano Brivio [email protected]

[teohhanhui]

We should expect muvm-server to be in PATH, no? Actually I think we should remove the special handling of find_muvm_exec everywhere.

It's only coincidentally fixing the problem for you because you don't have muvm-server in PATH, and find_muvm_exec falls back to searching in the directory of std::env::current_exe...

So it doesn't actually help with using the dev version of the binaries, and is otherwise unnecessary in release with correct packaging.

[sbrivio-rh]

We should expect muvm-server to be in PATH, no?

That's not really practical for development, right? I mean, every time I build I should do the equivalent of make install (as root) to try out what I'm working on?

So it doesn't actually help with using the dev version of the binaries

What would help?

[teohhanhui]

That's not really practical for development, right?

PATH="$PWD/target/debug:$PATH"

[sbrivio-rh]

and find_muvm_exec falls back to searching in the directory of std::env::current_exe...

By the way, that's actually good enough for me. I don't have anything installed, and that way it runs from my current build directory.

[sbrivio-rh]

That's not really practical for development, right?

PATH="$PWD/target/debug:$PATH"

That looks rather dangerous. While POSIX doesn't really restrict the usage of PATH in any particular way, it's not supposed to include temporary build directories like that.

By the way, that's actually good enough for me. I don't have anything installed, and that way it runs from my current build directory.

You might smile about it but... muvm is not widely packaged (yet).

[teohhanhui]

@sbrivio-rh I think there's some miscommunication there. I didn't mean to add that to PATH in .bashrc or its equivalent. But only for the current terminal where you need to run the dev version of muvm from?

[teohhanhui]

But personally I wouldn't mind at all if we want to add something to search in the directory of std::env::current_exe first, only for dev builds... Presumably that's always the correct thing to do.

[sbrivio-rh]

@sbrivio-rh I think there's some miscommunication there. I didn't mean to add that to PATH in .bashrc or its equivalent. But only for the current terminal where you need to run the dev version of muvm from?

I understand, but that's one more step (and I develop things in several different environments), plus somebody might be tempted to add it permanently because of the nuisance.

But personally I wouldn't mind at all if we want to add something to search in the directory of std::env::current_exe first, only for dev builds... Presumably that's always the correct thing to do.

Right, I can give it a try... any hint as to where I should start?

[teohhanhui]

any hint as to where I should start?

I think actually we could just change find_muvm_exec to do that.

I haven't looked at the git blame, but I suspect the current behaviour is my mistake... Haha.

[sbrivio-rh]

Right, yes, that comes from the original implementation and commit 0cdc3e1 ("Rewrite it in Rust").

Any #[cfg(...)] thing you have in mind? Otherwise I'll try to find out one...

[teohhanhui]

#[cfg(debug_assertions)]

AFAICT there isn't any other that we could use.

[teohhanhui]

@sbrivio-rh PTAL

[sbrivio-rh]

Looks good to me! Testing in a moment...

[sbrivio-rh]

It works for me, thanks!

Reviewed-by: Stefano Brivio <[email protected]>

LGTM

🥬

...or whatever you use. :)

[slp]

LGTM.