-
Notifications
You must be signed in to change notification settings - Fork 528
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
Fix AOT build #2125
Fix AOT build #2125
Conversation
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.
Yay! Hopefully that will also fix the apk tests with AOT. Looking forward to it.
Looks good.
@radekdoulik yeah, it should... :) |
build |
86dbc3c
to
2f3bdbe
Compare
Logs are taken from the msbuild binlog for OK, this is beyond weird... BCL tests DID run fine, btw - they aren't reported in the github status for some reason:
and
Their logs are pulled from the device just fine, however the next APK tests in order (JcwGen_Tests and Locale_Tests) run but there's a failure downloading the test results all of the sudden:
and later on the same for:
On the off chance that it might be an issue with disk space in the emulator, I'm going to push one more commit which increases the data partition size to 8GB and we'll see how it goes. |
Hmm, it seems we don't create a new emulator image because some emulator's already running:
|
So the problem here appears to be that the first two test suites create the results file in the external storage which creates the files with permissions that make them world-readable. The last 2 result files are created in the private data store with stricter permissions that make them only owner-readable. Thus we can't access them. |
8a40bd3
to
478e5f5
Compare
After examining the emulator image I noticed that the successful test suites have the following entry in their <uses-sdk android:minSdkVersion="14" android:targetSdkVersion="22" /> and the failing ones have instead <uses-sdk android:minSdkVersion="14" /> Considering that the code which constructs the results path targets Android API 19 and newer in its crucial part, the theory is that the lack of the target SDK version causes Android to behave in a legacy manner - to return |
478e5f5
to
f59d896
Compare
Addition of |
So, the APK tests failed again, as before, despite the permission changes. However, it's not because the changes weren't effective - they were, as the tests run locally without a hitch (with a single failure across the entire test suite, not related to this PR). However, it seems the problem lies in that the emulator we start on the build bots is never stopped. In effect, the next PR build sees a running emulator and uses it for the tests, which results in:
And then further errors stemming from the one above. We fail to kill the emulator instance after the tests complete:
The |
build |
f59d896
to
3969f6d
Compare
build |
Made changes to the Jenkins PR builder to kill all emulator instances and remove all test AVDs before the build, let's wait to see if it makes a difference of any kind |
build |
3855e9a
to
7287526
Compare
This latest update rewrites the NOTE!! I wasn't able to test the changes on the runtime as currently my local msbuild is broken and fails to run our tests - thus the |
7287526
to
d9d6f70
Compare
Managed to fix my msbuild and run the tests. Just pushed an update that fixes a number of issues I've found. Locally the code appears to work fine:
Note the |
@@ -18,12 +18,10 @@ | |||
<AndroidManifest>Properties\AndroidManifest.xml</AndroidManifest> | |||
<AndroidSupportedAbis>armeabi-v7a;x86</AndroidSupportedAbis> | |||
<AndroidUseSharedRuntime>False</AndroidUseSharedRuntime> | |||
<AndroidUseLatestPlatformSdk>false</AndroidUseLatestPlatformSdk> | |||
<TargetFrameworkVersion>v8.1</TargetFrameworkVersion> | |||
<AndroidUseLatestPlatformSdk>true</AndroidUseLatestPlatformSdk> | |||
</PropertyGroup> |
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.
What is the reasoning for this change? Wouldn't it limit the tests, when nearly everything would target v8.1?
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.
This is just to bring parity with other tests - note however line 22, it renders line 21 ineffective by choosing whatever the latest platform is. The test projects must be somehow made to use the same project options, but that's a material for a different PR.
The `leeroy` make target failed with a "file not found" exception while attempting to invoke the `cross-arm` crosscompiler and the reason for this was that `cross-arm` was never built because of the `armeabi-v7a` target missing in the `ALL_AOT_ABIS` make variable in the `build-tools/scripts/BuildEverything.mk` file. This commit adds the missing target architecture and fixes the build. In addition to the above changes this commit modifies APK tests to: * target Android SDK 22 (to maintain uniformity across tests and also to make sure that the code which determines location of the on-device test result files returns the same location for all the tests - it relies on APIs introduced in API level 19) * request external storage premissions. This ensures that we can create the test result files in a location that's publically accessible. If access to external storage is denied our test code will create the results file in the application data directory and on Android 28 the file will be only readable by owner, thus making `adb pull` fail to download it.
d9d6f70
to
88b104c
Compare
Timeout while deploying APK to the emulator... will build again :( |
build |
The failed status is because of
But I see no actual error in the logs. Just the exit status. The failed tests are for the |
Fixes dotnet#2371 After dotnet#2125, the `RunInstrumentationTests` task stopped writing the logcat output to the `LogcatFilename` specified as parameter. That resulted in missing input files for the `ProcessLogcatTiming` task.
* [build-tools] Write the intrumentation timing output again Fixes #2371 After #2125, the `RunInstrumentationTests` task stopped writing the logcat output to the `LogcatFilename` specified as parameter. That resulted in missing input files for the `ProcessLogcatTiming` task. Just ask to append to the file. We don't need to check, whether the file exists.
The
leeroy
make target failed with a "file not found" exception whileattempting to invoke the
cross-arm
crosscompiler and the reason for this wasthat
cross-arm
was never built because of thearmeabi-v7a
target missing inthe
ALL_AOT_ABIS
make variable in thebuild-tools/scripts/BuildEverything.mk
file.
This commit adds the missing target architecture and fixes the build.