search

nanovms / ops

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

Find binaries with PATH #1612

Closed mayl closed 5 months ago

mayl commented 5 months ago

Removes absolute paths, instead finds binaries using PATH resolution

mayl commented 5 months ago

This builds and mostly works, but some of the commands don't work and I'm not sure if it's a nixos thing or an ops thing.

For example:

❯ ops network create
exit status 1

but running

❯ ops -j --show-debug --show-warnings --show-errors network create
exit status 1

doesn't give me more information to debug what's exiting with 1.

This is the same for me before and after this patch.

eyberg commented 5 months ago

sorry, just coming back to this,

would just be using '/bin/sh' work here and we can remove the bash refs? none of these cmds afaik actually use anything bash specific, the longer term fix is to get rid of as much of the shelling out as possible

as for the error you are seeing - the network creation stuff either needs the user in a group w/privileges or suid or the like - it's for users that want to run their own orchestration; it could be more helpful w/the error msg though

mayl commented 5 months ago

sorry, just coming back to this,

Np! I'm working on this on hobby time so nights and weekends so don't expect super responsiveness from me either...

would just be using '/bin/sh' work here and we can remove the bash refs? none of these cmds afaik actually use anything bash specific, the longer term fix is to get rid of as much of the shelling out as possible

Sure, I was just carrying forward the bash/sh usage as it existed before, but we can make this all just sh. /bin/sh has the same hardcoded path issue on nix. For example on my system:

❯ readlink $(which sh)
/nix/store/rnzr5gb65b4f5xk7dmljk93km9wpsll6-bash-interactive-5.2-p15/bin/sh

Is there a reason to run all this in a shell besides piping between commands? Linking stdout to stdin looks like it could also be setup in go and then there would be no dependency on any shell. Just another thought...

as for the error you are seeing - the network creation stuff either needs the user in a group w/privileges or suid or the like - it's for users that want to run their own orchestration; it could be more helpful w/the error msg though

Hmm, on my machine even running ops as root creates the same result for me. I really don't think it's related to these changes, I think it's more likely something in my nix machine's networking is setup to be immutable. I can't really expect ops to handle that weirdness, I just don't want to inadvertently introduce something which breaks any of your other users...

eyberg commented 5 months ago

any piping you see in those commands would have been done for convenience to make something work quickly - most of this stuff has equivalents in go that could/should be used instead; it's not just piping - it'd be best if any exec call were to be avoided if possible - there are a few of these that vastly easier to fix then the others (eg: mkdir/cp should be straight-forward; replacing the call to something like qemu not so much) - but i wouldn't expect this pr to do any of this - I think your original goal was just to have less dependence on the abs. path for bash;

you should know that at least for the cmd_network stuff you were looking at - that currently has deps on brctl && dnsmasq; another place uses arp && ifconfig

so it's not just bash that has some hard-coded deps

mayl commented 5 months ago

you should know that at least for the cmd_network stuff you were looking at - that currently has deps on brctl && dnsmasq; another place uses arp && ifconfig

so it's not just bash that has some hard-coded deps

Yes, but that's fine because it's resolved through $PATH, which nix has facilities to populate appropriately. I actually add these dependencies in the nix package as well. The problem from nix's perspective is if you changed these to /bin/brctl for instance. In that case it's not just sufficient for there to be a brctrl executable available, it also has to be literally located at /bin/brctl.

I noticed your most recent commit. That looks like it could completely subsume this PR if you just swap /bin/bash for bash or sh in those two locations in tools.

mayl commented 5 months ago

superseded by https://github.com/nanovms/ops/pull/1615