nanovms / ops

ops - build and run nanos unikernels
https://ops.city
MIT License
1.3k stars 132 forks source link

Does exec/command need full paths? #1609

Closed mayl closed 6 months ago

mayl commented 7 months ago

I working on updating the version of ops included in nixpkgs here.

Because in nix, binaries like bash are not found at /bin/bash, I've been patching out the calls like below from /bin/bash/ to just bash and then populating ops's path so that it can find the required binaries.

https://github.com/nanovms/ops/blob/0df5de6a1439ab51caf6606abefca7599fe1e4f6/cmd/cmd_network.go#L255

So far, this seems to be working for me and it got me wondering, does ops actually need these hardcoded absolute paths to bash? If not, would you be open to a PR to let ops find these binaries solely based on PATH? If it doesn't have negative impact for other systems it would make packaging in nixos cleaner, which I accept is a bit of a niche use case... But maybe you are sympathetic to niche use cases? :smile:

eyberg commented 7 months ago

first off - thanks for the nix port! (let us know if there is a link we can throw up on ops.city or something)

no, this is just convention

ideally we aren't shelling out for anything but that is a completely diff. chunk of work to do which is independent of this work; if you've got something that will work on (linux, wsl2, macos) we are def. open

mayl commented 7 months ago

Well, I can't take credit for the initial packaging. It was actually first packaged 2years ago around version 0.1.32, but hasn't been updated since so I'm trying to update it.

I'm not a go developer, but I took a look at the docs which suggests to me that when getting rid of the absolute path, go will find executables to run based on the normal path rules for the platform it's compiled on (minus go ignores the PWD...)

I'll put a PR together for this. Do you have CI checks for wsl or macos? I only have access to a linux machine unfortunately...

eyberg commented 6 months ago

is it possible to just symlink this in your nix pkg (https://github.com/mayl/nixpkgs/commit/9dcb1d346d4b7097b90dc5b6aa8bd99b78a94ae7) ? if that's not possible we could look to see if one of these env vars are set (https://nixos.org/manual/nix/stable/command-ref/env-common) and if so use that; i had forgotten that go in the past few releases has changed how exec works

mayl commented 6 months ago

I can't symlink to /bin/ in nixos. I don't think special casing with those environment variables is going to help.

What's the reluctance to use exec.Command("bash", "-c", cmdStr) instead of exec.Command("/bin/bash", "-c", cmdStr)? As far as I can tell that will work on all platforms, but I only have Linux available to test on (where it works).

eyberg commented 6 months ago

that actually doesn't work anymore in newer go versions on purpose; i've seen quite a few pkgs put symlinks in - why wouldn't that work?

i was looking at the nix env vars and that seems like something that could be dealt with there; we don't really want to force another cmd on non-nix users just to lookup the path of a shell but we can do that behavior if we know we're in nix (by seeing that one of these env vars are set)

mayl commented 6 months ago

Is this link out of date? When I read "The functions Command and LookPath look for a program in the directories listed in the current path, following the conventions of the host operating system.", I interpreted that as long as bash exists in $PATH go will find it?

Nix makes lots of symlinks generally, but as part of it's sandboxing rules any given derivation can only write to it's output path in the nix store (which is under /nix/store almost always). Putting a system-wide symlink at /bin/bash to one particular bash shell would break the nix sandboxing rules.

Those env vars you linked are for configuring how nix commands are set, they aren't set when ops would run. Even if they were set, you wouldn't be able to figure out the exact /nix/store/<hash>-bash/bin/bash path.

I'm not sure how clear the above is, there's a lot of nix weirdness that we could get into that I'm trying to avoid and keep things brief...

If we have to resort to reading an absolute path out of environment variables, I guess I'd prefer we just write the logic to find bash in $PATH manually, then pass that through to command.

eyberg commented 6 months ago

does this work for you? https://github.com/nanovms/ops/pull/1615

mayl commented 6 months ago

Yep, that'll work!

mayl commented 6 months ago

Also, thanks for sticking with this weird edge case. I'll update my nix expression to this latest version and work on getting that into a nixpkgs pr.