-
Notifications
You must be signed in to change notification settings - Fork 389
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 daita
as a custom config name
#6202
Conversation
855b560
to
b057211
Compare
9c4a3f2
to
f0c8b56
Compare
0e32556
to
3c78155
Compare
a59f282
to
1fbda5b
Compare
a900bcf
to
a832721
Compare
a832721
to
2f6dd7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One big concern with making DAITA a feature is that you would not be able to use --all-features
(on platforms without DAITA) anymore. The DAITA feature would have to be enabled on Windows and Linux and disabled on other platforms, and it would be up to the developer to know which platforms have the feature, otherwise the app will not build.
It seems that features simply cannot be specified per platform, it is a fundamental limitation of cargo. You cannot specify which features exists or are enabled by default per platform. It is not possible to get around this using build.rs
either, as feature resolution is done before the build scripts are run. You can set cfg variables like we do with wireguard_go
in talpid-wireguard
, but this can't affect which features are enabled in dependencies.
I have thought a lot about how to get around this, but it may be the case that the only proper solution is to simply abandon this PR and keep the raw #[cfg(any(target_os = "windows", target_os = "linux"))]
gates, unfortunately.
daf0938
to
f14fd21
Compare
aa19a28
to
f743af9
Compare
f743af9
to
255fe1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 64 of 64 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @MarkusPettersson98)
mullvad-management-interface/build.rs
line 7 at r1 (raw file):
// Enable Daita by default on Linux and Windows. println!("cargo:rustc-check-cfg=cfg(daita)");
should be double :
s, i thought it would reject the single-color syntax since check-cfg is a newly added thing 🤔
mullvad-relay-selector/build.rs
line 5 at r1 (raw file):
// Enable Daita by default on Linux and Windows. println!("cargo:rustc-check-cfg=cfg(daita)");
scobido doubleido, etc
maybe do a search replace
talpid-wireguard/src/lib.rs
line 70 at r1 (raw file):
/// TODO: Document /// TODO: Rename
👁️ 👄 👁️
wireguard-go-rs/Cargo.lock
line 1 at r1 (raw file):
# This file is automatically @generated by Cargo.
wgrs is part of the workspace, this file should be deleted
wireguard-go-rs/libwg/wireguard-go
line 1 at r1 (raw file):
Subproject commit 1e3bc447c174e243df151cf668ba8eab36739ce0
I guess update this after the other PR is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @hulthe)
mullvad-management-interface/build.rs
line 7 at r1 (raw file):
Previously, hulthe (Joakim Hulthe) wrote…
should be double
:
s, i thought it would reject the single-color syntax since check-cfg is a newly added thing 🤔
Done. That's a bit weird aye, but good catch!
mullvad-relay-selector/build.rs
line 5 at r1 (raw file):
Previously, hulthe (Joakim Hulthe) wrote…
scobido doubleido, etc
maybe do a search replace
Done.
talpid-wireguard/src/lib.rs
line 70 at r1 (raw file):
Previously, hulthe (Joakim Hulthe) wrote…
👁️ 👄 👁️
Lmao, reverting this
wireguard-go-rs/Cargo.lock
line 1 at r1 (raw file):
Previously, hulthe (Joakim Hulthe) wrote…
wgrs is part of the workspace, this file should be deleted
🤦
wireguard-go-rs/libwg/wireguard-go
line 1 at r1 (raw file):
Previously, hulthe (Joakim Hulthe) wrote…
I guess update this after the other PR is merged
Yup
13d49d4
to
a49697f
Compare
b13dcc3
to
09f121c
Compare
d248a6b
to
a74161b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 15 of 15 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hulthe and @MarkusPettersson98)
ci/buildserver-build.sh
line 166 at r7 (raw file):
git checkout "$ref" git submodule update git submodule update --init --recursive --depth=1 wireguard-go-rs || true
What does the || true
part do? My guess is that it swallows error non-zero exit codes. Would it not be equivalent to add a;
?
885d8e0
to
b9156cc
Compare
55cd5cd
to
df21d29
Compare
7be0309
to
1a86a5c
Compare
8c62881
to
ba7f8ca
Compare
a9ac040
to
935e21f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 20 of 20 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hulthe and @MarkusPettersson98)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @hulthe and @MarkusPettersson98)
talpid-wireguard/src/lib.rs
line 825 at r9 (raw file):
/// Configure and start a Wireguard-go tunnel. fn open_wireguard_go_tunnel(
Missing a #[cfg(wireguard_go)]
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @hulthe and @MarkusPettersson98)
talpid-wireguard/src/lib.rs
line 825 at r9 (raw file):
Previously, Serock3 (Sebastian Holmin) wrote…
Missing a
#[cfg(wireguard_go)]
here
Actually, this belongs to the other PR
ba7f8ca
to
1415816
Compare
Gate DAITA compilation on `"cargo::rustc-cfg=daita"` emitted in build files per platform.
Distribute the app with `libwg.so` Embed absolute rpath for dev builds on platforms which dynamically links `libwg` Add WorkingDirectory key to plist file to load `libwg.so` correctly on macOS.
Align `build-apk.sh` with `build.sh`
Fix the build server scripts for Linux and Android by checking out the wireguard-go submodule.
935e21f
to
67fd997
Compare
This PR adds support for gating DAITA behind a cfg option. This makes it way easier to track what code is strictly DAITA-related, as well as enabling DAITA support on other platforms down the line.
This PR also changes the build chain of the app on Android, Linux and macOS.
libwg
is now distributed as a shared library, which is how wireguard-go currently is built for Android.This fixes DES-937.
This change is