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.

Sign up for GitHub

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

Jump to bottom

Fix Android build in CI #345

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Fix Android build in CI #345

merged 5 commits into from
Apr 24, 2023

Conversation

thunderbiscuit
Copy link
Member

Not yet sure why the Android build breaks. Might have to do with the update to the ubuntu-22.04 image.

@thunderbiscuit thunderbiscuit force-pushed the fix-android-build branch 3 times, most recently from 63f58d2 to f4c15ff Compare April 19, 2023 15:56
@thunderbiscuit thunderbiscuit self-assigned this Apr 19, 2023
@thunderbiscuit thunderbiscuit added the CI Continuous integration pipeline related label Apr 19, 2023
@thunderbiscuit thunderbiscuit force-pushed the fix-android-build branch 3 times, most recently from 2a51807 to 9130366 Compare April 19, 2023 17:12
@yellowHatpro
Copy link
Contributor

yellowHatpro commented Apr 21, 2023 •
edited
Loading

I might be super wrong, but isn't ubuntu 20.04 also affected due to NDK replacement?
Please correct me if I am wrong, but the reason why we are not using 22.04, is it because it is affected by ndk replacement as mentioned in actions/runner-images#5930 ?
If so, it is mentioned in the issue that the 20.04 will also be affected 🤔

@thunderbiscuit
Copy link
Member Author

Yep indeed they both need the manual download of NDK 21. I wondered whether there was something else about the ubuntu-22.04 image that might have caused the issue, but indeed both versions return the same error. So I don't think the upgrade to the 22.04 version was the problem in the end.

@notmandatory
Copy link
Member

notmandatory commented Apr 22, 2023 •
edited
Loading

Give this a try to fix the build: afa284f

I think it broke because of an issue in rust 1.68 which is default in ubuntu 20.04 and 22.04. Sounds like this issue: cross-rs/cross#1222

Rust version changed with this runner image release on Mar 13: actions/runner-images@87dc452

@thunderbiscuit thunderbiscuit mentioned this pull request Apr 22, 2023
Closed
@thunderbiscuit
Copy link
Member Author

I have added a workflow_dispatch to all test workflows so as to be able to trigger them manually in the future (so as to not need the 1 line changes in the readmes just to run specific workflows).

notmandatory
notmandatory reviewed Apr 23, 2023
View reviewed changes
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.

ACK 52b6dfb, with a few comments.

bdk-jvm/README.md Outdated
@@ -98,3 +98,5 @@ dependencies {

[`bdk`]: https://github.com/bitcoindevkit/bdk
[`bdk-ffi`]: https://github.com/bitcoindevkit/bdk-ffi

Temporary line: needed to run CI
Copy link
Member

Choose a reason for hiding this comment

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

Did you forgot to remove this line?

bdk-python/README.md Outdated
@@ -46,3 +46,5 @@ tox -vv
```shell
pip install ./dist/bdkpython-<yourversion>-py3-none-any.whl
```

Temporary line: needed to run CI
Copy link
Member

Choose a reason for hiding this comment

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

Did you forgot to remove this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove now.

bdk-swift/README.md Outdated
@@ -50,3 +50,5 @@ own GitHub repository use the following steps:
[Xcode]: https://developer.apple.com/documentation/Xcode
[`bdk`]: https://github.com/bitcoindevkit/bdk
[`bdk-ffi`]: https://github.com/bitcoindevkit/bdk-ffi

Temporary line: needed to run CI
Copy link
Member

Choose a reason for hiding this comment

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

Did you forgot to remove this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a few more changes to the yaml files so I still needed them yesterday for my last push, but now that everything has passed I will remove them.

.github/workflows/publish-jvm.yaml
@@ -43,7 +46,7 @@ jobs:
build-jvm-full-library:
name: Create full bdk-jvm library
needs: [build-jvm-macOS-M1-native-lib]
runs-on: ubuntu-22.04
runs-on: ubuntu-20.04
Copy link
Member

Choose a reason for hiding this comment

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

Should the publish-android, publish-python and test-python or any other places with ubuntu-22.04 also be changed to ubuntu-20.04?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah let's do it. I'll add this to the agenda for the next dev call.

@thunderbiscuit
Copy link
Member Author

Sorry man I left this PR not in its final state this weekend. Today I'll squash all commits into one, and remove the extra lines in the readmes (I still needed them yesterday to make sure I ran all the workflows with the latest changes on the yaml, but now that they have run and succeeded, I can remove them).

The idea of running everything on 20.04 I think might be a good one for the short term. I'll fix those everywhere as well.

@thunderbiscuit thunderbiscuit force-pushed the fix-android-build branch 4 times, most recently from fdb3c3b to a59e358 Compare April 24, 2023 16:01
@thunderbiscuit
Copy link
Member Author

The build for the older versions of Python breaks, but I'm also realizing that we are supporting probably way too old versions; Python 3.6 is 8 years old at this point, and even Debian stable (version 10, Bullseye) ships with 3.9. I feel it fair to expect users of the bitcoin development kit Python bindings use a recent-ish version of Python, and propose we deprecate support for 3.6 and 3.7.

@thunderbiscuit thunderbiscuit force-pushed the fix-android-build branch 2 times, most recently from 3394678 to 12d083b Compare April 24, 2023 17:20
@thunderbiscuit
Copy link
Member Author

Note that I also removed the PyPy releases in favour of just the CPython ones. I'd be happy to bring them back if we get demand for it, but at this point I don't know that it's really useful, and the versions it was even supporting were 3.7 and 3.8.

I think the cleaner approach is to start by supporting recent versions of CPython, and address demands for older/esoteric versions if and as they come.

notmandatory
notmandatory approved these changes Apr 24, 2023
View reviewed changes
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.

ACK 871a06d

Nice well organized PR!

@thunderbiscuit thunderbiscuit merged commit 871a06d into bitcoindevkit:master Apr 24, 2023
@thunderbiscuit thunderbiscuit deleted the fix-android-build branch November 14, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notmandatory notmandatory notmandatory approved these changes

Assignees

@thunderbiscuit thunderbiscuit

Labels
CI Continuous integration pipeline related
Projects
BDK-Bindings
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thunderbiscuit @yellowHatpro @notmandatory