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

wireguard-go: fix executable name #88610

Merged
merged 1 commit into from May 23, 2020
Merged

wireguard-go: fix executable name #88610

merged 1 commit into from May 23, 2020

Conversation

@Ma27
Copy link
Member

Ma27 commented May 22, 2020

Motivation for this change

It's supposed to be wireguard-go instead of wireguard.

Closes #88567

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@Ma27 Ma27 force-pushed the Ma27:wireguard-go branch from 9562e3b to 0ededeb May 23, 2020
@flokli
Copy link
Contributor

flokli commented May 23, 2020

@flokli
Copy link
Contributor

flokli commented May 23, 2020

Why is it not already named like this by the upstream build system? Or is this a convention made up by us?

Ah, I see - it's documented in #88567. In that case, please add an explanation to the commit message, so people can see why this happened just by looking at the git history.

It's supposed to be `wireguard-go` instead of `wireguard`. Upstream does
this right in their Makefile, however we use our own build-script which
creates a wrong file in $out, so it has to be fixed in the
`postInstall`-hook.

Closes #88567
@Ma27 Ma27 force-pushed the Ma27:wireguard-go branch from 0ededeb to 0f65693 May 23, 2020
@Ma27
Copy link
Member Author

Ma27 commented May 23, 2020

@flokli fixed.

@flokli flokli merged commit a04104a into NixOS:master May 23, 2020
17 checks passed
17 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0f65693"; rev="0f65693e6b63b47c89f5a77cf6b985eb652485a7"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0f65693"; rev="0f65693e6b63b47c89f5a77cf6b985eb652485a7"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0f65693"; rev="0f65693e6b63b47c89f5a77cf6b985eb652485a7"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0f65693"; rev="0f65693e6b63b47c89f5a77cf6b985eb652485a7"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0f65693"; rev="0f65693e6b63b47c89f5a77cf6b985eb652485a7"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0f65693"; rev="0f65693e6b63b47c89f5a77cf6b985eb652485a7"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="0f65693"; rev="0f65693e6b63b47c89f5a77cf6b985eb652485a7"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
wireguard-go, wireguard-go.passthru.tests on aarch64-linux Success
Details
wireguard-go, wireguard-go.passthru.tests on x86_64-darwin Success
Details
wireguard-go, wireguard-go.passthru.tests on x86_64-linux Success
Details
@Ma27 Ma27 deleted the Ma27:wireguard-go branch May 23, 2020
@Ma27
Copy link
Member Author

Ma27 commented May 23, 2020

@flokli can you also take care of the backport? :)

@Ma27
Copy link
Member Author

Ma27 commented May 23, 2020

Ported to stable as 88b9351.

@flokli
Copy link
Contributor

flokli commented May 23, 2020

Huh, didn't get to this before now, but we should probably add the alias for 20.03 to be extra safe of not breaking someones own script in a stable release ;-)

Ma27 added a commit that referenced this pull request May 23, 2020
As proposed in #88610 (comment)
@Ma27
Copy link
Member Author

Ma27 commented May 23, 2020

Fair enough. Done in 404041b.

@zx2c4
Copy link
Contributor

zx2c4 commented May 23, 2020

Huh, didn't get to this before now, but we should probably add the alias for 20.03 to be extra safe of not breaking someones own script in a stable release ;-)

More realistically, I kind of doubt anybody uses wireguard-go on Nix. The person who added this package originally disappeared immediately after.

@zx2c4
Copy link
Contributor

zx2c4 commented May 23, 2020

Fair enough. Done in 404041b.

That commit does not look good. Now the binary is called wireguard but you have a symlink for the real name? That's bonkers. Call it wireguard-go, get rid of the binary called wireguard, and call it a day.

@Ma27
Copy link
Member Author

Ma27 commented May 23, 2020

@zx2c4 please note that this is only on release-20.03 for backwards-compatibility. As discussed above, this is not on master and will therefore vanish on the next release :)

@kirelagin
Copy link
Member

kirelagin commented May 24, 2020

I kind of doubt anybody uses wireguard-go on Nix.

There are people who use Nix and wireguard on operating systems other than Linux (e.g. macOS).
I originally added the package to use it in a wireguard module for nix-darwin, but soon after that a native macOS app for Wireguard was released, and it works well enough for the nix-darwin module to stay low on my todo list.

@lilyball
Copy link
Member

lilyball commented May 28, 2020

I filed this ticket because I needed wireguard-tools from Nix on macOS, which relies on wireguard-go.

@kirelagin
Copy link
Member

kirelagin commented May 28, 2020

The official .app bundles everything it needs.

However if you are after some terminal-based or plaintext-configuration-based solution, then, I think, getting LnL7/nix-darwin#79 to work is the coolest option. IIUC when I was working on it, I got stuck at the route management part, and I didn’t like how it was implemented in the Wireguard upstream script either, so I was trying to come up with an alternative solution and then gave up, because the .app appeared.

Now I think the best option would be to actually use the proper VPN api of the OS (as the WireGuard.app does), I am just not sure how to easily hook nix-darwin to it.

@lilyball
Copy link
Member

lilyball commented May 28, 2020

I’m using a script set up by my company that relies on wg-quick. It actually was expecting this to be installed with Homebrew but I modified it for Nix; however I don’t know enough about Wireguard to feel confident at doing anything other than modifying the path it expects wg-quick to exist at.

I’m also pushing for my team to adopt Nix for other reasons, and I think requiring nix-darwin is too high of a burden for that. Of course the rest of my team is free to still use Homebrew as well, so they could just get wireguard-tools from that; I refuse to use Homebrew for personal reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.