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

Minimal fix for crash #872 #873

Closed
wants to merge 11 commits into from
Closed

Minimal fix for crash #872 #873

wants to merge 11 commits into from

Conversation

1fish2
Copy link
Contributor

@1fish2 1fish2 commented Sep 16, 2018

Summary:

Scrolling down the 2018 and 2019 Districts screen to where "FIRST In Texas District" should appear will crash.

Issues Reference:
#872

Test Plan:
Manual test.

Screenshots:
screenshot_1537073111

This fixes the details in the build files that are no longer supported (see PR #861 for more fixes), leaving warnings about obsolete (broken?) build features:

```
Configuration 'debugProguardCompile' is obsolete and has been replaced with 'debugProguardImplementation' and 'debugProguardApi'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'androidTestCompile' is obsolete and has been replaced with 'androidTestImplementation' and 'androidTestApi'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'compile' is obsolete and has been replaced with 'implementation' and 'api'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'androidTestApi' is obsolete and has been replaced with 'androidTestImplementation'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'testCompile' is obsolete and has been replaced with 'testImplementation' and 'testApi'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'debugCompile' is obsolete and has been replaced with 'debugImplementation' and 'debugApi'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'releaseCompile' is obsolete and has been replaced with 'releaseImplementation' and 'releaseApi'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'testApi' is obsolete and has been replaced with 'testImplementation'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html

Configuration 'debugBlueCompile' is obsolete and has been replaced with 'debugBlueImplementation' and 'debugBlueApi'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html
```
Note 1: The README setup instructions step 7.ix:

    ix. In the Firebase console, create a Remote Config,

don't match the current Firebase console. What should the steps be now?

Note 2: Does the APK require a Google Play AVD to work? If so, the README should say so.
Are any additional changes needed?
@1fish2
Copy link
Contributor Author

1fish2 commented Sep 16, 2018

This PR does not try to update travis-ci nor fully fix the build files. See PR #861 for a broader attempt, if not 100% successful.

@1fish2
Copy link
Contributor Author

1fish2 commented Sep 17, 2018

Oops, I thought I pushed the whole fix the other day.

That's the minimal fix. One could go a little further and log an unrecognized district code. Also of course update the travis CI build. I could take a guess at that but it's a mystery to me.

@sorianog
Copy link

@1fish2 You still planning on going forward with this PR? Looks like Travis ain't happy 😢

@1fish2
Copy link
Contributor Author

1fish2 commented Sep 22, 2018 via email

@sorianog
Copy link

@1fish2 Hmm, I can take a look at the tests after you're done.

* build-tools-25.0.2 -> 27.0.3
* Follow the instructions in https://docs.travis-ci.com/user/languages/android/

For gradle builds, there are still the following deprecation warnings with deadlines and more. I think some of them come from libraries. See PR #861 for an attempt to fix more of that.

```
Configuration 'debugProguardCompile' is obsolete and has been replaced with 'debugProguardImplementation' and 'debugProguardApi'.
Configuration 'androidTestCompile' is obsolete and has been replaced with 'androidTestImplementation' and 'androidTestApi'.
Configuration 'compile' is obsolete and has been replaced with 'implementation' and 'api'.
Configuration 'androidTestApi' is obsolete and has been replaced with 'androidTestImplementation'.
Configuration 'testCompile' is obsolete and has been replaced with 'testImplementation' and 'testApi'.
Configuration 'debugCompile' is obsolete and has been replaced with 'debugImplementation' and 'debugApi'.
Configuration 'releaseCompile' is obsolete and has been replaced with 'releaseImplementation' and 'releaseApi'.
Configuration 'testApi' is obsolete and has been replaced with 'testImplementation'.
Configuration 'debugBlueCompile' is obsolete and has been replaced with 'debugBlueImplementation' and 'debugBlueApi'.

It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html
```

**Note:** Per [Google Play's target API level requirement](https://developer.android.com/distribute/best-practices/develop/target-sdk)

> Google Play will require that new apps target at least Android 8.0 (API level 26) from August 1, 2018, and that app updates target Android 8.0 from November 1, 2018.

So we can push an app update before Nov. 1. After that, we'll have to update to at least target SDK 26. Last I checked, Robolectric wasn't up to that and there's library version hell.
@1fish2
Copy link
Contributor Author

1fish2 commented Sep 22, 2018

@sorianog I pushed a cut at making travis-ci work. Please do look at the tests. Many thanks!

We must push a release before Nov. 1. Starting Nov. 1, releases must support API 26. That's where Robolectric, library versions, and Android changes hit the fan.

in another cut at making travis-ci happy.
I'm making educated guesses here.
Fixing that and the google-collections + guava collision makes some of the tests pass. Robolectric is unhappy. I'm calling it a day.
@sorianog
Copy link

@1fish2 Well, I'm glad I'm here to help since what you explained is definitely something I helped work on ("app currency" - keep app updated with latest libs/tools/frameworks) at my full-time gig. Esp, Roboelectric stuff. Want to be careful with the new setup for injections and what not. I'll keep that deadline in mind!

distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-3.3-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.4-all.zip

Choose a reason for hiding this comment

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

Ah, good this is a def a req for increasing your API level support. Gradle needs to be updated as you did here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Terrific! Welcome!

I have another cut at updating for Robolectric compatibility. I'll push that shortly after trying further to work through the errors that arise at each attempt.

Their migration guide is somewhat helpful. The good news is they've updated Robolectric for current or almost-current Android APIs. The bad news is various parts of their APIs changed and various other tools and libs have to upgrade to get there.

In general, the Android gradle rules (esp. for this project) and library dependencies are a mystery to me. Docs say we need to turn on includeAndroidResources = true but does that mean we should turn off the build.workaround-missing-resource.gradle?

implementation "com.squareup.retrofit2:retrofit:${retrofitVersion}"
implementation "io.reactivex:rxjava:${rxJavaVersion}"

Choose a reason for hiding this comment

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

I like how vars are used here to dynamically change the version of chosen libs/frameworks. Is this something that can be done with more of the libs in this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phil put this in. I surmise that it's particularly useful for the nested projects libImgur and libTba. I don't know why those are nested projects.

The build still gets into trouble when dependent libraries use different versions of these dependencies, not to mention outdated gradle commands like compile and task <<.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's particularly useful with ${supportLibVersion}. It does add complexity and surprises, like when gradle complains that we're not using the same support library version everywhere (which must be due to an indirect dependency), so I'd want to extend it only to places where the same version number must appear in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do the current build tools make retrofit obsolete?

Copy link

Choose a reason for hiding this comment

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

@1fish2 Nah, I think retrofit is still okay to include. That's how it is at my work's codebase at least!

It turns out to need yet newer build tools to use more recent Robolectric, which supposedly can test current Android versions.

I did a bunch of Robolectric migration work, partly by using their halfway-documented changes in in the migration guide, partly by experimentation.

Q. After adding `includeAndroidResources = true` per instructions, can/must we remove `build.workaround-missing-resource.gradle`?
It'd take a lot less guessing, googling, and experimenting.

This seems closer but there are still test errors like

    java.lang.IllegalStateException: this method should only be called by Robolectric

The log file says it's at `org.robolectric.shadows.ShadowDisplayManager.configureDefaultDisplay(ShadowDisplayManager.java:43)`.

    java.lang.IllegalArgumentException: couldn't find 'android/src/main/AndroidManifest.xml

The manifest file is present.
1fish2 added 2 commits October 1, 2018 13:07
If anyone knows how to make it work, please try.

Update to build tools 28.0.3.
@1fish2
Copy link
Contributor Author

1fish2 commented Oct 1, 2018

@sorianog I'm out of ideas to try other than updating library versions, which might have n^2 compatibility problems but it might fix some of the build problems like Configuration 'compile' is obsolete. So if you can make progress, please do.

@sorianog
Copy link

sorianog commented Oct 1, 2018

@1fish2 Thanks for the work thus far! I'll see what I can do.

@phil-lopreiato
Copy link
Member

I removed the hard-coded districts entirely in #874 so I'm going to close this, but let's definitely figure out all the inane things we need to do to get the app to build with new tools

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.

3 participants