Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

Convert Gradle script plugin for generating UniFfi bindings into a composite build. #72

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Convert Gradle script plugin for generating UniFfi bindings into a composite build. #72

merged 1 commit into from
Aug 25, 2022

Conversation

kirillzh
Copy link
Contributor

@kirillzh kirillzh commented Jul 29, 2022

Description

buildSrc has a major drawback of invalidating entire project's cache after a chance in the buildSrc itself. This is a probably not a big issue for the project of this size, but a way to fix this is to implement plugin as a standalone, composite build, which also happens to be a more idiomatic way of implementing plugins.

Notes to the reviewers

These are the rough steps that I took for converting the plugin:

  1. . Create new standalone plugins Gradle module for the plugin
  2. Create two new UniFfiJvmPlugin and UniFfiAndroidPlugin Gradle plugin classes, copy-and-pasting existing script plugin implementations
  3. Register two new plugins using the same IDs as convention plugins: org.bitcoindevkit.plugins.generate-jvm-bindings and org.bitcoindevkit.plugins.generate-android-bindings
  4. Delete buildSrc

All Submissions:

@thunderbiscuit
Copy link
Member

Very neat. Will review later today but I think this is a great step forward.

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

This is awesome. 🔥 I really enjoy digging into Gradle, and I had not really looked into how to make our plugins into their own standalone projects. Thanks for showing us how it's done!

My only comment slash small nit is that I think we should keep the style consistent and apply 4 spaces for the indents in the build.gradle.kts and the settings.gradle.kts files. Otherwise this is good to go.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

utACK cc20cf6

Looks good!

@thunderbiscuit
Copy link
Member

@kirillzh I can't seem to push the small fix for the indentation to your branch. Do you mind adding it yourself? I'll merge this right after.

It's basically just using 4 spaces indent for the build.gradle.kts and the settings.gradle.kts files.

@kirillzh
Copy link
Contributor Author

kirillzh commented Aug 9, 2022

@thunderbiscuit, reformatted plugins/build.gradle.kts and plugins/settings.gradle.kts 👍🏻

@thunderbiscuit
Copy link
Member

The CI is failing with this error:

   Compiling bdk-ffi-bindgen v0.2.0 (/home/runner/work/bdk-kotlin/bdk-kotlin/bdk-ffi/bdk-ffi-bindgen)
error: linker `aarch64-linux-android21-clang` not found
  |
  = note: No such file or directory (os error 2)

error: could not compile `bdk-ffi-bindgen` due to previous error
warning: build failed, waiting for other jobs to finish...

> Task :android:buildAndroidAarch64Binary FAILED
error: could not compile `bdk-ffi` due to previous error

I will investigate tomorrow.

@thunderbiscuit
Copy link
Member

Potentially coming from GitHub migrating the NDK to version 25 (see announcement here: actions/runner-images#5930).

@thunderbiscuit
Copy link
Member

I managed to fix the CI. Will open the PR tomorrow, and then we can finally merge this thing!

@notmandatory
Copy link
Member

@kirillzh the CI issue was fixed in #77 so ready for you to rebase this one, thanks!

@thunderbiscuit
Copy link
Member

This time around the error is

* Where:
Build file '/home/runner/work/bdk-kotlin/bdk-kotlin/plugins/build.gradle.kts' line: 1

* What went wrong:
Plugin [id: 'org.gradle.kotlin.kotlin-dsl', version: '2.1.7'] was not found in any of the following sources:

- Gradle Core Plugins (plugin is not in 'org.gradle' namespace)
- Included Builds (None of the included builds contain this plugin)
- Plugin Repositories (could not resolve plugin artifact 'org.gradle.kotlin.kotlin-dsl:org.gradle.kotlin.kotlin-dsl.gradle.plugin:2.1.7')
  Searched in the following repositories:
    Gradle Central Plugin Repository

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Aug 24, 2022

Not sure where the decision to pull this particular version of the plugin is made... do you mind trying to set the version explicitly?

plugins {
    id("java-gradle-plugin")
    `kotlin-dsl` version "2.4.1"
    // id("org.gradle.kotlin.kotlin-dsl") version "2.4.1"
}

Although version 2.1.7 is a valid version available on the org.gradle repository so not sure what's up there.

@kirillzh kirillzh closed this Aug 24, 2022
@kirillzh kirillzh reopened this Aug 24, 2022
repositories {
mavenCentral()
google()
gradlePluginPortal()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

* What went wrong:
Plugin [id: 'org.gradle.kotlin.kotlin-dsl', version: '2.1.7'] was not found in any of the following sources:

- Gradle Core Plugins (plugin is not in 'org.gradle' namespace)
- Included Builds (None of the included builds contain this plugin)
- Plugin Repositories (could not resolve plugin artifact 'org.gradle.kotlin.kotlin-dsl:org.gradle.kotlin.kotlin-dsl.gradle.plugin:2.1.7')
  Searched in the following repositories:
    Gradle Central Plugin Repository

@thunderbiscuit, adding gradlePluginPortal() repository here should fix the problem, though not entirely sure why 😅

Usually, plugins adds Gradle plugin repository implicitly so adding this explicitly should not be necessary 🤔 🤷🏻‍♂️

By default, the plugins {} DSL resolves plugins from the public Gradle Plugin Portal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, looks like Gradle plugin repository was down yesterday, so gradlePluginPortal() is in fact not necessary here, removing gradlePluginPortal()!

@notmandatory notmandatory added this to the Release 0.9.0 milestone Aug 25, 2022
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ReAck 989b733

Looks ready, @thunderbiscuit or I will merge tomorrow if no objections

@notmandatory notmandatory merged commit 989b733 into bitcoindevkit:master Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants