Remove wireguard-go on Windows

[dlon]

It could still be manually enabled from the CLI, but the fallback was already gone. This PR removes the remaining parts.

As of this PR, go and mingw are no longer required to build the app on Windows.

---

This change is 

[linear]

DES-316 Remove wireguard-go on Windows

On Windows we have been using WireGuardNT for a long time without any known issues really. The only reason we have kept wireguard-go around is in case the kernel driver has been messy, or if we turned out to need wireguard-go for DAITA.

With the kernel driver appearing stable for a long time and DAITA decided to use WireGuardNT I think we can drop the wireguard-go fallback.

When we had automatic fallback to wireguard-go we could not possibly know how often the kernel driver failed. But since we have [removed the automatic fallback since 2023.3](#4337) and I have not heard a lot of complaints from support, it should be fine.

A benefit of ditching wireguard-go is that we also get rid of the dependency on golang completely. And with that we also get rid of the dependency on mingw on Windows, which is especially messy. See this thread for example: https://mullvad.slack.com/archives/C33N9MLQ7/p1690976457806729

[faern]

Reviewed 22 of 31 files at r1.
Reviewable status: 22 of 31 files reviewed, 5 unresolved discussions (waiting on @dlon)

---

talpid-wireguard/src/lib.rs line 775 at r1 (raw file):

        }

        #[cfg(unix)]

A bit of a nitpick. But I actually think #[cfg(not(windows))] is better than #[cfg(unix)]. The very best, but most verbose and maybe overkill might even be to always list the full list of exact platforms where we want this code. That's more explicitly telling the reader where we want this code to run. But not(windows) is at least clear that it's "all platforms we support except Windows". Whereas unix to me only reads as "all Unix platforms" which is more vague. Is Android a unix platform? Not 100% clear IMO. Any other non-unix platforms where this is not supposed to run?

Since the cfg just above this is cfg(target_os = "windows") I think this is not symmetrical. Since it's basically an if-else situation I think they should have the same cfg-logic but inverted. Or, consider using cfg_if! to make that if-else relationship clearer.

---

talpid-wireguard/src/logging.rs line 49 at r1 (raw file):

}

#[allow(dead_code)]

Is this still needed? I don't know where/why this is dead code. But if it's still needed on some platform, can that be highlighted with cfg_attr(..., allow(dead_code))?

---

talpid-wireguard/src/logging.rs line 94 at r1 (raw file):

// Callback that receives messages from WireGuard
#[cfg(unix)]

Same as the other comment I left. I honestly think #[cfg(unix)] is too vague here/not ideal. What do you think about cfg(not(windows))? Do you consider it worse for some reason?

---

talpid-wireguard/src/stats.rs line 30 at r1 (raw file):

impl Stats {
    #[cfg(unix)]

Is this module even used by non-unix? All tests and most code is cfgd away on non-unix. Can maybe the entire module be cfgd to reduce the number of cfgs in the module?

---

talpid-wireguard/src/wireguard_go.rs line 1 at r1 (raw file):

#![cfg(unix)]

I think this belongs above the mod wireguard_go statement in lib.rs. That's usually how we do it. And then it does not being an existent but empty module on non-unix.

[dlon]

Reviewable status: 22 of 31 files reviewed, 5 unresolved discussions (waiting on @faern)

---

talpid-wireguard/src/stats.rs line 30 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is this module even used by non-unix? All tests and most code is cfgd away on non-unix. Can maybe the entire module be cfgd to reduce the number of cfgs in the module?

We do need Stats/StatsMap in wireguard-nt, but they're constructed directly. It doesn't make sense to put implementation details for wireguard-nt in this module, in my opinion. Even though that is something we do for wireguard-go and the Linux kernel impl.

Maybe we should instead move that code out of this module?

[faern]

Reviewed 9 of 31 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dlon)

---

talpid-wireguard/src/stats.rs line 30 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

We do need Stats/StatsMap in wireguard-nt, but they're constructed directly. It doesn't make sense to put implementation details for wireguard-nt in this module, in my opinion. Even though that is something we do for wireguard-go and the Linux kernel impl.

Maybe we should instead move that code out of this module?

I see. So the code that is cfgd is actually just wireguard-go parsing code? Maybe that code should live in the wireguard-go module then?

It's also not very clear that cfg(unix) means "code for wireguard-go". I wonder if we should use feature flags instead and explicitly do --features wireguard-go on the platforms where we want it 🤷 (this is not a decision, but just brainstorming) We can also avoid explicitly stating the features. We can go lower level and do something like:

// In build.rs
#[cfg(any(linux, target_os = "macos", target_os = "android"))]
println!("cargo:rustc-cfg=wireguard_go");

// In the crate code
#[cfg(wireguard_go)]
...

That would make things WAY easier to understand IMO

[dlon]

Reviewable status: 26 of 34 files reviewed, 3 unresolved discussions (waiting on @faern)

---

talpid-wireguard/src/logging.rs line 49 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is this still needed? I don't know where/why this is dead code. But if it's still needed on some platform, can that be highlighted with cfg_attr(..., allow(dead_code))?

Done. In line with your other suggestion, cfg_attr(wireguard_go, allow(dead_code)) might be more readable, though.

---

talpid-wireguard/src/logging.rs line 94 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Same as the other comment I left. I honestly think #[cfg(unix)] is too vague here/not ideal. What do you think about cfg(not(windows))? Do you consider it worse for some reason?

I moved this to wireguard_go where it belongs.

---

talpid-wireguard/src/stats.rs line 30 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I see. So the code that is cfgd is actually just wireguard-go parsing code? Maybe that code should live in the wireguard-go module then?

It's also not very clear that cfg(unix) means "code for wireguard-go". I wonder if we should use feature flags instead and explicitly do --features wireguard-go on the platforms where we want it 🤷 (this is not a decision, but just brainstorming) We can also avoid explicitly stating the features. We can go lower level and do something like:

// In build.rs
#[cfg(any(linux, target_os = "macos", target_os = "android"))]
println!("cargo:rustc-cfg=wireguard_go");

// In the crate code
#[cfg(wireguard_go)]
...

That would make things WAY easier to understand IMO

I've moved the implementation-specific code into wireguard_nt, wireguard_kernel, and wireguard_go.

---

talpid-wireguard/src/wireguard_go.rs line 1 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think this belongs above the mod wireguard_go statement in lib.rs. That's usually how we do it. And then it does not being an existent but empty module on non-unix.

Done.

[faern]

Reviewed 1 of 8 files at r2.
Reviewable status: 27 of 34 files reviewed, 3 unresolved discussions (waiting on @dlon)

---

talpid-core/src/dns/windows/iphlpapi.rs line 85 at r2 (raw file):

        // This function is loaded at runtime since it may be unavailable. See the module-level
        // docs. TODO: `windows_sys` can be used directly when support for Windows 10, 2004,
        // is dropped.

This comment is actually wrong. The TODO applies when we drop support for anything older than 2004. However, this is completely unrelated to this PR and I'm not sure why this PR also does a bunch of formatting.

[faern]

Reviewable status: 27 of 34 files reviewed, 4 unresolved discussions (waiting on @dlon)

---

talpid-wireguard/src/logging.rs line 49 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Done. In line with your other suggestion, cfg_attr(wireguard_go, allow(dead_code)) might be more readable, though.

Yes, if we are going with the wireguard_go cfg this should definitely have it as well.

---

talpid-wireguard/src/wireguard_go.rs line 325 at r2 (raw file):

    #[derive(err_derive::Error, Debug, PartialEq)]
    pub enum Error {
        #[error(display = "Failed to parse peer pubkey from string \"_0\"")]

This will just print "_0" not the actual content of self.0. It's missing {}. Same on the error below

---

talpid-wireguard/src/wireguard_kernel/stats.rs line 4 at r2 (raw file):

use crate::stats::{Stats, StatsMap};

impl Stats {

I'm not sure I'm a fan of how this structure ended up. Now we have a bunch of stats modules in this crate and the impl Foo blocks are scattered all over.

Why is parse_device_message even an associated function on stats. It neither takes self nor returns Self. It does not look like it belongs on this type really. Looks more like it should be a method on DeviceMessage maybe (parse_stats(&self) -> StatsMap)? Or freestanding function in wireguard_kernel.

It's just a smell to me to have a dedicated module for a single function that is defined somewhere else than the type it operates on and also does not really operate on the type it's accociated with.

[dlon]

Reviewable status: 27 of 34 files reviewed, 4 unresolved discussions (waiting on @faern)

---

talpid-wireguard/src/wireguard_kernel/stats.rs line 4 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I'm not sure I'm a fan of how this structure ended up. Now we have a bunch of stats modules in this crate and the impl Foo blocks are scattered all over.

Why is parse_device_message even an associated function on stats. It neither takes self nor returns Self. It does not look like it belongs on this type really. Looks more like it should be a method on DeviceMessage maybe (parse_stats(&self) -> StatsMap)? Or freestanding function in wireguard_kernel.

It's just a smell to me to have a dedicated module for a single function that is defined somewhere else than the type it operates on and also does not really operate on the type it's accociated with.

I think of it as a constructor for Stats (or StatsMap, in reality). This particular constructor is only visible to wireguard_kernel. Does that make sense?

As for why it's in a module, that's just because I put everything in impl::stats for all implementations, and it made more sense in the other cases.

I can move it to a function if you still disagree, though.

[faern]

Reviewable status: 27 of 34 files reviewed, 4 unresolved discussions (waiting on @dlon)

---

talpid-wireguard/src/wireguard_kernel/stats.rs line 4 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think of it as a constructor for Stats (or StatsMap, in reality). This particular constructor is only visible to wireguard_kernel. Does that make sense?

As for why it's in a module, that's just because I put everything in impl::stats for all implementations, and it made more sense in the other cases.

I can move it to a function if you still disagree, though.

I actually don't know exactly what I prefer. But I think this current solution maybe has one slight code smell too many 🤔

[faern]

Reviewed 4 of 8 files at r2.
Reviewable status: 31 of 34 files reviewed, 3 unresolved discussions (waiting on @dlon)

---

talpid-wireguard/src/wireguard_kernel/stats.rs line 4 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I actually don't know exactly what I prefer. But I think this current solution maybe has one slight code smell too many 🤔

Can we make this a free standing function? Then at least we get rid of having impl Stats blocks scattered in so many places. Since there is no benefit of having this associated with Stats from what I can see. Then I guess we can live with the rest.

[faern]

Reviewed 1 of 8 files at r2.
Reviewable status: 32 of 34 files reviewed, 2 unresolved discussions (waiting on @dlon)

---

talpid-wireguard/src/wireguard_kernel/stats.rs line 4 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can we make this a free standing function? Then at least we get rid of having impl Stats blocks scattered in so many places. Since there is no benefit of having this associated with Stats from what I can see. Then I guess we can live with the rest.

Well, let's go then. We do this in the other stats modules as well. I'm not particularly fond of it. But let's move ahead instead of changing all of them. Better to be consistent at least. So we should have it like this either in all stats modules or none.

[dlon]

Reviewable status: 20 of 35 files reviewed, 2 unresolved discussions (waiting on @faern)

---

talpid-wireguard/src/logging.rs line 49 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Yes, if we are going with the wireguard_go cfg this should definitely have it as well.

Done.

---

talpid-wireguard/src/wireguard_go.rs line 325 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This will just print "_0" not the actual content of self.0. It's missing {}. Same on the error below

True. This was actually there before, but fixed.

---

talpid-core/src/dns/windows/iphlpapi.rs line 85 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This comment is actually wrong. The TODO applies when we drop support for anything older than 2004. However, this is completely unrelated to this PR and I'm not sure why this PR also does a bunch of formatting.

You're correct, but I've reverted this diff since it's irrelevant to this PR.

[faern]

Reviewed 1 of 8 files at r2, 10 of 12 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

---

CHANGELOG.md line 105 at r4 (raw file):

#### Windows
- Removed wireguard-go (userspace WireGuard) support.

Nit: Remove for correct form