mozilla / rust-android-gradle

Apache License 2.0
1.03k stars 67 forks source link

Capture inputs and outputs for generated tasks #116

Closed rosingrind closed 1 year ago

rosingrind commented 1 year ago

This pull request will close #41 as it introduces Gradle's file watching behavior. The only possible problem may be a Cargo.toml custom source path case, which I believe can be solved by extending class CargoExtension with custom source path. If you think that this case should be handled, let me know/feel free to edit

rosingrind commented 1 year ago

As task outputs are defined, current setup instructions lead to warning Execution optimizations have been disabled for task ':app:mergeDebugJniLibFolders' https://github.com/mozilla/rust-android-gradle/blob/4fba4b9db16d56ba4e4f9aef2c028a4c2d6a9126/README.md?plain=1#L75-L79

Merging my pull request or not, consider changing these lines to recommend referencing merge*JniLibFolders when setting task dependencies:

project.afterEvaluate {
    tasks.getByName("mergeDebugJniLibFolders").dependsOn("cargoBuild")
    tasks.getByName("mergeReleaseJniLibFolders").dependsOn("cargoBuild")
}
ncalexan commented 1 year ago

@rosingrind thanks for the patch. Patches are always appreciated!

However, I'm wary of accepting this patch. The issue is that the only way to discover the set of inputs to cargo is to run cargo. In the past, cargo had unstable support for producing Make rules for its inputs. That support was used for exactly this purpose: integrating cargo into build systems (including Firefox's build system). But it was never stable and has since been removed. The only way is to run cargo.

That means that a whole set of on-disk modifications won't be recognized, and it's very hard to understand when and why that happens. (E.g., workspaces.) Therefore, I've always leaned towards the predictable experience of invoking cargo each build. Yes, that's not maximally efficient. Yes, that is not the Gradle-friendly approach. But it's correct and therefore predictable, and I don't want to compromise on correctness.

Thoughts?

rosingrind commented 1 year ago

Thanks for your reply!

I totally understand your point about building cargo projects, but this is a Gradle plugin and this is not the Gradle way to do builds. On my system with a single cargo project, this loose patch trims down 4 seconds from rebuild time (2 for each producing library, build time down from 4.8 to 0.6 and this is not even using configuration caches) - imagine rebuilding a setup with 4 rust directories producing a total of 20 libs. This whole idea, if not sticking to Gradle way to implementing tasks (inputs/outputs, caching, optimizations etc), comes down to these 7 lines with manual lunwind tweaks:

tasks.create<Exec>("cargoBuild") {
    workingDir(rustBasePath)

    val host = "darwin-x86_64"
    val toolchain = "aarch64-linux-android"
    val toolchainUpper = toolchain.toUpperCase().replace('-', '_')
    val linker = "${ext.ndkDirectory}/toolchains/llvm/prebuilt/${host}/bin/${toolchain}${ext.defaultConfig.minSdk}-clang++"
    environment("CARGO_TARGET_${toolchainUpper}_LINKER", linker)
    commandLine ("cargo", "build", "--target", toolchain)
}

I am not trying to pressure you - quite the opposite, I wish to make this plugin better. rust-android-gradle holds 1st positions when it comes to Rust in Gradle popularity, but I believe the plugin doesn't make much sense for those who trigger cargo manually. I believe that this manual control is the situation you're appealing to: if I wish not to make Gradle invade my cargo build pipeline, I won't just use the plugin. This will gain me a full building control, and won't even alter Gradle build times - it's as easy as 2 lines in shell.

In conclusion, I think that any Gradle plugin should implement basic Gradle features, and even better will it be - implement best build system's patterns. If the plugin don't utilize build system's features - it's no better than shell script :/

rosingrind commented 1 year ago

When talking about no way to track .toml project source dirs: there must be a way to implement this in plugin, starting from trying to get it automatically with commands and ending with specifying these manually as plugin arguments. How to implement this - it's your decision as a maintainer and project leader, just be aware that the basic plugin functions are still missing, I proposed a simple hard-coded way to implement this. Leaning on cargo restrictions is no excuse in my opinion