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

fix the build #113

Merged
merged 76 commits into from
Apr 10, 2021
Merged

fix the build #113

merged 76 commits into from
Apr 10, 2021

Conversation

joebowbeer
Copy link
Contributor

@joebowbeer joebowbeer commented Apr 7, 2021

Fixing the build before I tackle the publish changes...

The build problem was because gradle could not resolve the ndk-build path.

I believe the root cause of failure was that getNdkBuildExecutionPath() was being evaluated too early. However, the builds still succeeded for a while because ANDROID_NDK_HOME was provided by GitHub Action's builder. Then, when AGP stopped reading these deprecated env vars, our builds failed.

Note that GitHub Action's builder image has the LTS version baked-in:

https://github.com/actions/virtual-environments/blob/main/images/linux/scripts/installers/android.sh

This is the ndkVersion we specify, saving us from having to install it ourselves.

Related:

https://issuetracker.google.com/issues/144111441
gradle/gradle#12440
https://stackoverflow.com/questions/60404457

@joebowbeer joebowbeer requested a review from tkirshboim April 7, 2021 07:03
@joebowbeer joebowbeer marked this pull request as ready for review April 8, 2021 20:09
@joebowbeer joebowbeer changed the title [DRAFT] fix the build fix the build Apr 8, 2021
@tkirshboim
Copy link
Member

tkirshboim commented Apr 10, 2021

When I tried to build this branch I received the following error:

Build file '../pd-for-android/PdCore/build.gradle' line: 89

A problem occurred evaluating project ':PdCore'.
> Could not create task ':PdCore:buildNative'.
   > NDK is not installed

The error message is misleading because an NDK installation was in place. The problem was not that NDK was not installed, but rather that a certain NDK version was not installed. I resolved this issue by installing NDK version 21.4.7075529.
Is there a way that we could provide a better error message in this case? Maybe NDK version 21.4.7075529 is not installed?

If a specific NDK version is required to build, we should probably document this in the build instructions part of the Readme.

Copy link
Member

@tkirshboim tkirshboim left a comment

Choose a reason for hiding this comment

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

I was able to build and run PdTest and it worked as before, once I installed the correct NDK version.

@joebowbeer
Copy link
Contributor Author

I agree that this is confusing, and I hope it will improve.

The error message you receive is not generated by our code. (I think it is from NdkHandler.) The error message is generated when our code resolves android.ndkDirectory. Checking for null would fail in the same way; by the time our code checks that ndk-build is executable, the script has already failed...

Specifying the ndkVersion as we do is the recommended approach:

https://developer.android.com/studio/projects/configure-agp-ndk

This particular (LTS) version is specified because it is preinstalled in the GitHub builder image, but any 21.x version should work. However, this is not a new problem. I just changed the ndkVersion that is specified; if you did not already have the previous version installed then it would have failed before in the same way. It did for me, and also failed in CI.

I think the best opportunity for better messaging is to add something to the README. I'll update that after the publish changes are made.

@joebowbeer joebowbeer requested a review from tkirshboim April 10, 2021 12:19
@joebowbeer
Copy link
Contributor Author

@tkirshboim I removed the unused defines.

Copy link
Member

@tkirshboim tkirshboim left a comment

Choose a reason for hiding this comment

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

LGTM!

@joebowbeer joebowbeer merged commit 074c4b3 into libpd:master Apr 10, 2021
@joebowbeer joebowbeer deleted the joebowbeer-workflow branch April 10, 2021 18:17
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.

2 participants