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

Add Error Message when wireguard-go not found. replacement for #232 #284

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

noyez
Copy link

@noyez noyez commented Aug 26, 2023

Previously when trying to add a peer, i for the following output, which
was not helpful since i wasn't sure what file could not be found.

bkn@mbp16 bkn % sudo /Users/bkn/.cargo/bin/innernet  --verbose  install mbp16.toml
✔ Interface name · chester
[*] bringing up interface chester.
[E] failed to start the interface: chester - No such file or directory (os error 2).
[*] bringing down the interface.
[!] failed to bring down interface: chester - WireGuard name file can't be read.
[E] Failed to redeem invite. Now's a good time to make sure the server is started and accessible!

[E] chester - No such file or directory (os error 2)

After changes:

[1] bkn@mbp16> sudo target/debug/innernet --verbose  install /Users/brad/mbp16.conf             /Users/brad/source/rust/innernet
✔ Interface name · chester2
[*] bringing up interface chester2.
[E] failed to start the interface: chester2 - Cannot find "wireguard-go". Specify a custom path with WG_USERSPACE_IMPLEMENTATION..
[*] bringing down the interface.
[!] failed to bring down interface: chester2 - WireGuard name file can't be read.
[E] Failed to redeem invite. Now's a good time to make sure the server is started and accessible!

[E] chester2 - Cannot find "wireguard-go". Specify a custom path with WG_USERSPACE_IMPLEMENTATION.

ref new PR to replace #232

@noyez
Copy link
Author

noyez commented Aug 26, 2023

On a semi-related topic you might also consider adding functions to output the env-vars of WG_USERSPACE_IMPLEMENTATION, like so:

fn wg_userspace_implementation_env_var() -> &'static str { "WG_USERSPACE_IMPLEMENTATION" }
fn wg_quick_userspace_implementation_env_var() -> &'static str { "WG_QUICK_USERSPACE_IMPLEMENTATION" }

this will eliminate the same hardcoded strings in multiple locations throughout the code base. As someone who is prone to typos, this i had misspelled one of those env vars and it stumped my testing for a bit, so i thought why not just put those in a function (or a const or a const fn) or something.

Copy link
Member

@skywhale skywhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'm sorry that the PR is left unreviewed for a long time 🙇

Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also sorry for an incredibly late review on this, but it looks good! I just have on minor point to address.

},
Err(e) if e.kind() == io::ErrorKind::NotFound => Err(io::Error::new(
io::ErrorKind::NotFound,
"Cannot find \"wireguard-go\". Specify a custom path with WG_USERSPACE_IMPLEMENTATION.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably pull get_userspace_implementation() into a local var called userspace_implementation and then use it in Command::new() above, as well as here:

Suggested change
"Cannot find \"wireguard-go\". Specify a custom path with WG_USERSPACE_IMPLEMENTATION.",
"Cannot find {userspace_implementation:?}. Specify a custom path with WG_USERSPACE_IMPLEMENTATION.",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest commit.

Copy link
Member

@goodhoko goodhoko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot @noyez and forgive these late responses.

I've pushed a commit to address @bschwind review. So this should be good to go I believe!

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

Successfully merging this pull request may close these issues.

4 participants