Skip to content
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.

Sign up for GitHub

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

Jump to bottom

R8 integration #1489

Closed
wants to merge 2 commits into from
Closed

R8 integration #1489

wants to merge 2 commits into from

Conversation

atsushieno
Copy link
Contributor

(testing CI builds; needs more work)

@atsushieno atsushieno added do-not-merge PR should not be merged. use-merge-commit Normally we squash-and-merge PRs. Use this label so we instead use a merge commit. labels Mar 29, 2018
@jonpryor
Copy link
Member

General guideline for sanity (which we're breaking in a couple places, and should fix, but let's not make it worse):

  • build-tools is for things which will not be redistributed, e.g. things which aren't part of the macOS .pkg or Windows .vsix
  • src and tools are for things which will be redistributed as part of the SDK.

As such, build-tools/r8 should be src/r8.

@atsushieno
Copy link
Contributor Author

We distribute mono, libzip and LibzipSharp, and they are under build-tools.

atsushieno added a commit to atsushieno/xamarin-android that referenced this pull request Mar 30, 2018
@atsushieno
Move a couple of soure build scripts from build-tools to src.
a04aa4d 
dotnet#1489 (comment)
complains about the additions of new sources to build-tools is wrong because
they are not "build-tools", mentioning that the existing sources are wrong.

Wait, then, why not fix it right now.

Therefore, fixing this.
`mono-runtimes`, `libzip` and `libzip-windows` are moved to src.
atsushieno added a commit to atsushieno/xamarin-android that referenced this pull request Mar 30, 2018
@atsushieno
Move a couple of soure build scripts from build-tools to src.
09b66fa 
dotnet#1489 (comment)
complains that the additions of new sources to build-tools is wrong because
they are not "build-tools", mentioning that the existing sources are wrong.

Wait, then, why not fix it right now.

Therefore, fixing this.
`mono-runtimes`, `libzip` and `libzip-windows` are moved to src.
@atsushieno atsushieno mentioned this pull request Mar 30, 2018
Merged
@atsushieno
Copy link
Contributor Author

#1495

@jonpryor
Copy link
Member

#1495 has been merged.

For your next trick you should move src/Xamarin.Android.Tools.BootstrapTasks into build-tools. ;-)

@jonpryor
Copy link
Member

This PR also needs to add external/depot_tools.tpnitems and external/r8.tpnitems files, which contains license information for these added external repos. See e.g. external/proguard.tpnitems.

@atsushieno atsushieno force-pushed the r8-integration branch 2 times, most recently from edaf831 to 93dfb46 Compare April 4, 2018 10:41
@atsushieno atsushieno mentioned this pull request Apr 4, 2018
Closed
@atsushieno
Copy link
Contributor Author

For your next trick you should move src/Xamarin.Android.Tools.BootstrapTasks into build-tools. ;-)

I don't quite understand. Why don't you do it by yourself? Just do it. I will review your PR.

@atsushieno atsushieno force-pushed the r8-integration branch 5 times, most recently from 9c3da1a to 272e22c Compare April 6, 2018 16:09
@atsushieno
Copy link
Contributor Author

There is no documentation about those .tpnitems file format (MSBuild Item description) and I'm not an ESPer. What do people specify values at <ThirdPartyNotice Include="XXX/YYY"> ?

@jonpryor
Copy link
Member

There is no documentation about those .tpnitems file format (MSBuild Item description) and I'm not an ESPer.

I should write up some documentation.

In the meantime, try to copy what already exists. :-D

What do people specify values at

Generally, it would be "organization-name/repo-name" like github.com convention, e.g. grendello/libZipSharp, mono/boringssl, etc.

However, there are no requirements on the value; it's just a short (non-empty) string that is used as a "section header" within the ThirdPartyNotices.txt file.

For this PR, I'd suggest values of googlesource/depot_tools and googlesource/r8.

@jonpryor
Copy link
Member

Please update src/Mono.Android/Test/Mono.Android-Tests.csproj so that it sets $(AndroidUseR8)=True/etc., so that at least one on-device tests uses r8 & d8.

@jonpryor
Copy link
Member

@eno: the macOS+xbuild PR Build fails with:

Xamarin.Android.Common.targets: error : r8.jar does not exist (R8JarPath property has to point to a valid file (current value: '/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/tools/scripts/../../bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/r8.jar').

Additionally, that error message is the only "match" for r8.jar in the build log.

Further reading the log, it appears that the _BuildR8 target is never executed, nor is _SetupDepotTools.

Offhand, I'm not immediately sure why it's not being built.

I would suggest that you update r8.csproj to be "closer in spirit" to e.g. src/monodroid/monodroid.csproj, by adding a $(Platform) definition, $(TargetFrameworkVersion), $(OutputPath), etc., along with importing $(MSBuildBinPath)\Microsoft.Common.targets.

jonpryor added a commit that referenced this pull request May 1, 2018
@jonpryor
[tests] Report errors last in RunApkTests (Take 2!) (#1610)
4105f2c 
Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2963/testReport/

A build for PR #1489 ran 1110 tests; it *should* have run over 35,000.
Where are the missing tests?

What's missing are the `.apk` on-device tests; BCL tests make up
*most* of our expected test count.

Analysis of the build log shows that the on-device tests *are*
running, with test results being downloaded:

	…/android-toolchain/sdk/platform-tools/adb -s emulator-5570  pull "/data/data/Xamarin.Android.Bcl_Tests/files/.__override__/TestResults.xUnit.xml" "…/xamarin-android/tests/../bin/TestDebug/TestResult-Xamarin.Android.Bcl_Tests.xunit.xml"
	...
	/data/data/Xamarin.Android.Bcl_Tests/files/.__override__/TestResults.xUnit.xml: 1 file pulled. 53.5 MB/s (3273412 bytes in 0.058s)

but those results don't appear on the `testReport` URL.

The problem is that the `RenameApkTestCases` target is never being
executed, and the `RenameApkTestCases` is responsible for copying the
`TestResult*` files where Jenkins looks for them:

	Done building target "ReleaseAndroidTarget" in project "RunApkTests.targets" -- FAILED.:

The `RenameApkTestCases` target executes *after*
`ReleaseAndroidTarget`, but because `ReleaseAndroidTarget` failed, the
`RenameApkTestCases` target isn't executed.

Meanwhile, `ReleaseAndroidTarget` reports an error to ensure that
errors aren't overlooked; see commit 3b893cd.

Allow the `RenameApkTestCases` target to execute by adding a new
`ReportComponentFailures` target. The `ReleaseAndroidTarget` will no
longer report errors, allowing the `RenameApkTestCases` target to
execute, while if any tests *do* fail, the `ReportComponentFailures`
target will generate an appropriate error.

Furthermore, update all the `<MSBuild/>` invocations within
`RunTests.targets` and add `ContinueOnError="ErrorAndContinue"` to all
of them, to help ensure that we run all the tests we have, even if
previous tests fail.
@atsushieno atsushieno force-pushed the r8-integration branch 2 times, most recently from 1e9a437 to c4c0e9f Compare May 23, 2018 01:42
This was referenced Jun 1, 2018
Closed
Closed
@atsushieno
Copy link
Contributor Author

build

@atsushieno
Copy link
Contributor Author

build

@atsushieno
Copy link
Contributor Author

#1792 is still blocking further investigation.

@atsushieno
Copy link
Contributor Author

Locally make run-nunit-tests and make run-apk-tests showed no failure.

@atsushieno
Copy link
Contributor Author

build

@atsushieno
Copy link
Contributor Author

Why is this Azure problem triggered only here? I'm seeing other newer builds pass and I'm not happy to see this.

make: *** [prepare-external] Error 1
04:03:30 Build step 'Execute shell' marked build as failure
04:03:31 MicrosoftAzureStorage - Uploading files to Microsoft Azure
04:03:31 MicrosoftAzureStorage - Failed to upload any build artifacts to Azure Storage 
04:03:31 Verify the list of files to upload and that the Ant glob syntax is correct
04:03:31 
ERROR: MicrosoftAzureStorage - Error occurred while uploading to Azure - xamjenkinsartifact
04:03:31 java.io.IOException: MicrosoftAzureStorage - Failed to upload any build artifacts to Azure Storage 
04:03:31 Verify the list of files to upload and that the Ant glob syntax is correct
04:03:31 	at com.microsoftopentechnologies.windowsazurestorage.WAStoragePublisher.perform(WAStoragePublisher.java:326)

@atsushieno
Copy link
Contributor Author

sigh, *.sln merge failure.

@atsushieno
Copy link
Contributor Author

The latest build failure does not make sense to me at all:

17:44:58 MSBuild auto-detection: using msbuild version '15.0' from '/usr/lib/mono/msbuild/15.0/bin'.
17:44:59 Error parsing solution file at /mnt/jenkins/workspace/xamarin-anroid-linux-pr-builder/xamarin-android/Xamarin.Android.sln: Exception has been thrown by the target of an invocation.  Error parsing the nested project section in solution file. A project with the GUID "{D93CAC27-3893-42A3-99F1-2BCA72E186F4}" is listed as being nested under project "{E351F97D-EA4F-4E7F-AAA0-8EBB1F2A4A62}", but does not exist in the solution.  /mnt/jenkins/workspace/xamarin-anroid-linux-pr-builder/xamarin-android/Xamarin.Android.sln
17:44:59

This is proprietary project, which I never touched. Why does it fail only for me!?

Anyways I'm removing this entry.

@atsushieno atsushieno force-pushed the r8-integration branch 2 times, most recently from fc41d96 to c2d2102 Compare July 2, 2018 07:12
@atsushieno
Copy link
Contributor Author

I'm really tired of these failing PR builders which keeps intermittently fail for irrelevant issues. Can this PLEASE be merged NOW?

@atsushieno
Copy link
Contributor Author

After some investigation jonp found that Mono.Android-Tests failed (not even starting tests) due to bogus app build triggered by r8 change and the details are simply suppressed, therefore looking like failing for no reason. The build tasks likely have issues and should be fixed first.

@atsushieno
Copy link
Contributor Author

Development is blocked by https://xamarinhq.slack.com/files/U03CZ68AY/FBKAEBB0D/screenshot_from_2018-07-05_14-18-37.png

@atsushieno
[r8] add r8 and depot_tools submodules.
d5121da 
depot_tools is required to build r8. On Windows it is however not used but
a binary executable zip will be downloaded and used instead.
(see https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html#_setting_up for details)

r8 build is not integrated in the primary Makefile and .sln yet.

(r8 is not a "build tool" but we build msbuild tasks dependencies there too.)
@atsushieno
[msbuild][r8] add r8 support to alter dx, proguard and desugar.
21d9c27 
See dotnet#1423 for details.

multidex support is not done yet (needs to investigate what's expected there).

Xamarin.Android.sln has a lot of changed lines because they had wrong
ProjectTypeGuids (maybe regression from changes to .csproj from .*proj).
@jonathanpeppers jonathanpeppers mentioned this pull request Jul 31, 2018
Merged
@jonathanpeppers
Copy link
Member

Closing this one in favor of #2019, where I'll be taking this over.

I'll keep @atsushieno's commits there intact, and hopefully we can use a merge commit so we keep that history.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dellis1972 dellis1972 dellis1972 left review comments

@jonpryor jonpryor Awaiting requested review from jonpryor

Assignees
No one assigned
Labels
use-merge-commit Normally we squash-and-merge PRs. Use this label so we instead use a merge commit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@atsushieno @jonpryor @jonathanpeppers @dellis1972