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

Rust updates #15

Merged
merged 1 commit into from
Mar 13, 2019
Merged

Rust updates #15

merged 1 commit into from
Mar 13, 2019

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Mar 9, 2019

No description provided.

@str4d str4d requested a review from gmale March 9, 2019 03:50
Copy link
Contributor

@gmale gmale left a comment

Choose a reason for hiding this comment

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

I started this review optimistic about the Mozilla plugin but ended the review far less hopeful. Now, I'm beginning to like your original idea of creating gradle tasks ourselves.

It would take less than a half-day to convert our shell/batch scripts to a gradle plugin and it might not be a bad idea for Zcash to maintain an android-rust gradle plugin, similar to mozilla's but modernized with more functionality for non-rust developers (i.e. most Android teams) AND leveraging the new features in R19 which deprecated the need for make_standalone_toolchains.py

build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build-ndk-standalone.sh Outdated Show resolved Hide resolved
build-rust.sh Outdated Show resolved Hide resolved
build-rust.sh Outdated Show resolved Hide resolved
build.gradle Outdated
@@ -20,6 +20,10 @@ buildscript {
}
}

plugins {
Copy link
Contributor

Choose a reason for hiding this comment

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

there are two ways to declare plugins, we probably don't want to mix approaches. So we may want to change this to:

apply plugin: 'org.mozilla.rust-android-gradle.rust-android'

and then add the plugin to the buildscript dependencies like so per the readme usage section.:

buildscript {
    dependencies {
        classpath 'gradle.plugin.org.mozilla.rust-android-gradle:plugin:0.8.0'
    }
}

HOWEVER, I'd rather switch all the other plugins to using this style that you have here, instead, because it's cleaner and automatically adds the the buildscript dependency. I was using the other style because one of the plugins required it but I think I've since dropped that plugin in the latest version of this build file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree we should use the modern approach rather than legacy.

@str4d
Copy link
Contributor Author

str4d commented Mar 11, 2019

it might not be a bad idea for Zcash to maintain an android-rust gradle plugin, similar to mozilla's but modernized with more functionality for non-rust developers (i.e. most Android teams) AND leveraging the new features in R19 which deprecated the need for make_standalone_toolchains.py

I think it would be valuable to fork the Mozilla plugin and modify it as necessary instead of making our own plugin, because then we can offer the changes back upstream. If they accept them, then the wider ecosystem benefits, and we don't have to maintain a fork. If they don't accept them, then we're in the same position we would have been anyway.

Also some tweaks due to error-handling changes in librustzcash.
@str4d
Copy link
Contributor Author

str4d commented Mar 13, 2019

Rebased on master and removed the build system change, because #16 would conflict with it, and I'm still figuring out how to use a Gradle plugin from a GitHub branch.

@gmale
Copy link
Contributor

gmale commented Mar 13, 2019

LGTM

@str4d str4d merged commit db5abb8 into master Mar 13, 2019
@str4d str4d deleted the rust-updates branch March 13, 2019 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants