ytai / ioio

Software, firmware and hardware of the IOIO - I/O for Android
Apache License 2.0
746 stars 355 forks source link

Updated minSdkVersion #203

Closed topherbuckley closed 3 years ago

topherbuckley commented 3 years ago

I was having issues in Android Studio that HelloIOIO would not run as the "Default Activity not found".

Found tips on how to solve this here.

Looks to be some kind of bug in Android Studio or initialization error in gradle that is not able to resolve the minSDK inside the if statements as it stands. Compare this to HelloIOIOService, and you will see Android Studio has no complaints about running that as the minSdkVersion is not inside any if statements.

topherbuckley commented 3 years ago

I was doing that as a workaround as I can run the sample apps from a fresh cache, but as soon as I do a gradle sync, or build the project (essentially any gradle function), I will run into this error again until I invalidate the cache and restart Android Studio again. This was getting time consuming and annoying, so I tried to figure out the reason why this was happening, thus this PR.

Can you recreate this via: 1.) Start from a clean cache 2.) Pressing "Sync Project with Gradle Files" in Android Studio 3.) Try running HolidayIOIO or HelloIOIO without this minSdKVersion 14 statment outside the for each loop 4.) Enter the minSdKVersion 14 statment outside the for each loop and "Sync Project with Gradle Files" again 5.) Try running HolidayIOIO or HelloIOIO again.

I can reliably reproduce this:

before after

The reason for the fix was discovered via this error in the Manifest:

ManifestMergeError

I also see plenty of errors related to the minSdKVersion in the MainActivity as well after syncing (step 2 above):

ActivityErrors

My guess is that it is a threading or evaluation timing issue in the compiler coming from trying to compile the sample app build.gradle files at the same time as the IOIOLibAndroidBluetooth (and other library dependencies). The Manifest Merge Error claims the SDK 1 definition on HolidayIOIO is incompatible with the SDK 14 defined by the library dependency IOIOLibAndroidBluetooth. The minSDK in the build.gradle file of IOIOLibAndroidBluetooth is not within any if logic, and being a dependency for HolidayIOIO, its compilation is likely started before evaluating the HolidayIOIO build.gradle file. The compiler then reaches this code:

        gradle.startParameter.taskNames.each {
            if (it.contains("AndroidTest") || it.contains("cAT")) {
                minSdkVersion 21
            } else {
                minSdkVersion 14
            }
        }
        targetSdkVersion 30

        versionCode getGitCommitCount()
        versionName getTag()

        testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"

and evaluates only the first level of indentation and never reaches the definition of minSdkVersion until a second pass (compiling the inner parts of the for each loop). In between these steps, it assumes the default minSdk of 1 and throws these errors.

My reasoning for the background of the error is just speculation, but the solution is repeatable. Does this work around negatively affect anything you are working on?

topherbuckley commented 3 years ago

I'm confused, on my laptop and on CI it works properly.

Also, I should add, that I also don't have any issues with building it via gradlew nor do I see any CI errors. Does this give any clues as to some other issue? Maybe Android Studio is using a different compiler to do the linting compared to the gradle wrapper?

topherbuckley commented 3 years ago

Trying to find some things in the docs about this and here's what I've found so far:

default minSdkVersion is indeed 1 according to the Android manifest docs. So maybe the xml of HolidayIOIO manifest is evaluated before the minSdkVersion definition in the if statement is reached and it then makes the default value assumption?

I can also fix this by defining the minSdkVersion in the manifest:

manifest define

But I was under the assumption that this was bad practice, and after running the app and trying to sync again, I get the standard error you see when refactoring old Android code, "The minSdk version should not be declared in the android manifest file. You can move the version from the manifest to the defaultConfig in the build.gradle file."

topherbuckley commented 3 years ago

BTW, wouldn't this separation between AndroidTest and cAT be better suited to a separate build type? This would also eliminate this issue as we would set the higher sdk version in the testing specific build type.

topherbuckley commented 3 years ago

Shoot I did too much refactoring. Wait on this. I'll fix.

topherbuckley commented 3 years ago

Ok. Squashed all the messy commits and rebased. Let me know if you see the need for any other changes.

hannesa2 commented 3 years ago

Yes, now it's perfect

image