-
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
[Xamarin.Android.Build.Tasks] Add XA1025 error for Hybrid AOT on armeabi-v7a #4966
Conversation
@@ -407,6 +407,10 @@ In this message, the term "binding" means a piece of generated code that makes i | |||
<comment>The following are literal names and should not be translated: .NET, Xamarin.Android. | |||
{0} - The file name such as 'Foo.dll.config'</comment> | |||
</data> | |||
<data name="XA1025" xml:space="preserve"> | |||
<value>The experimental 'Hybrid' value for the 'AndroidAotMode' MSBuild property is not currently compatible with the armeabi-v7a target ABI. To continue using the experimental 'Hybrid' value for 'AndroidAotMode', deselect the armeabi-v7a target ABI in the Visual Studio project property pages or edit the project file in a text editor and remove 'armeabi-v7a' from the 'AndroidSupportedAbis' MSBuild property.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possible reason not to add this error message could be that, as far as I can find, the Hybrid
setting for the $(AndroidAotMode)
is undocumented, so adding an error message for it might in a small way lead to imprecise impressions about future availability and compatibility of the feature. Hopefully the fact that the message includes the term "experimental" right up front will minimize the risk of incorrect impressions.
<AndroidError Code="XA1025" | ||
ResourceName="XA1025" | ||
Condition=" '%(_BuildTargetAbis.Identity)' == 'armeabi-v7a' AND '$(AndroidAotMode)' == 'Hybrid' " /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As currently written, this new error will break builds for projects using armeabi-v7a;arm64-v8a
, even though those projects would run successfully on arm64-v8a devices.
The idea is that even though users might currently be using armeabi-v7a;arm64-v8a
"successfully," the inclusion of armeabi-v7a
in $(AndroidSupportedAbis)
gives a false impression that apps built with both ABIs enabled might work on armeabi-v7a devices too, when in fact they will not.
I think this might be an acceptable level of build breakage considering that the Hybrid
setting is undocumented and therefore not expected to be used by many projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably a couple MSBuild tests like this one:
https://github.com/xamarin/xamarin-android/blob/eb89395e672b3131cf4eaf828d5de408b843f1d9/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs#L4292-L4299
You might just add a call to proj.SetAndroidSupportedAbis ("x86")
or something so they don't break.
Thanks for adding this. I am aware the Hybrid AOT is experimental but without it Xamarin Android is not a viable option for enterprise as obfuscation can only do so much. For example dotfuscator cannot obfuscate code pulled in via nuget. I really do hope in the future Hybrid AOT can make it out of experimental. |
Changes in the latest commits: Correct the failing tests. Specifically:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tests end up green, looks good to me 👍
Changes in the latest force-push: Correct a typo in the commit message. |
@brendanzagaeski: Should there be an "easy" way to enable Hybrid+armv7, to test that it's still broken (at some point in the future)? As-is, the only way to do such "one-off" testing would be to explicitly delete the |
@brendanzagaeski I don't think this PR is completely right. In VS if select just arm64-v8a then AotAssemblies gets set to false even though AndroidAotMode stays as Hybrid. In VS advanced options this shows as (Notice AndroidSupportedAmis is empty If I untick armeabi-v7a then VS alters the project to disable AOT and LLVM So from what I can see the only way to get hybrid AOT (CIL Stripping) working is to specify which builds both arm64-v8a and armeabi-v7a. Combining Hybrid AOT and Obfuscation seems like the only real solution for an enterprise app as obfuscation abilities are limited (dotfuscator can't bundle or obfuscate anything from nuget), bundle assemblies into native code doesn't actually perform any obfuscation. So perhaps there are some VS bugs out there but the message I think that this PR needs to be is if you have Hybrid AOT enabled then you must set and build both arm64-v8a and armeabi-v7a but it wont work on an armeabi-v7a device. Is there anything in the future to get Hybrid AOT out of experimental? Obfuscation alone has some blind spots that cannot be ignored. |
Apologies for the long delay on updating this pull request. I investigated the behavior EDMIStephen described. With Visual Studio Enterprise 2019 version 16.7, I could not reproduce the behavior where Additional testing notes: To check that the proposed error message logic and wording are accurate, I used Visual Studio 2019 version 16.8 Preview with different values of
(In all scenarios, I had In summary, the current proposed wording and logic for the error message in this PR align with the observed behaviors for all of the scenarios, so this PR is looking good to move forward. Next steps: I will rebase the PR and add a way to disable the error to make it easier to test the |
43511e9
to
430699d
Compare
Changes in the latest force-push:
|
@@ -2164,6 +2164,9 @@ because xbuild doesn't support framework reference assemblies. | |||
Condition=" '%(_BuildTargetAbis.Identity)' == 'armeabi' " | |||
Text="Invalid value 'armeabi' in %24(AndroidSupportedAbis). This ABI is no longer supported. Please update your project properties to remove the old value. If the properties page does not show an 'armeabi' checkbox, un-check and re-check one of the other ABIs and save the changes." | |||
/> | |||
<AndroidError Code="XA1025" | |||
ResourceName="XA1025" | |||
Condition=" '$(AndroidAotModeValidateAbi)' != 'false' AND '%(_BuildTargetAbis.Identity)' == 'armeabi-v7a' AND '$(AndroidAotMode)' == 'Hybrid' " /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting $(AndroidAotModeValidateAbi)
=false
will now disable the error. I think a "public" property is OK because the property is expected to be API-stable until the error is no longer relevant (that is, until $(AndroidAotMode)
=Hybrid
produces apps that run successfully on armeabi-v7a). Because AndroidAotMode
is not documented, I also left the new property likewise undocumented. I'm happy to adjust either of these decisions.
I debated a little bit on the name of the property. In particular, I wondered whether it would be preferable to word the property in the "negative" like $(AndroidAotModeSkipValidateAbi)
because the default behavior right now is that validation is enabled when AndroidAotModeValidateAbi
is null
, which might be a tiny bit counter-intuitive. Keeping the property in the "positive" keeps the name shorter though, so overall I decided I'd keep it that way for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we make AndroidAotModeValidateAbi
a public property we need to document it. We should probably document AndroidAotMode
as well but that could probably be done in another PR maybe..
#### Application and library build and deployment | ||
|
||
- Projects using the experimental `Hybrid` value for the `AndroidAotMode` | ||
MSBuild property in combination with the armeabi-v7a target ABI did not yet | ||
produce a build error to help indicate that they would abort during startup | ||
in armeabi-v7a ABI device environments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document $(AndroidAotModeValidateAbi)
here as well as in BuildProcess.md
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
As I was writing a note for the The chunk of draft release note I had stared writing was:
As I wrote that out, I realized I couldn't think of much value in the property for external users. Most of the value would be for xamarin-android's test suite. I'll tentatively move forward with that line of thinking, making the property "private" and adding an on-device test for this configuration. But if anybody has other thoughts, I'm happy to adjust that plan, so let me know! |
I'm fine with that idea. 👍 |
430699d
to
83403ae
Compare
Changes in the latest force-push:
|
…abi-v7a Context: dotnet#1218 (comment) The undocumented, experimental `Hybrid` value for the `$(AndroidAotMode)` MSBuild property is not currently compatible with the armeabi-v7a target ABI. Attempting to run an app built with `$(AndroidAotMode)`=`Hybrid` in an armeabi-v7a environment results in a crash. Since it is known that this configuration currently produces a crash, emit a build error for it to improve the visibility of the known issue and reduce the time users might spend searching for the cause of the crash. Example of the current crash: F libc : Fatal signal 11 (SIGSEGV), code 2, fault addr 0x913a50c8 in tid 31140 (ppxamarinforms1) W : debuggerd: handling request: pid=31140 uid=10146 gid=10146 tid=31140 F DEBUG : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** F DEBUG : Build fingerprint: 'motorola/perry_metropcs_c/perry:7.1.1/NCQS26.69-64-21/33:user/release-keys' F DEBUG : Revision: 'p3b0' F DEBUG : ABI: 'arm' F DEBUG : pid: 31140, tid: 31140, name: ppxamarinforms1 >>> com.companyname.mobileappxamarinforms1 <<< F DEBUG : signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x913a50c8 F DEBUG : r0 be9384d4 r1 00000000 r2 be9385a0 r3 9320b1c0 F DEBUG : r4 00000000 r5 94167208 r6 00000000 r7 be938584 F DEBUG : r8 be938838 r9 ae040008 sl 00000000 fp be9385a0 F DEBUG : ip 913a50c8 sp be9384e4 lr 942a8420 pc 913a50c8 cpsr 000f0010 F DEBUG : F DEBUG : backtrace: F DEBUG : #00 pc 000250c8 [anon:libc_malloc:91380000] F DEBUG : #1 pc 0000141c <anonymous:942a7000> W ActivityManager: Activity pause timeout for ActivityRecord{7ded4d1 u0 com.companyname.mobileappxamarinforms1/crc64e53dff5578afb8f2.MainActivity t5740} I ActivityManager: Killing 30471:com.google.android.apps.fireball/u0a145 (adj 906): empty dotnet#13 D ConnectivityService: ConnectivityService NetworkRequestInfo binderDied(NetworkRequest [ LISTEN id=624, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ], android.os.BinderProxy@f4c4e10) D ActivityManager: cleanUpApplicationRecord -- 30471 E ConnectivityService: RemoteException caught trying to send a callback msg for NetworkRequest [ LISTEN id=624, [ Capabilities: INTERNET&NOT_RESTRICTED&TRUSTED&FOREGROUND] ] W : debuggerd: resuming target 31140 I BootReceiver: Copying /data/tombstones/tombstone_05 to DropBox (SYSTEM_TOMBSTONE) W ActivityManager: Force finishing activity com.companyname.mobileappxamarinforms1/crc64e53dff5578afb8f2.MainActivity I Zygote : Process 31140 exited due to signal (11) Other changes: Update the `HybridAOT` test to cover the new error. Correct the `BuildIncrementalAot` test so that it sets `$(AndroidSupportedAbis)`. A side effect is that the test cases that use `$(AndroidAotMode)`=`Full` now build successfully. Note that although those test cases now build successfully, the resulting app packages abort when run on device because Xamarin.Android requires JIT compilation: Unhandled Exception: System.ExecutionEngineException: Attempting to JIT compile method '(wrapper other) void Java.Interop.JavaVMInterface:PtrToStructure (intptr,object)' while running in aot-only mode. TODO: Add a build error for `$(AndroidAotMode)`=`Full`, likely by updating error XA3002.
83403ae
to
6ca6e8e
Compare
Changes in the latest force-push:
|
Context: #1218 (comment)
The undocumented, experimental
Hybrid
value for the$(AndroidAotMode)
MSBuild property is not currently compatible withthe armeabi-v7a target ABI. Attempting to run an app built with
$(AndroidAotMode)
=Hybrid
in an armeabi-v7a environment results in acrash.
Since it is known that this configuration currently produces a crash,
emit a build error for it to improve the visibility of the known issue
and reduce the time users might spend searching for the cause of the
crash.
Example of the current crash:
Other changes:
Update the
HybridAOT
test to cover the new error.Correct the
BuildIncrementalAot
test so that it sets$(AndroidSupportedAbis)
. A side effect is that the test cases thatuse
$(AndroidAotMode)
=Full
now build successfully. Note thatalthough those test cases now build successfully, the resulting app
packages abort when run on device because Xamarin.Android requires JIT
compilation:
TODO: Add a build error for
$(AndroidAotMode)
=Full
, likely byupdating error XA3002.