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

Fix wireguard-go-rs build for different Android targets #6701

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Aug 29, 2024

This PR addresses several issues with building libwg.so for the supported Android targets. It removes wireguard-go-rs/libwg/build-android.sh in favor of wireguard-go-rs/build.rs.

The biggest change is that only the actual target is built, and not all 4 Android targets which was the case for build-android.sh.


This change is Reviewable

@Rawa Rawa assigned Rawa and unassigned Rawa Aug 29, 2024
@Rawa Rawa added the Android Issues related to Android label Aug 29, 2024
Pururun
Pururun previously approved these changes Aug 29, 2024
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)


wireguard-go-rs/build.rs line 166 at r1 (raw file):

/// Compile libwg as a dynamic library for android and place it in `OUT_DIR`.
// NOTE: We use dynamic linking as Go cannot produce static binaries specifically for Android.
fn build_android_dynamic_lib(daita: bool) -> anyhow::Result<()> {

Does it make sense to toggle daita? I don't think we adapt the UI to this feature flag, so it might be problems if we generate a binary without the daita support while UI thinks we do have it?


wireguard-go-rs/build.rs line 226 at r1 (raw file):

    };

    // Create the directory with RWX permissions for all users so that the build server can clean

Do we need RWX, would RW- be enough?

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a 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, 2 unresolved discussions (waiting on @Rawa)


wireguard-go-rs/build.rs line 166 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Does it make sense to toggle daita? I don't think we adapt the UI to this feature flag, so it might be problems if we generate a binary without the daita support while UI thinks we do have it?

I mostly did it like this to be in line with how libwg is built for linux and macOS (see build_static_lib). Since adding support for DAITA is not part of this PR, the daita: bool argument is just preparatory work for #6684. Either the argument is simply set to true in that PR, or we remove this argument and make the subsequent changes #6684 such that we always assume that libwg should be compiled with DAITA 😊


wireguard-go-rs/build.rs line 226 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we need RWX, would RW- be enough?

RW- ought to be enough, but I would prefer if we didn't need to muck with permissions at all 🤔

Rawa
Rawa previously approved these changes Aug 29, 2024
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @Pururun)

albin-mullvad
albin-mullvad previously approved these changes Aug 29, 2024
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


wireguard-go-rs/build.rs line 59 at r2 (raw file):

            "armv7-linux-androideabi" => Ok(Armv7),
            "i686-linux-android" => Ok(I686),
            _ => bail!("{input} is not a support android target!"),

typo: -> supported

Code quote:

support

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @albin-mullvad)


wireguard-go-rs/build.rs line 59 at r2 (raw file):

Previously, albin-mullvad wrote…

typo: -> supported

Done

Rawa
Rawa previously approved these changes Aug 29, 2024
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


wireguard-go-rs/build.rs line 221 at r3 (raw file):

/// Note: This function will create the parent directory/directories to `output` if necessary.
fn android_move_binary(binary: &Path, output: &Path) -> anyhow::Result<()> {
    // Create the directory with RWX permissions for all users so that the build server can clean

Should we remove/update the comment?

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


wireguard-go-rs/build.rs line 221 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Should we remove/update the comment?

🤦
Fixed!

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98 and @Rawa)


wireguard-go-rs/build.rs line 210 at r1 (raw file):

    // If daita is enabled, also enable the corresponding rust feature flag
    if daita {

since daita isn't implemented yet I think we should bail! here just to make that clear


wireguard-go-rs/libwg/Android.mk line 7 at r3 (raw file):

DESTDIR ?= $(OUT_DIR)
CARGO_TARGET_DIR ?=
TARGET ?=

What is TARGET and CARGO_TARGET_DIR used for?

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @hulthe and @Rawa)


wireguard-go-rs/build.rs line 210 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

since daita isn't implemented yet I think we should bail! here just to make that clear

While you are right, it will be enabled in #6684 so the window of potential confusion is rather small 😊

Do you think I should remove the bool?


wireguard-go-rs/libwg/Android.mk line 7 at r3 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

What is TARGET and CARGO_TARGET_DIR used for?

Preparatory work, could be pushed to #6684 I suppose 🤷

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98 and @Rawa)


wireguard-go-rs/build.rs line 210 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

While you are right, it will be enabled in #6684 so the window of potential confusion is rather small 😊

Do you think I should remove the bool?

Ah ok, If it's going to be fixed soon then I don't mind


wireguard-go-rs/libwg/Android.mk line 7 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Preparatory work, could be pushed to #6684 I suppose 🤷

Ah ok, If it's going to be fixed soon then I don't mind

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @Rawa)

@MarkusPettersson98 MarkusPettersson98 merged commit ebba945 into main Aug 29, 2024
50 of 51 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the fix-libwg-android-build branch August 29, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants