Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does exec/command need full paths? #1609

Closed
mayl opened this issue Apr 11, 2024 · 9 comments
Closed

Does exec/command need full paths? #1609

mayl opened this issue Apr 11, 2024 · 9 comments

Comments

@mayl
Copy link

mayl commented Apr 11, 2024

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.

ecmd := exec.Command("/bin/bash", "-c", cmdStr)

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? 😄

@eyberg
Copy link
Contributor

eyberg commented Apr 11, 2024

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
Copy link
Author

mayl commented Apr 13, 2024

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
Copy link
Contributor

eyberg commented Apr 23, 2024

is it possible to just symlink this in your nix pkg (mayl/nixpkgs@9dcb1d3) ? 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
Copy link
Author

mayl commented Apr 23, 2024

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
Copy link
Contributor

eyberg commented Apr 23, 2024

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
Copy link
Author

mayl commented Apr 23, 2024

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 added a commit that referenced this issue Apr 23, 2024
@eyberg
Copy link
Contributor

eyberg commented Apr 23, 2024

does this work for you? #1615

@mayl
Copy link
Author

mayl commented Apr 23, 2024

Yep, that'll work!

eyberg added a commit that referenced this issue Apr 23, 2024
@mayl
Copy link
Author

mayl commented Apr 23, 2024

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.

@mayl mayl closed this as completed Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants