Add init-command option

[nrabulinski]

Closes #37, supersedes #38 while being more general

Allows to execute a command before the guest server starts.
Useful on distros like NixOS where we need additional mounts.

E.g. on NixOS this means I can use muvm from main with this patch by running muvm --init-command=init.sh $SHELL with init.sh being

#!/bin/sh

ln -s /run/muvm-host/run/opengl-driver /run/opengl-driver
ln -s /run/muvm-host/run/current-system /run/current-system

whereas currently it's not as straightforward

[teohhanhui]

I remember there was a mention somewhere of plans of having an init script automatically run...

[nrabulinski]

I was thinking whether you should be able to specify this option multiple times and came to the conclusion that if your setup has more than one step, you can just wrap it in a shell script as I did.
The issue there could be that if your distro wraps muvm with a default init script (like NixOS certainly would) then it's unexpected that passing in this argument again will either be ignored or overwrite the previous value. Thoughts?

Also, I was thinking about adding a short option and came up with -x (a'la gdb), but I'd also like some opinions here.

[nrabulinski]

I remember there was a mention somewhere of plans of having an init script automatically run...

Yeah, in the PR I mention 😄

[teohhanhui]

The issue there could be that if your distro wraps muvm with a default init script (like NixOS certainly would) then it's unexpected that passing in this argument again will either be ignored or overwrite the previous value.

The last one should win?

EDIT: Or actually, what do you think about having an entrypoint script like Docker?

[nrabulinski]

That's the current behavior. I'm just thinking out loud whether that's the best UX... I don't mind how it is right now though, just throwing ideas around

[nrabulinski]

an entrypoint script like Docker

WDYM? I'm probably not well versed in Docker enough but from my understanding ENTRYPOINT is the command that's always executed, then comes CMD which is the "default" which you can then override

[teohhanhui]

Yeah, sorry, just thinking out loud. But I see now why that doesn't work as an init script... 😅

But do you think it'd make sense to have this be something that's automatically used by default, a la ~/.bashrc? Perhaps ~/.muvm_init?

[nrabulinski]

I think it does, though I've considered maybe instead of ~/.muvm_init to have $XDG_CONFIG_HOME/muvm/config.json which would hold the path to the default command(s?), on top of allowing to, e.g. change the emulator precedence (to overwrite the currently hardcoded one of fex > box64 > none) and potentially let you define custom ones.

Definitely wouldn't be a part of this PR in particular, but would open the door to such options, if users would want them

[teohhanhui]

The name --init-command could be confused with --init in Docker, i.e. the process that runs as PID 1, which in libkrun is /init.krun

[nrabulinski]

What about something like --execute-pre/-x?

[nrabulinski]

Semi-related: Why is the workdir being set to $HOME? Would it be fine to default to cwd instead? That'd simplify the implementation of execute-pre (name pending) since you could then either use a command from PATH or a command relative to cwd and just let Command::spawn handle figure it out. I also don't see muvm or libkrun using workdir for anything in particular.

[teohhanhui]

Why is the workdir being set to $HOME? Would it be fine to default to cwd instead?

Oh yeah, I guess that's the reason why it doesn't stay in the current directory when you do e.g. muvm bash lol...

since you could then either use a command from PATH or a command relative to cwd and just let Command::spawn handle figure it out.

If the cwd is not in PATH, the command would still have to use relative paths if it wants to run something relative to cwd.

[nrabulinski]

If the cwd is not in PATH, the command would still have to use relative paths if it wants to run something relative to cwd.

Yeah I know, I realized yesterday I implemented it wrong with Path::canonicalize 😄. But currently if we wanted to allow --execute-pre=./init.sh we'd need to first resolve execute-pre ourselves based on whether it contains a / or not, instead of delegating it to Command::spawn which already does that.

[nrabulinski]

Oh yeah, I guess that's the reason why it doesn't stay in the current directory when you do e.g. muvm bash lol...

Yeah... this is resolved by #157 anyway, but still workdir being $HOME gets in the way here

[nrabulinski]

I changed the option from init-command to execute-pre and made it so that it can be specified multiple times and the commands will be run in order.
I also added commits, first of which removes using $USER for looking up the user and instead looks it up by uid, and the second which removes looking up the user altogether and sets krun's workdir to cwd. I decided to split them like this so the commits are independent and can all be reviewed per-commit, but lmk if I'm too granular 😅

[WhatAmISupposedToPutHere]

Setting workdir to cwd can cause an issue where the first command is started from a removable mount, and it will then hold on to it and prevent unmounting, even if no process inside the vm is using that mount anymore.

[nrabulinski]

Setting workdir to cwd can cause an issue where the first command is started from a removable mount, and it will then hold on to it and prevent unmounting, even if no process inside the vm is using that mount anymore.

Would you prefer me implement resolving the command or pass in cwd as part of GuestConfiguration and set current_dir for every init_command?

[WhatAmISupposedToPutHere]

The second one feels more robust, imo

[nrabulinski]

Alternatively we could also only allow absolute paths and commands which are in PATH. I'm fine with any of those options

[nrabulinski]

I'll revert the cwd commit then, but I think leaving in the no username commit should still be fine? Looking up the user by reading $USER is iffy anyway.

[nrabulinski]

Would you prefer the config file work to be started as part of this PR or should I do that in a follow up once this PR lands? Also despite the UX improvements #157 brings I'd like this PR to get in sooner since this resolves the biggest blocker when it comes to muvm on NixOS and makes it possible to properly package it :)

[rowanG077]

Is this design in some way not accepted? It's been open without feedback for a few weeks now. If a maintainer didn't get to it yet that's understandable. Just wanting to know the state that's all.

[slp]

LGTM. @teohhanhui @WhatAmISupposedToPutHere WDYT?

[WhatAmISupposedToPutHere]

Should probably also add a way to specify it via configs in /etc or whatever, but all my issues were resolved.

[slp]

Ack, let's merge this one. @nrabulinski could you please do a rebase?

[nrabulinski]

Ack, let's merge this one. @nrabulinski could you please do a rebase?

Will do, I also started working on the muvm config. Do you want that to be a part of this PR too? Or should I make it part of a subsequent one

[nrabulinski]

Rebased and added reading ~/.config/muvm/config.json with fallback to /etc/muvm/config.json. Currently the config file only allows to specify execute_pre which will be prepended to all muvm calls, but I want to extend that to all the other options (EDIT: In future PRs, not this one). I haven't updated the README or anything to mention the config file yet though

[WhatAmISupposedToPutHere]

Could you also add /usr/lib/muvm/config.json to config search path, for distro-provided configs?

[nrabulinski]

Done. I also made it so that the list can be trivially extended in the future (though that most likely won't be needed)

[nrabulinski]

If we want to bikeshed some more I could also introduce something like MUVM_CONFIG_PATH variable, but IMO that won't be necessary, especially since the goal is to have all config options also be configurable as command switches, so at worst distributions can provide a small wrapper binary/script around the main muvm bin (like NixOS will) with the default options prepended.

[slp]

LGTM, thanks!