Added --classpath_entry for Desugar #2158
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
build |
dellis1972
reviewed
Sep 10, 2018
jonathanpeppers
requested changes
Sep 10, 2018
There was a problem hiding this comment.
Hi @erikpowa,
Could you share what you have setup in your projects?
Since this test is passing for me: master...jonathanpeppers:desugar-classpath-entry
And then I looked at your example:
#desugar_bad_way.bat
set JAVAEXE="C:\Program Files\Java\jdk1.8.0_181\\bin\java.exe"
set ANDROIDJAR="C:\Java\Android\android-sdk\platforms\android-28\android.jar"
%JAVAEXE% ^
-jar "desugar_deploy.jar" ^
--bootclasspath_entry %ANDROIDJAR% ^
--min_sdk_version 27 ^
--input "com.twitter.sdk.android_twitter-core.jar" ^
--output "desugar.jar"
pause
Versus:
#desugar_good_way.bat
set JAVAEXE="C:\Program Files\Java\jdk1.8.0_181\\bin\java.exe"
set ANDROIDJAR="C:\Java\Android\android-sdk\platforms\android-28\android.jar"
%JAVAEXE% ^
-jar "desugar_deploy.jar" ^
--classpath_entry com.squareup.retrofit2_converter-gson.jar ^
--classpath_entry com.squareup.retrofit2_retrofit.jar ^
--classpath_entry com.squareup.okhttp3_okhttp.jar ^
--bootclasspath_entry %ANDROIDJAR% ^
--min_sdk_version 27 ^
--input "com.twitter.sdk.android_twitter-core.jar" ^
--output "desugar.jar"
pause
How is your project setup, where it doesn't have all of these jar files passed in as --input?
I'd like to understand that before merging this.
|
@jonathanpeppers There was no problem with
look at package.config, everything is resolved when it comes to references and I have these as Now I'm confused. Is Btw If someone doesn't have correct references for a binding library, then the desugar will fail anyway with or without |
|
@erikpowa my question is:
Why aren't your jar files added to the list of java libraries in our build? https://github.com/xamarin/xamarin-android/blob/743e69a52742a8b61b6bdaacc14e83a070c391e0/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L2464-L2483 I think this might actually be the core problem we need to fix here. Do you have a binding project for each of these jars? Each one with a build action of
Then a main Xamarin.Android app project that references them all? |
|
@jonathanpeppers ohh sorry, misunderstood.
|
twitter-core.csproj
|
@jonathanpeppers Here I'm using patched/modified 9.1.0.19 (so I have --classpath_entry) |
|
Ok things look right to me: So all we need to do is put But I definitely need to put these arguments in a response file... it would be quite long here. |
|
@erikpowa I got a test case to reproduce exactly what you are seeing: master...jonathanpeppers:desugar-classpath-entry I think I was missing the AAR file for the Twitter SDK, and that reproduces the issue. Unfortunately the test cases don't all pass... I believe the fix works for your app, but it's not working in all cases. So I'll have to keep looking at this... |
|
Here is the current error, happening at the Same error reported here: #1738 |
|
I had a lot of
But your test is interesting I will try manually repro. My first guess is too old artifacts are being used (?) these are the lowest satisfactions for twitter-core that are being used in the test. Have you tried bump them up?
((Edit: TMI: POM's can be wrong, there are tons of invalid poms in every maven, example: map-utils doesn't specify dependency "com.google.android.gms-play-services-maps" while it's required for compile scope)) |
|
I was wrong, I created a new project and repro-d it locally. |
|
@erikpowa so I think something in our desugar logic is putting duplicate types in these jar files. I need to use a java decompiler and see if that is the case for jar files in I have to work on other things, but I'll come back to this later this week, hopefully. |
|
Right now I'm comparing my 2 project, one that still working with the same package.config, while the other one isn't. |
|
@erikpowa is it possible that deleting your Offhand I haven't checked, but we might not be deleting files in |
|
@jonathanpeppers Omg, finally............... I changed the minimum target ... Non-failing project had Edit: yeah I constantly deleting obj/bin when I test something Repro project (included artifacts as nuget packages): |
|
@erikpowa ok, my tests are green when I add to I think this is good for now. Next I need to look into response file support here, since the command line is going to be really long... |
|
|
|
build |
jonathanpeppers
added
the
use-merge-commit
|
@erikpowa did you sign the CLA? I thought I saw it was green before. I might have confused the bot when I rebased. |
|
Signed just now. The problem still remains, the desugar under api 24 fails because it puts it's classes and ain't removing it (this is the only problem left). Edit: I can imagine it's actually silently failing and maybe that's why the desugar's classes are still inside the jars. |
|
@jonathanpeppers 2 possible solution for #1738 :
If I add OR
If I add The problem is I don't know what
means here. (is it AAR or is it like Embeeded Resource in .net or access to the R) The only thing left is to determine which argument to use and when to use + test Edit: I wrote wrong argument for the 2nd solution |
jonathanpeppers
removed
the
use-merge-commit
jonathanpeppers
approved these changes
Sep 12, 2018
There was a problem hiding this comment.
Looks good to me now, the test will help us when we add d8/r8.
@dellis1972 let me know what you think, I can squash/merge tomorrow and write our usual lengthy commit message.
I think we should add response file support for this after #1715 is merged.
|
build |
|
@jonathanpeppers I accidentally added invalid code to Desugar.cs, now it should be okay |
To get some better testing here, I wrote a test that: - Uses the Twitter AAR file from maven central - Lots of dependencies for Twitter's SDK - Added proguard rules where needed - Requires `android:minSdkVersion="24"` for using these libraries
under API 24 pass `--desugar_try_with_resources_omit_runtime_classes`
DesugarChecks should build now under api 24
ops I copied wrong line
|
build |
jonathanpeppers
requested changes
Sep 18, 2018
There was a problem hiding this comment.
After rebasing (we have had broken builds lately), there is a test failure here:
Desugar(true, true, true) fails:
Warnings
PROGUARD retrofit2.OkHttpCall$1: can't find referenced class com.google.devtools.build.android.desugar.runtime.ThrowableExtension [E:\A\_work\14\s\bin\TestDebug\temp\Desugar(True,True,True)\UnnamedProject.csproj]
PROGUARD retrofit2.OkHttpCall$1: can't find referenced class com.google.devtools.build.android.desugar.runtime.ThrowableExtension [E:\A\_work\14\s\bin\TestDebug\temp\Desugar(True,True,True)\UnnamedProject.csproj]
PROGUARD there were 2 unresolved references to classes or interfaces. [E:\A\_work\14\s\bin\TestDebug\temp\Desugar(True,True,True)\UnnamedProject.csproj]
...
java.io.IOException: Please correct the above warnings first.
at proguard.Initializer.execute(Unknown Source)
at proguard.ProGuard.initialize(Unknown Source)
at proguard.ProGuard.execute(Unknown Source)
at proguard.ProGuard.main(Unknown Source)
Ignore ProGuard `can't find referenced class com.google.devtools.build.android.desugar...`.
|
(some research: according to this comment those classes aren't meant to be packed (as my understanding), so the warning are expected from proguard therefore ) @jonathanpeppers this should be fine btw: I couldn't find any reference in desugared+decompiled |
|
build |
jonathanpeppers
approved these changes
Sep 19, 2018
|
I think the test failure here, is random / network related on-device test. |
|
@jonpryor @dellis1972 are we good to merge this? I was going to look into response file support, but we probably need this: #1715 |
|
@jonathanpeppers asked:
I believe we are. Does this PR also fix Issue #1738, referenced in this comment? If so, the final commit message should mention that as well. |
|
Hey guys, has this been released yet? If not, what is the timetable for this fix? |
|
Hi @gmoney494, Due to some "unlucky" timing, this likely isn't going to be in a release until the next version of Visual Studio, sorry... Hopefully D8/R8 support will be available around the same time, which is currently in progress if you want to follow along: #2019 |
|
@jonathanpeppers by next version, are you referring to VS 15.9/.x or 16? |
|
@cheles this will be in 16.0 preview 1. I don't think there is a public date yet. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #2143
Context: #1639
Xamarin.Android's current implementation of desugar runs a command
such as:
However, certain jars are failing with messages such as:
Or another example:
The first fix here is to add the
--classpath_entryflag for every--input, for some reasonDesugaris not treating--inputjars asclasspath entries?
Next, we expanded on the
BuildTest.Desugartest using a complicatedset of Java libraries from Maven central:
This was working, as long as you had
android:minSdkVersion="24"inyour
AndroidManifest.xml. This wasn't ideal if you wanted to runyour app on older API levels...
Luckily, another command line switched solved this problem:
What a crazy name for a flag! Documented by
desugaras:This makes sense that something would happen at API level 24...
Once doing this, the
Desugartest passes without modifyingminSdkVersion.The expanded
Desugartest will also greatly help with our D8/R8implementation
(#2040).
This complex set of Java dependencies from Maven will be useful.