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

Implement gradle rust plugin #7319

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Dec 10, 2024

Draft PR, very rough, mostly to spot any issues with CI, but feel free to review. :)

Things left to be decided:

  • Can we use a symlink for the maybenot_machines? Will it work on windows? This file is no longer required.
  • How do we handle the difference between debug/release? Currently we check if the starting task contains "release". This will break if we build debug and release at the same time. However I don't think we do this, so maybe it is an ok solution for now?
  • Currently we only generate a new relays.json file if none exists or if it is a release build (see above for how we determine that). We could add further checks for maybe the age of the file or add a flag to force it to always build. We decided to move relays.json back into build, so that it will be deleted on gradle clean
  • How do we handle flags and local properties. We should avoid using the local.properties file, but what should we use instead? The plugin already uses local.properties so we can continue use it for now.
  • Should we do cargo clean when gradle clean is run?

This change is Reviewable

@Pururun Pururun requested review from kl, Rawa and albin-mullvad December 10, 2024 22:20
Copy link

linear bot commented Dec 10, 2024

@Pururun Pururun added the Android Issues related to Android label Dec 10, 2024
@MarkusPettersson98
Copy link
Contributor

MarkusPettersson98 commented Dec 11, 2024

Things left to be decided:

* Can we use a symlink for the maybenot_machines? Will it work on windows?

This should no longer be needed when #7158 is merged 😊

Copy link
Contributor

@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: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/.gitignore line 6 at r2 (raw file):

local.properties
# We want to generate relays.json not store it
app/src/main/assets/relays.json

This seems applicable to all platforms! Should we move the ignore to the root .gitignore? 😊

Code quote:

# We want to generate relays.json not store it
app/src/main/assets/relays.json

Copy link
Contributor

@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: 0 of 16 files reviewed, 5 unresolved discussions (waiting on @Pururun)


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

    let mut move_command = Command::new("cp");
    move_command
        .arg("-p")

⛏️ move_command should probably be renamed now that cp is used instead of mv 😉

Also, could we document why the -p flag is important? 🙏

Code quote:

    let mut move_command = Command::new("cp");
    move_command
        .arg("-p")

android/build-apk.sh line 21 at r2 (raw file):

RUN_PLAY_PUBLISH_TASKS="no"
PLAY_PUBLISH_TASKS=()
SKIP_STRIPPING=${SKIP_STRIPPING:-"no"}

⛏️ This seems to be unused (🎉)

Code quote:

SKIP_STRIPPING=${SKIP_STRIPPING:-"no"}

android/build-apk.sh line 56 at r2 (raw file):

    elif [[ "$PRODUCT_VERSION" == *"-alpha"* ]]; then
        echo "Removing old Rust build artifacts"
        cargo clean

Is the intention to get rid of all explicit invocations of cargo in this script? Is cargo clean run when invoking gradlew clean? 😊

Code quote:

        echo "Removing old Rust build artifacts"
        cargo clean

android/build-apk.sh line 61 at r2 (raw file):

        PLAY_PUBLISH_TASKS=(publishPlayStagemoleReleaseBundle)
    else
        cargo clean

dito

Code quote:

cargo clean

Copy link
Contributor Author

@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.

That is great. :)

Reviewable status: 0 of 16 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ move_command should probably be renamed now that cp is used instead of mv 😉

Also, could we document why the -p flag is important? 🙏

Done


android/build-apk.sh line 21 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ This seems to be unused (🎉)

Nice catch!. Removed.


android/build-apk.sh line 56 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Is the intention to get rid of all explicit invocations of cargo in this script? Is cargo clean run when invoking gradlew clean? 😊

Cargo clean is not invoked when running gradle clean. Maybe we want to do it, I'll add it to the list.

Copy link
Contributor Author

@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.

Reviewable status: 0 of 17 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


android/.gitignore line 6 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

This seems applicable to all platforms! Should we move the ignore to the root .gitignore? 😊

We decided to move the file back into build so it will be ignored.

@Pururun Pururun force-pushed the implement-gradle-rust-plugin-droid-1377 branch from d096cde to 1b940e2 Compare December 12, 2024 11:38
@Rawa Rawa force-pushed the implement-gradle-rust-plugin-droid-1377 branch from 1b940e2 to d408fb3 Compare December 12, 2024 14:16
@Rawa Rawa force-pushed the implement-gradle-rust-plugin-droid-1377 branch from 06ebd1d to e18506b Compare December 12, 2024 15:39
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.

3 participants