Use StandardCharsets. #2131

Isira-Seneviratne · 2023-02-15T01:46:50Z

Use StandardCharsets, which is available for API levels below 19 via core library desugaring.

simonpoole · 2023-02-15T14:24:02Z

Hi, it will probably be a week or so before I get to look at this.

Have you run the Integration / Android tests on the PR yet? Previously when I've tried to use the desugaring support it had failed due to bugs in the Google supplied code (for example ConcurrentHashMap). Which is one of the reasons we haven't switched.

Isira-Seneviratne · 2023-02-15T14:26:01Z

Have you run the Integration / Android tests on the PR yet?

Not yet. I'll run them tomorrow and see if there are any issues.

simonpoole · 2023-02-15T16:23:26Z

Have you run the Integration / Android tests on the PR yet?

Not yet. I'll run them tomorrow and see if there are any issues.

Without a lot of HW the tests take a very long time to run, roughly 30 minutes on 20 parallel emulators, hours if not run in parallel. I'll do it when I'm back home next week. Just wanted to know if you had already gone through the effort.

simonpoole · 2023-02-26T13:33:29Z

I just ran the tests on Android 9 which fails fairly spectacularly (98 fails of 220 tests), so something is wrong somewhere (most likely the additional dex file not being included). I can run the app on a local 9.0 emulator so there's likely not something that is fundamentally wrong in that respect.

Just an example

java.lang.NoSuchMethodError: No static method compare(Ljava/lang/Object;Ljava/lang/Object;Ljava/util/Comparator;)I in class Lj$/util/Objects; or its super classes (declaration of 'j$.util.Objects' appears in /data/app/de.blau.android.test-DLbAQYRiYw3xsOIQJ5gSdQ==/base.apk!classes2.dex)
at bentleyottmann.SweepSegment.<init>(SweepSegment.java:26)
...

The other, currently less important, niggle is that you reordered the imports, creating a lot of unnecessary noise in the PR. Please sort as follows:

java
javax
org
com

Isira-Seneviratne · 2023-02-26T14:41:19Z

Just an example

java.lang.NoSuchMethodError: No static method compare(Ljava/lang/Object;Ljava/lang/Object;Ljava/util/Comparator;)I in class Lj$/util/Objects; or its super classes (declaration of 'j$.util.Objects' appears in /data/app/de.blau.android.test-DLbAQYRiYw3xsOIQJ5gSdQ==/base.apk!classes2.dex)
at bentleyottmann.SweepSegment.<init>(SweepSegment.java:26)
...

Hmm, it seems like in this instance, the Objects.compare() method is not being desugared for some reason.

The other, currently less important, niggle is that you reordered the imports, creating a lot of unnecessary noise in the PR. Please sort as follows:
java
javax
org
com

Sure, I'll fix the imports tomorrow.

simonpoole · 2023-02-27T07:21:16Z

The weird thing with the specific error is that this was running on Android 9 SDK 28 which should have native support for Objects.compare in its runtime.

This seems to be the root cause for a number of the errors (I haven't checked all 98) as this is used when setting up the menu when multiple elements are selected.

simonpoole · 2023-02-27T09:49:29Z

I haven't done any more work on the testing issue yet, however I've re-encountered the ConcurrentHashMap issue I mentioned. This turns out to be the serialisation library not finding it on Android 4.1 and 4.4 when desugaring is enabled. Wit:

02-27 09:31:36.356: E/AndroidRuntime(2717): FATAL EXCEPTION: main
02-27 09:31:36.356: E/AndroidRuntime(2717): Process: de.blau.android, PID: 2717
02-27 09:31:36.356: E/AndroidRuntime(2717): java.lang.NoClassDefFoundError: j$.util.concurrent.ConcurrentHashMap
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.objenesis.ObjenesisBase.<init>(ObjenesisBase.java:57)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.objenesis.ObjenesisBase.<init>(ObjenesisBase.java:43)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.objenesis.ObjenesisStd.<init>(ObjenesisStd.java:31)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.nustaq.serialization.FSTConfiguration.createAndroidDefaultConfiguration(FSTConfiguration.java:399)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.nustaq.serialization.FSTConfiguration.createAndroidDefaultConfiguration(FSTConfiguration.java:395)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at de.blau.android.App.<clinit>(App.java:746)

simonpoole · 2023-02-27T10:34:40Z

Pro memoria list of supported APIs with desugaring https://developer.android.com/studio/write/java11-nio-support-table

Isira-Seneviratne · 2023-02-27T10:38:28Z

I haven't done any more work on the testing issue yet, however I've re-encountered the ConcurrentHashMap issue I mentioned. This turns out to be the serialisation library not finding it on Android 4.1 and 4.4 when desugaring is enabled. Wit:

02-27 09:31:36.356: E/AndroidRuntime(2717): FATAL EXCEPTION: main
02-27 09:31:36.356: E/AndroidRuntime(2717): Process: de.blau.android, PID: 2717
02-27 09:31:36.356: E/AndroidRuntime(2717): java.lang.NoClassDefFoundError: j$.util.concurrent.ConcurrentHashMap
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.objenesis.ObjenesisBase.<init>(ObjenesisBase.java:57)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.objenesis.ObjenesisBase.<init>(ObjenesisBase.java:43)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.objenesis.ObjenesisStd.<init>(ObjenesisStd.java:31)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.nustaq.serialization.FSTConfiguration.createAndroidDefaultConfiguration(FSTConfiguration.java:399)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at org.nustaq.serialization.FSTConfiguration.createAndroidDefaultConfiguration(FSTConfiguration.java:395)
02-27 09:31:36.356: E/AndroidRuntime(2717): 	at de.blau.android.App.<clinit>(App.java:746)

That seems to be due to that section of code being executed before the attachBaseContext() method, which is where the multidex loading occurs. Moving it to execute after the multidex loading has occurred should fix the issue.

https://developer.android.com/studio/build/multidex

simonpoole · 2023-02-27T11:06:06Z

Moving it to execute after the multidex loading has occurred should fix the issue.

Yes it does.

Leaves the testing issue. google is not very clear on if tests should even work with de-sugaring.

simonpoole

PLS don't merge!

Always rebase on the HEAD of branch you are working relative to, that is in this case master, and then force push your work. This gives us a clean and sequential series of commits.

Isira-Seneviratne · 2023-02-27T11:20:18Z

PLS don't merge!

Always rebase on the HEAD of branch you are working relative to, that is in this case master, and then force push your work. This gives us a clean and sequential series of commits.

My bad, I'm used to merging.

simonpoole

This should have been squashed with the initial commit and then the whole thing force pushed.

I would suggest using interactive rebase to clean up everything and then force push.

simonpoole · 2023-02-27T11:47:33Z

My bad, I'm used to merging.

No panic, I got it wrong many times at first. It just makes things far easier for everybody involved.

simonpoole · 2023-02-27T12:47:17Z

Another problem, on 4.1 and 4.4 I run in to this error:

02-27 11:52:27.854: E/AndroidRuntime(7593): java.lang.NoClassDefFoundError: de.blau.android.util.collections.LongOsmElementMap$SafeIterator
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.util.collections.LongOsmElementMap.iterator(LongOsmElementMap.java:435)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.osm.Storage.getWayNodes(Storage.java:483)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.util.ACRAHelper.addElementCounts(ACRAHelper.java:53)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.DebugInformation.getDebugText(DebugInformation.java:139)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.DebugInformation.onCreate(DebugInformation.java:66)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at android.app.Activity.performCreate(Activity.java:5008)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1079)

which looked as if it could have a similar cause as (Iterator is de-sugared too)

https://issuetracker.google.com/issues/157681341

however setting debuggable to false for the debug build didn't fix the issue.

Isira-Seneviratne · 2023-02-27T13:09:26Z

Another problem, on 4.1 and 4.4 I run in to this error:

02-27 11:52:27.854: E/AndroidRuntime(7593): java.lang.NoClassDefFoundError: de.blau.android.util.collections.LongOsmElementMap$SafeIterator
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.util.collections.LongOsmElementMap.iterator(LongOsmElementMap.java:435)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.osm.Storage.getWayNodes(Storage.java:483)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.util.ACRAHelper.addElementCounts(ACRAHelper.java:53)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.DebugInformation.getDebugText(DebugInformation.java:139)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at de.blau.android.DebugInformation.onCreate(DebugInformation.java:66)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at android.app.Activity.performCreate(Activity.java:5008)
02-27 11:52:27.854: E/AndroidRuntime(7593): 	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1079)

which looked as if it could have a similar cause as (Iterator is de-sugared too)

https://issuetracker.google.com/issues/157681341

however setting debuggable to false for the debug build didn't fix the issue.

Maybe LongSparseArray could be used instead of a custom map-like class?

simonpoole · 2023-02-27T13:17:44Z

Maybe LongSparseArray could be used instead of a custom map-like class?

There would be significant downsides to that, not the least memory and performance wise (it is quite possible to have 10'000 or more entries which is outside of the design parameters of LongSparseArray).

Not to mention that the issue effects every implementation of Iterator in the code so changing use in one place is not going to actually achieve anything.

simonpoole · 2023-02-27T14:18:15Z

So the Iterator issue is clearly the same google issue as I mentioned (multiple definitions of the Interface), so it will probably never run in debug mode, which isn't really necessary as we can't/don't do automated testing on anything below Android 8 in any case.

So we just have to verify that a release build would work.

Update: this isn't looking good, to the point that I would say this is a R8 bug. Medium term we're going to up the min SDK anyway. But not just for this PR it would be nice to get the de-sugaring working now.

simonpoole · 2023-02-28T11:03:59Z

Sorry, but this is not going to work as long as we are supporting 4.1 to 4.4. I've tried multiple different approaches but there doesn't seem to be a way to stop R8 from adding both java.util.Iterator and the de-sugared version Interfaces to the class definition (verified by decompiling the dex files) which causes the runtime on 4.X to bork.

The only avenue to resolving this that could be explored that I haven't looked at, is building a custom version of the de-sugaring library that removes the java.util.Iterator de-sugaring. However this would not stop similar issues in other cases, potentially it would be better to work backwards by adding extra classes to the minimal version of the de-sugaring library.

simonpoole · 2024-05-07T08:40:54Z

The master branch has now dropped support for pre-Android 5 devices so this should be able to proceed now. Could you rebase the PR on the current master?

simonpoole · 2024-08-21T08:15:50Z

As is, integration tests fail (and likely normal use would fail too, likely this set of bugs https://issuetracker.google.com/issues/266687543 Potentially this can be fixed by upgrading AGP to 8.2. Nope.

As the de-sugaring is not actually necessary for this PR any more, I intend to merge it without it, it would have been nice to get it working, but there are limits to the time we can spend on a nice to have.

simonpoole reviewed Feb 27, 2023

View reviewed changes

Isira-Seneviratne force-pushed the StandardCharsets branch from c2bdf8e to c444f6d Compare February 27, 2023 13:00

simonpoole added Stalled Maintainability labels Feb 28, 2023

simonpoole force-pushed the master branch from 79a3b32 to 990f773 Compare August 23, 2023 08:54

Enable core library desugaring.

1676fd4

simonpoole force-pushed the StandardCharsets branch from c444f6d to 8414540 Compare August 20, 2024 10:33

Isira-Seneviratne and others added 2 commits August 20, 2024 12:43

Use StandardCharsets.

02903ac

Move FST initialization

b6a7e8e

simonpoole force-pushed the StandardCharsets branch from 8414540 to 1474767 Compare August 20, 2024 10:43

simonpoole removed the Stalled label Aug 20, 2024

simonpoole added 3 commits August 21, 2024 14:07

Refactor to static class

2520312

Update test dependencies

58a398d

Disable desugaring

2fb9f77

simonpoole force-pushed the StandardCharsets branch from 21a35d7 to 2fb9f77 Compare August 21, 2024 12:08

simonpoole merged commit dbf6df5 into MarcusWolschon:master Aug 21, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use StandardCharsets. #2131

Use StandardCharsets. #2131

Isira-Seneviratne commented Feb 15, 2023

simonpoole commented Feb 15, 2023

Isira-Seneviratne commented Feb 15, 2023 •

edited

Loading

simonpoole commented Feb 15, 2023 •

edited

Loading

simonpoole commented Feb 26, 2023 •

edited

Loading

Isira-Seneviratne commented Feb 26, 2023 •

edited

Loading

simonpoole commented Feb 27, 2023 •

edited

Loading

simonpoole commented Feb 27, 2023

simonpoole commented Feb 27, 2023

Isira-Seneviratne commented Feb 27, 2023 •

edited

Loading

simonpoole commented Feb 27, 2023

simonpoole left a comment

Isira-Seneviratne commented Feb 27, 2023

simonpoole left a comment

simonpoole commented Feb 27, 2023

simonpoole commented Feb 27, 2023

Isira-Seneviratne commented Feb 27, 2023

simonpoole commented Feb 27, 2023 •

edited

Loading

simonpoole commented Feb 27, 2023 •

edited

Loading

simonpoole commented Feb 28, 2023 •

edited

Loading

simonpoole commented May 7, 2024

simonpoole commented Aug 21, 2024 •

edited

Loading

Use StandardCharsets. #2131

Use StandardCharsets. #2131

Conversation

Isira-Seneviratne commented Feb 15, 2023

simonpoole commented Feb 15, 2023

Isira-Seneviratne commented Feb 15, 2023 • edited Loading

simonpoole commented Feb 15, 2023 • edited Loading

simonpoole commented Feb 26, 2023 • edited Loading

Isira-Seneviratne commented Feb 26, 2023 • edited Loading

simonpoole commented Feb 27, 2023 • edited Loading

simonpoole commented Feb 27, 2023

simonpoole commented Feb 27, 2023

Isira-Seneviratne commented Feb 27, 2023 • edited Loading

simonpoole commented Feb 27, 2023

simonpoole left a comment

Choose a reason for hiding this comment

Isira-Seneviratne commented Feb 27, 2023

simonpoole left a comment

Choose a reason for hiding this comment

simonpoole commented Feb 27, 2023

simonpoole commented Feb 27, 2023

Isira-Seneviratne commented Feb 27, 2023

simonpoole commented Feb 27, 2023 • edited Loading

simonpoole commented Feb 27, 2023 • edited Loading

simonpoole commented Feb 28, 2023 • edited Loading

simonpoole commented May 7, 2024

simonpoole commented Aug 21, 2024 • edited Loading

Isira-Seneviratne commented Feb 15, 2023 •

edited

Loading

simonpoole commented Feb 15, 2023 •

edited

Loading

simonpoole commented Feb 26, 2023 •

edited

Loading

Isira-Seneviratne commented Feb 26, 2023 •

edited

Loading

simonpoole commented Feb 27, 2023 •

edited

Loading

Isira-Seneviratne commented Feb 27, 2023 •

edited

Loading

simonpoole commented Feb 27, 2023 •

edited

Loading

simonpoole commented Feb 27, 2023 •

edited

Loading

simonpoole commented Feb 28, 2023 •

edited

Loading

simonpoole commented Aug 21, 2024 •

edited

Loading