Skip to content
This repository has been archived by the owner on Apr 29, 2024. It is now read-only.

Remove all NdkVersion constraints #206

Conversation

mahesh-hegde
Copy link
Contributor

Removes all ndkVersion constraints in build.gradle files.

If this passes CI, this change has to be discussed with upstream.

Motivation

The Android SDK makes the assumption that each small minor NDK version is different. so if you have 21.1.65557 (for sake of example) and project needs 21.1.65556, it will redownload the latter.

Look at the CI logs for this windows job. It's downloading 3 NDKs!.

In a developer machine, each NDK takes multiple GBs of storage space. Additionally, it takes forever to download an NDK where I live.

We should strive to use a single NDK version whenever possible, and that should be the installed version.

Concerns:

Does removing NDKVersion constraint everywhere, including the jni support library, cause errors on machine without an NDK installed already?

Ideally it should auto install whatever NDK flutter currently recommends.

cc: @dcharkes

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4517999691

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 86.912%

Totals Coverage Status
Change from base Build 4505284851: 0.0%
Covered Lines: 1959
Relevant Lines: 2254

💛 - Coveralls

@mahesh-hegde
Copy link
Contributor Author

https://github.com/dart-lang/jnigen/actions/runs/4517999691/jobs/7957586423?pr=206

Running Gradle task 'assembleRelease'...                        
Checking the license for package NDK (Side by side) 23.1.77[79](https://github.com/dart-lang/jnigen/actions/runs/4517999691/jobs/7957586423?pr=206#step:10:80)620 in C:\Android\android-sdk\licenses
License for package NDK (Side by side) 23.1.7779620 accepted.
Preparing "Install NDK (Side by side) 23.1.7779620 (revision: 23.1.7779620)".
"Install NDK (Side by side) 23.1.7779620 (revision: 23.1.7779620)" ready.
Installing NDK (Side by side) 23.1.7779620 in C:\Android\android-sdk\ndk\23.1.7779620
"Install NDK (Side by side) 23.1.7779620 (revision: 23.1.7779620)" complete.
"Install NDK (Side by side) 23.1.7779620 (revision: 23.1.7779620)" finished.
Checking the license for package NDK (Side by side) 21.4.7075529 in C:\Android\android-sdk\licenses
License for package NDK (Side by side) 21.4.7075529 accepted.
Preparing "Install NDK (Side by side) 21.4.7075529 (revision: 21.4.7075529)".
"Install NDK (Side by side) 21.4.7075529 (revision: 21.4.7075529)" ready.
Installing NDK (Side by side) 21.4.7075529 in C:\Android\android-sdk\ndk\21.4.7075529
"Install NDK (Side by side) 21.4.7075529 (revision: 21.4.7075529)" complete.
"Install NDK (Side by side) 21.4.7075529 (revision: 21.4.7075529)" finished.

Still seems to be downloading 2 NDKs. I have failed.

@mahesh-hegde mahesh-hegde marked this pull request as ready for review March 25, 2023 10:49
@HosseinYousefi
Copy link
Contributor

Still seems to be downloading 2 NDKs. I have failed.

So should we still remove the ndk constraints?

@mahesh-hegde
Copy link
Contributor Author

mahesh-hegde commented Mar 27, 2023

It doesn't make much difference, because ndkVersion was already commented out in package:jni. This PR only changes examples.

The larger question is how to get all plugin builds to use an existing NDK install.

@dcharkes

@dcharkes
Copy link
Contributor

I am not very familiar with where the extra NDK versions come from, but I know that Flutter itself pins an NDK version in its template.

See the NDK versioning doc.

The Android Gradle Plugin is pinned, and it in turn pins the NDK version.

If your existing NDK install is a different version, it will automatically download a new NDK. I don't really know a way around that.

@mahesh-hegde
Copy link
Contributor Author

Fair enough. I think we can close this. Maybe document the fact somewhere, that you may uncomment the NDK version when creating the plugin project.

@HosseinYousefi
Copy link
Contributor

Closing this then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants