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

Download NDK versions instead of using shipped ones by the runner #7327

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Berstanio
Copy link
Contributor

Since the NDK version pointed by ANDROID_NDK_HOME can change at any time and newer NDK version discontinue support for older android version, I think it is preferable to pin our NDK version to a specific one.

See #7307 (comment)

I verified that the commands work locally, but I haven't verified them integrated into a GHA.

@Berstanio Berstanio requested a review from a team as a code owner January 25, 2024 11:03
@Frosty-J
Copy link
Contributor

In this case, we should set ndkVersion "23.2.8568313" so people testing locally get the same results as GitHub Actions. If you wish to use r23 do you no longer stand by #7310 (review) ?

@Berstanio
Copy link
Contributor Author

Berstanio commented Jan 25, 2024

In this case, we should set ndkVersion "23.2.8568313" so people testing locally

I don't think this property affects us at all, it shouldn't get picked up by jnigen. The user is responsible on how to set the "NDK_HOME" variable.

do you no longer stand by #7310 (review) ?

I still stand by this review, that android 19 should be reasonable. However, android 16-18 are currently broken in older libGDX versions without a full discussion about it, so I choose to roll it back to this point for the moment. But I have also no real problem with r25.

@obigu
Copy link
Contributor

obigu commented Jan 25, 2024

In this case, we should set ndkVersion "23.2.8568313" so people testing locally

I don't think this property affects us at all, it shouldn't get picked up by jnigen. The user is responsible on how to set the "NDK_HOME" variable.

do you no longer stand by #7310 (review) ?

I still stand by this review, that android 19 should be reasonable. However, android 16-18 are currently broken in older libGDX versions without a full discussion about it, so I choose to roll it back to this point for the moment. But I have also no real problem with r25.

I think it's a good idea to point to a sepecific NDK version to prevent this issue in the future but I'd point to latest one (damage, if any, has already been done). When #7310 is merged (which can be done now) Gdx Setup will be changed immediately to min API 19 fixing the problem for new projects.

@Berstanio
Copy link
Contributor Author

but I'd point to latest one

The latest one is r26 which discontinues API 19 and 20 though.

@obigu
Copy link
Contributor

obigu commented Jan 25, 2024

but I'd point to latest one

The latest one is r26 which discontinues API 19 and 20 though.

Ok, previous one then :)

@obigu obigu added this to the 1.12.2 milestone Feb 22, 2024
- name: Download NDK
run: |
wget https://dl.google.com/android/repository/android-ndk-r25c-linux.zip -O android-ndk.zip
echo "53af80a1cce9144025b81c78c8cd556bff42bd0e android-ndk.zip" | sha256sum --check
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried doing this on my machine and it said "no properly formatted checksum lines found". But then I realised 53af80a1cce9144025b81c78c8cd556bff42bd0e is the sha1 of the file, not the sha256.

The actual sha256 seems to be 769ee342ea75f80619d985c2da990c48b3d8eaf45f48783a2d48870d04b46108

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

4 participants