-
Notifications
You must be signed in to change notification settings - Fork 533
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
D8 and R8 integration #2019
D8 and R8 integration #2019
Conversation
3766aa7
to
7681387
Compare
@jonathanpeppers looks ok so far. I do think we need to make sure we test the new R8 stuff on windows where the path has spaces. Historically, the .bat files shipped by google are really bad a handling those cases. We just need to make sure we can rely on the .bat and not have to call the various tooling directly. |
7681387
to
59c68e7
Compare
Currently hitting this:
|
59c68e7
to
9b5436d
Compare
I think the PR build is actually really green now! To get the right warning codes, I think I need #1986 merged so I can use Then I think all that is left here is some cleanup, and I can remove the |
Merged #1986
…On 31 July 2018 at 22:42, Jonathan Peppers ***@***.***> wrote:
I think the PR build is actually *really green* now!
To get the right warning codes, I think I need #1986
<#1986> merged so I can
use XA4304.
Then I think all that is left here is some cleanup, and I can remove the
do-not-merge label.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2019 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAxeeYHShbrThh5PVBEzvuBvv2qlQ5A0ks5uMM8sgaJpZM4Vnijr>
.
|
Ok been doing more testing today... And I started seeing some odd behavior when switching the I expect things to look like this: But in some situations,
Looks like this PR is not running I also noticed usage of the You can name your configurations whatever you like. We should also specify |
Ok actually, I think we should be running d8 - if proguard is off So I need to make a new For reference,
For reference,
|
9b5436d
to
281012b
Compare
I'm going to start changing this PR drastically, rebasing, squashing, etc. If we ever need the history of what was originally here: master...jonathanpeppers:d8-and-r8-history |
7b727ef
to
51f12d8
Compare
1b6fdcb
to
4d3fd2a
Compare
5b21404
to
1f0a82a
Compare
1f0a82a
to
d5363e8
Compare
Changes to
|
acbeaac
to
55b88a8
Compare
Split out part of the changes in the last commit, for a future PR, diff is smaller now:
|
I think this build was green, the test failures are network-related, or a file sharing violation (assume it's a new bot?). I added PR 2343 to hopefully help the "flakiness" of some of these tests. |
Fixes: dotnet#1423 Fixes: dotnet#2040 ~~ Overview ~~ This enables d8 and r8 integration for Xamarin.Android. d8 is a "next-generation" dex compiler (over dx), and r8 is a replacement for proguard. For full details on the feature, see `Documentation/guides/D8andR8.md`. ~~ MSBuild targets changes ~~ New MSBuild properties include: - `$(AndroidDexTool)` - an enum-style property with options: `dx` and `d8`. Defaults to `dx`. - `$(AndroidLinkTool)` - an enum-style property, that can be left blank to disable code shrinking. Valid values are `proguard` and `r8`. Existing MSBuild properties still retain the old behavior: - `$(AndroidEnableProguard)` will set `$(AndroidLinkTool)` to `proguard`, although it is no longer used internally by Xamarin.Android targets. - `$(AndroidEnableDesugar)` will default to `true` if `d8` or `r8` are used. New MSBuild tasks include: - `<D8 />` - runs `d8.jar` with the required options for Xamarin.Android. - `<R8 />` - subclasses `<D8 />`, and adds functionality for multi-dex and code-shrinking. Additionally, any new MSBuild targets are placed in a new `Xamarin.Android.D8.targets` file. This is good first step to make `Xamarin.Android.Common.targets` smaller. Additionally: * `build.props` is now invalidated via the `$(AndroidDexTool)` and `$(AndroidLinkTool)` properties, instead of `$(AndroidEnableProguard)` * `build.props` is invalidated via `$(AndroidEnableDesugar)` ~~ Test changes ~~ Tests that need to validate various combinations of properties are now using parameters such as: [Values ("dx", "d8")] string dexTool, [Values ("", "proguard", "r8")] string linkTool Set on the `XamarinAndroidApplicationProject` such as: var proj = new XamarinAndroidApplicationProject { DexTool = dexTool, LinkTool = linkTool, }; In other cases, a simple `useD8` flag was added to set `DexTool="d8"`. Since adding test parameters, exponentially causes our test cases to expand, I removed some non-essential parameters: - `BuildProguardEnabledProject` dropped `useLatestSdk`, as it does not seem applicable here (and is deprecated). Otherwise would have 24 test cases... - `BuildApplicationWithSpacesInPath` dropped `isRelease` and defaulted it to `true`. We aren't going to likely encounter issues with spaces in a path that happen *only* in a `Debug` build. Otherwise we would have 24 test cases here... - `Desugar` dropped `enableDesugar` because it is certain this application project will not build *without* desugar. We don't need to test this, and would have 24 test cases otherwise... Also dropped some `[TestCaseSource]` attributes where the `[Values]` parameter was much cleaner. ~~ Changes to test/sample projects ~~ `HelloWorld` - `$(AndroidDexTool)` set to `d8` if unspecified. So we can track the performance benefit. `Xamarin.Forms Integration` - uses `d8` and `$(AndroidLinkTool)` set to `r8`, using a `proguard.cfg` file for Xamarin.Forms. Will help us track startup performance benefits of Java code shrinking and build performance. `Mono.Android-Tests` - uses `d8` and `$(AndroidLinkTool)` set to `r8`, to verify these on-device tests pass. `Runtime-MultiDex` - `$(AndroidDexTool)` set to `d8` if unspecified, to validate multi-dex is working properly with `d8`. ~~ Xamarin.Android build changes ~~ This adds https://r8.googlesource.com/r8 as a submodule in `external/r8`, and builds `d8.jar` and `r8.jar` via `src/r8/r8.csproj`. Currently using version 1.2.50 of r8. ~~ Deployment changes ~~ Three new files will need to be included in Xamarin.Android installers: - `d8.jar` - `r8.jar` - `Xamarin.Android.D8.targets` ~~ General changes ~~ * Fixed doc for `xa4304` error code * Added `xa4305` error code for `CompileToDalvik`, `R8`, and `CreateMultiDexMainDexClassList` * Removed log messages in `CreateMultiDexMainDexClassList` ~~ Performance Comparison ~~ | MSBuild Target | Options Enabled | Time | APK size (bytes) | dex size (bytes) | | --- | --- | ---: | ---: | ---: | | _CompileToDalvikWithDx | n/a | 11074ms | 13378157 | 3894720 | | _CompileToDalvikWithD8 | d8, (desugar enabled) | 8543ms | 13124205 | 3314064 | | _CompileToDalvikWithD8 | d8, (desugar disabled) | 9550ms | 13124205 | 3314064 | | _CompileToDalvikWithDx | multi-dex | 15632ms | 13390498 | 3916496 | | _CompileToDalvikWithD8 | d8, multi-dex | 25979ms | 13054626 | 3264096 | | _CompileToDalvikWithDx | proguard | 11903ms | 12804717 | 2446964 | | _CompileToDalvikWithD8 | d8, r8 | 13799ms | 12513901 | 1835588 | | _CompileToDalvikWithDx | multi-dex, proguard | 17279ms | 12804770 | 2449512 | | _CompileToDalvikWithD8 | d8, multi-dex, r8 | 13792ms | 12513954 | 1837588 | _NOTE: desugar is enabled by default with d8/r8_ I timed this builds with [this script][powershell_script], with a "Hello World" Xamarin.Forms app. Build logs here: [d8andr8.zip][d8andr8_zip] One can draw their own conclusions on which options are faster, better, smaller. See further detail in `D8andR8.md`. [powershell_script]: https://github.com/jonathanpeppers/HelloWorld/blob/39e2854f6ca39c0941fb8bd6f2a16d8b7663003e/build.ps1 [d8andr8_zip]: https://github.com/xamarin/xamarin-android/files/2470385/d8andr8.zip Co-authored-by: Atsushi Eno <atsushieno@gmail.com>
~~ xa-prep-tasks ~~ The `<DownloadUri/>` MSBuild task now has an optional `HashHeader` property: - If set, a http HEAD request is made to the URL, and the destination file path gets a suffix added for the value. - To make this work properly, `DestinationFiles` is also an `[Output]` property, since these paths could change as a result. ~~ android-toolchain ~~ Chromium's `depot_tools` are downloaded the same as all of our other dependencies. I had to update the `<DownloadUri/>` MSBuild task to have an optional `HashHeader` property and made `DestinationFiles` an `<Output/>`. We can use this to populate `@(_DownloadedItem)`. `_UnzipFiles` also "bootstraps" `depot_tools`, and has to set/unset `$PATH` appropriately. I did this in the same place as the license acceptance for the Android SDK. ~~ r8.targets ~~ - Removed all of the `depot_tools` downloading code, and setup `android-toolchain` as a `@(ProjectReference)` - All targets that set `$PATH`, unset it when finished - Use `$(_GradleArgs)` to supply `--no-daemon` - `_CleanR8` should delete `*.jar` files in our build tree ~~ Xamarin.Android.Build.Tasks ~~ - Fixed any formatting that we noticed - Renamed `$(AndroidD8JarPath)` and `$(AndroidR8JarPath)` to be prefixed with `Android` - Refactored `$(IntermediateOutputPath)_dex_stamp` stamp file in `_CompileToDalvikWithDx` and `_CompileToDalvikWithD8` to match the new convention of `$(_AndroidStampDirectory)_CompileToDalvik.stamp` - `*.dex` files weren't in `FileWrites`??? - `CompileToDalvik` had a `DexOutputs` output property that was completely unused, so I removed it. Also removed extra log messages.
55b88a8
to
2e8acea
Compare
Context: dotnet#2019 Since 4bb4b2e, our builds have been failing on VSTS on macOS and Windows. Jenkins is green, however. ~~ macOS ~~ Currently getting a failure building `r8` such as: 2018-10-30T20:19:05.2225960Z * What went wrong: 2018-10-30T20:19:05.2227400Z Could not determine java version from '11.0.1'. So I believe the problem here is that Java 11 is in `$PATH`, and this is only a scenario on the VSTS macOS build agents. It turns out I was able to clean up quite a bit in `r8.targets`: - We don't need to call `SetEnvironmentVariable`, I was able to set `EnvironmentVariables="...;PATH=$(ChromeToolsDirectory)"` and things still worked fine. - We should pass `--stacktrace` to gradle so that we get detailed error messages on a failure. ~~ Windows ~~ Currently getting a failure in `Xamarin.Android.LibraryProjectZip-LibBinding.csproj` such as: 2018-10-31T14:02:28.8336608Z .\gradlew assembleDebug --stacktrace --no-daemon 2018-10-31T14:02:40.7122197Z NDK is missing a "platforms" directory. 2018-10-31T14:02:40.7122937Z If you are using NDK, verify the ndk.dir is set to a valid NDK directory. It is currently set to C:\Users\dlab14\android-toolchain\sdk\ndk-bundle. 2018-10-31T14:02:40.7124870Z If you are not using NDK, unset the NDK variable from ANDROID_NDK_HOME or local.properties to remove this warning. It doesn't really make sense to me why this started happening with d8/r8 support... The `~\android-toolchain\sdk\ndk-bundle` path seems completely wrong. However, it looks like we should be setting `ANDROID_NDK_HOME=$(AndroidNdkDirectory)`. This is the only other place we are calling `gradle`, so it is somehow *related* to the d8/r8 changes.
Context: dotnet#2019 Context: http://build.devdiv.io/2168276 Context: http://build.devdiv.io/2169681 Since 4bb4b2e, our builds have been failing on VSTS on Windows. Jenkins is green, however. Currently getting a failure in `Xamarin.Android.LibraryProjectZip-LibBinding.csproj` such as: 2018-10-31T14:02:28.8336608Z .\gradlew assembleDebug --stacktrace --no-daemon 2018-10-31T14:02:40.7122197Z NDK is missing a "platforms" directory. 2018-10-31T14:02:40.7122937Z If you are using NDK, verify the ndk.dir is set to a valid NDK directory. It is currently set to C:\Users\dlab14\android-toolchain\sdk\ndk-bundle. 2018-10-31T14:02:40.7124870Z If you are not using NDK, unset the NDK variable from ANDROID_NDK_HOME or local.properties to remove this warning. It doesn't really make sense to me why this started happening with d8/r8 support... The `~\android-toolchain\sdk\ndk-bundle` path seems completely wrong. However, it looks like we should be setting `ANDROID_NDK_HOME=$(AndroidNdkDirectory)`. After that change, we got a new error: 2018-10-31T17:19:30.1558223Z Checking the license for package Android SDK Build-Tools 25.0.2 in C:\Users\dlab14\android-toolchain\sdk\licenses 2018-10-31T17:19:30.1562674Z EXEC : warning : License for package Android SDK Build-Tools 25.0.2 not accepted. [E:\A\_work\14\s\tests\CodeGen-Binding\Xamarin.Android.LibraryProjectZip-LibBinding\Xamarin.Android.LibraryProjectZip-LibBinding.csproj] So this is the actual problem! In fa57aa8, I changed `android-toolchain` to not run `<AcceptAndroidSdkLicense/>` all the time. That was actually making this `.\gradlew` command successfully install Android SDK Build-Tools 25.0.2 and use it without a license prompt. We started seeing the problem since 4bb4b2e, because it caused a new `~\android-toolchain\sdk` directory to get setup. The fix here is to just use Build-Tools 28.0.0, since that is the version we boostrap and install during our build. Other changes: - Added `--stacktrace` to the gradle calls in `r8.targets`, so we get better error messages. It also matches the gradle command in `Xamarin.Android.LibraryProjectZip-LibBinding.csproj`.
Context: dotnet#2019 Context: http://build.devdiv.io/2168276 Context: http://build.devdiv.io/2169681 Since 4bb4b2e, our builds have been failing on VSTS on Windows. Jenkins is green, however. Currently getting a failure in `Xamarin.Android.LibraryProjectZip-LibBinding.csproj` such as: 2018-10-31T14:02:28.8336608Z .\gradlew assembleDebug --stacktrace --no-daemon 2018-10-31T14:02:40.7122197Z NDK is missing a "platforms" directory. 2018-10-31T14:02:40.7122937Z If you are using NDK, verify the ndk.dir is set to a valid NDK directory. It is currently set to C:\Users\dlab14\android-toolchain\sdk\ndk-bundle. 2018-10-31T14:02:40.7124870Z If you are not using NDK, unset the NDK variable from ANDROID_NDK_HOME or local.properties to remove this warning. It doesn't really make sense to me why this started happening with d8/r8 support... The `~\android-toolchain\sdk\ndk-bundle` path seems completely wrong. However, it looks like we should be setting `ANDROID_NDK_HOME=$(AndroidNdkDirectory)`. After that change, we got a new error: 2018-10-31T17:19:30.1558223Z Checking the license for package Android SDK Build-Tools 25.0.2 in C:\Users\dlab14\android-toolchain\sdk\licenses 2018-10-31T17:19:30.1562674Z EXEC : warning : License for package Android SDK Build-Tools 25.0.2 not accepted. [E:\A\_work\14\s\tests\CodeGen-Binding\Xamarin.Android.LibraryProjectZip-LibBinding\Xamarin.Android.LibraryProjectZip-LibBinding.csproj] So this is the actual problem! In fa57aa8, I changed `android-toolchain` to not run `<AcceptAndroidSdkLicense/>` all the time. That was actually making this `.\gradlew` command successfully install Android SDK Build-Tools 25.0.2 and use it without a license prompt. We started seeing the problem since 4bb4b2e, because it caused a new `~\android-toolchain\sdk` directory to get setup. The fix here is to just use Build-Tools 28.0.0, since that is the version we boostrap and install during our build. But we would never actually remember to update this value... So we should make a `build.gradle.in` file, and replace with our existing property from `Configuration.props`: <XABuildToolsFolder Condition="'$(XABuildToolsFolder)' == ''">28.0.0</XABuildToolsFolder> Other changes: - Added `--stacktrace` to the gradle calls in `r8.targets`, so we get better error messages. It also matches the gradle command in `Xamarin.Android.LibraryProjectZip-LibBinding.csproj`.
Context: http://build.devdiv.io/2168276 Context: http://build.devdiv.io/2169681 Context: #2019 Since 4bb4b2e, our builds have been failing on VSTS on Windows. Jenkins+macOS & Linux is green, however. Windows is currently getting a failure in `Xamarin.Android.LibraryProjectZip-LibBinding.csproj` such as: .\gradlew assembleDebug --stacktrace --no-daemon NDK is missing a "platforms" directory. If you are using NDK, verify the ndk.dir is set to a valid NDK directory. It is currently set to C:\Users\dlab14\android-toolchain\sdk\ndk-bundle. If you are not using NDK, unset the NDK variable from ANDROID_NDK_HOME or local.properties to remove this warning. It doesn't really make sense to me why this started happening with d8/r8 support. The `~\android-toolchain\sdk\ndk-bundle` path seems completely wrong. Regardless, it looks like we should be setting `ANDROID_NDK_HOME=$(AndroidNdkDirectory)`. After that change, we got a new error: Checking the license for package Android SDK Build-Tools 25.0.2 in C:\Users\dlab14\android-toolchain\sdk\licenses EXEC : warning : License for package Android SDK Build-Tools 25.0.2 not accepted. [E:\A\_work\14\s\tests\CodeGen-Binding\Xamarin.Android.LibraryProjectZip-LibBinding\Xamarin.Android.LibraryProjectZip-LibBinding.csproj] So this is the actual problem! In fa57aa8, I changed `android-toolchain` to not run `<AcceptAndroidSdkLicense/>` all the time. That was actually making this `.\gradlew` command successfully install Android SDK Build-Tools 25.0.2 and use it without a license prompt. We started seeing the problem since 4bb4b2e, because it caused a new `~\android-toolchain\sdk` directory to get created. The fix here is to just use Build-Tools 28.0.0, since that is the version we bootstrap and install during our build. This requires updating the `buildToolsVersion` value within `build.gradle`. However, we would never actually remember to update this value. To simplify maintenance, make a `build.gradle.in` which sets `buildToolsVersion` to `@BUILD_TOOLS_VERSION@`, and replace `@BUILD_TOOLS_VERSION@` with `$(XABuildToolsFolder)`, specified within `Configuration.props`. Additionally: add `--stacktrace` to the gradle calls within `r8.targets`, so we get better error messages. It also matches the `gradle` command in `Xamarin.Android.LibraryProjectZip-LibBinding.csproj`.
@jonathanpeppers @jonpryor how long until this is released? |
@gmoney494 it will be in preview 2 of the next Visual Studio. Unfortunately I have no idea if there is a public date for that. If you really need this feature, try one of the download links on the homepage for commercial Xamarin.Android. You need 9.1.199.x or greater. |
@jonathanpeppers is there a good documentation to install it externally instead of waiting for the preview? I downloaded the latest .pkg (i have a Mac) and installed it, but i run into 'ResolveLibraryProjectImports' error, it seems its missing the new system dependencies. Any help here? |
@gmoney494 I heard from someone else they had trouble installing the latest master build on top of Visual Studio Windows (stable). What is the error message you got on Mac? |
Well i get the installer to show and go through installing no problem. But when i try to do a build, i get this error:
Which is funny, because i can manually build it from termninal successfully with the |
this might be that the current stable VSForMac is trying to run the build in 32bit more but the libzip .dylib is only 64bit now. which would explain why it would work from the terminal. |
@dellis1972 is there a way to specify Vs to use the 64 bit? |
@gmoney494 |
Hello @jonathanpeppers @dellis1972, any workaround to build in 64bits in VSForMac ? I encounter the same issue and need the debugger. |
i got the install to work on my device, i have limited debugger support (have to use the old Android studio log cat to see anything in the console). However, i would need the breakpoint functionality to fully debug my code. Would the 2019 VS pre-release for Mac include these fixes/changes? There has not been a new release on the Alpha or Beta channels on my mac for months now. Just this latest security patch. Luckily my deadlines have been pushed back (for other reasons), but this has been a literal stopper for me. |
Yes ! VS 2019 for Mac include theses changes. You have to download VS pre-release. It works on my side. |
Hello @joelmartinez @dellis1972 But when I checkthe output log, I see that I know that I'm doing something wrong but I don't know what it is and where to get help. EDIT: I used the installer from the Download section and it worked well |
Hi @wiwski I think you need 9.1.199.x: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-d16-0/ I would only use it on top of the Visual Studio 2019 preview. When 2019 Preview 2 ships, you can just use that, but it is not out yet. |
Fixes: #1423
Fixes: #2040
Overview
This enables d8 and r8 integration for Xamarin.Android. d8 is a
"next-generation" dex compiler (over dx), and r8 is a replacement for
proguard. For full details on the feature, see
Documentation/guides/D8andR8.md
.MSBuild targets changes
New MSBuild properties include:
$(AndroidDexTool)
- an enum-style property with options:dx
and
d8
. Defaults todx
.$(AndroidLinkTool)
- an enum-style property, that can be leftblank to disable code shrinking. Valid values are
proguard
andr8
.Existing MSBuild properties still retain the old behavior:
$(AndroidEnableProguard)
will set$(AndroidLinkTool)
toproguard
, although it is no longer used internally byXamarin.Android targets.
$(AndroidEnableDesugar)
will default totrue
ifd8
orr8
areused.
New MSBuild tasks include:
<D8 />
- runsd8.jar
with the required options forXamarin.Android.
<R8 />
- subclasses<D8 />
, and adds functionality for multi-dexand code-shrinking.
Additionally, any new MSBuild targets are placed in a new
Xamarin.Android.D8.targets
file. This is good first step to makeXamarin.Android.Common.targets
smaller.Additionally:
build.props
is now invalidated via the$(AndroidDexTool)
and
$(AndroidLinkTool)
properties, instead of$(AndroidEnableProguard)
build.props
is invalidated via$(AndroidEnableDesugar)
Test changes
Tests that need to validate various combinations of properties are now
using parameters such as:
Set on the
XamarinAndroidApplicationProject
such as:In other cases, a simple
useD8
flag was added to setDexTool="d8"
.Since adding test parameters, exponentially causes our test cases to
expand, I removed some non-essential parameters:
BuildProguardEnabledProject
droppeduseLatestSdk
, as it does notseem applicable here (and is deprecated). Otherwise would have 24
test cases...
BuildApplicationWithSpacesInPath
droppedisRelease
and defaultedit to
true
. We aren't going to likely encounter issues with spacesin a path that happen only in a
Debug
build. Otherwise we wouldhave 24 test cases here...
Desugar
droppedenableDesugar
because it is certain thisapplication project will not build without desugar. We don't need
to test this, and would have 24 test cases otherwise...
Also dropped some
[TestCaseSource]
attributes where the[Values]
parameter was much cleaner.
Changes to test/sample projects
HelloWorld
-$(AndroidDexTool)
set tod8
if unspecified. Sowe can track the performance benefit.
Xamarin.Forms Integration
- usesd8
and$(AndroidLinkTool)
setto
r8
, using aproguard.cfg
file for Xamarin.Forms. Will help ustrack startup performance benefits of Java code shrinking and build
performance.
Mono.Android-Tests
- usesd8
and$(AndroidLinkTool)
set tor8
,to verify these on-device tests pass.
Runtime-MultiDex
-$(AndroidDexTool)
set tod8
ifunspecified, to validate multi-dex is working properly with
d8
.Xamarin.Android build changes
This adds https://r8.googlesource.com/r8 as a submodule in
external/r8
, and buildsd8.jar
andr8.jar
viasrc/r8/r8.csproj
.Currently using version 1.2.50 of r8.
Deployment changes
Three new files will need to be included in Xamarin.Android
installers:
d8.jar
r8.jar
Xamarin.Android.D8.targets
General changes
xa4304
error codexa4305
error code forCompileToDalvik
,R8
, andCreateMultiDexMainDexClassList
CreateMultiDexMainDexClassList
Performance Comparison
NOTE: desugar is enabled by default with d8/r8
I timed this builds with this script, with a "Hello World" Xamarin.Forms app. Build logs here: d8andr8.zip
One can draw their own conclusions on which options are faster, better, smaller.
Some of my thoughts:
desugar
is slower?multi-dex
makes the dex file larger, because new classes are required. The app wasn't large enough to warrant aclasses2.dex
.d8
does not support multi-dex, and so choosingd8
+multi-dex
actually runsr8
with--no-tree-shaking --no-minification
. These options are slower?