Skip to content

Conversation

@jonpryor
Copy link
Contributor

Bumps to cecil/mono-2017-10/76ffcdab.
This is consistent with cba77e9, which hopefully means we won't get
the unit test failures seen in PR #1101.

@jonpryor
Copy link
Contributor Author

Doubly weird here is that it built on macOS+msbuild, but failed to build on macOS+xbuild.

The only difference between those environments should be around whether unit tests are executed, not the build itself.

Very puzzling. :-(

I've restarted the macOS+xbuild build. Let's see if Take 2 fairs any better.

@jonpryor
Copy link
Contributor Author

Doubly weird here is that it built on macOS+msbuild

Different machines. Because of course it's some form of machine/environment difference.

The passing macOS+msbuild build was on xam-mac-mini-6, while the failing macOS+xbuild build was on xam-mac-mini-7.

@grendello
Copy link
Contributor

This should fix the build mono/mono#6260. The failure is caused by:

CMake Error at /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/external/mono/external/boringssl/util/android-cmake/android.toolchain.cmake:800 (message):
                                                  Specified Android native API level 'android-12' is not supported by your
                                                  NDK/toolchain.

@jonpryor
Copy link
Contributor Author

The underlying cause of the failure is actually PR #1105 (!).

The $(AndroidNdkDirectory) contents suffers from a similar version of the "forward only" problem described in commit b28856f. PR #1105 bumps to Android NDK r16, and it ran on xam-mac-mini-7. When it ran there, it updated $(AndroidNdkDirectory) -- $HOME/android-toolchain/ndk to contain NDK r16, but nothing else supports r16 yet. (That's what PR #1105 is for: adding some support for NDK r16!)

@grendello
Copy link
Contributor

@jonpryor
Copy link
Contributor Author

As with PR #1101, there are 98 unit test failures. This requires further investigation.

For starters, what about a smaller test case? :-)

# assume a complete xamarin-android build...
$ make prepare all
$ cd samples/HelloWorld
$ ../../bin/Debug/bin/xabuild /p:Configuration=Release /p:BundleAssemblies=True /p:TargetFrameworkVersion=v8.1 /v:diag > b.txt

Fortunately, it fails:

error : Error executing task GenerateJavaStubs: Could not load assembly 'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'. Perhaps it doesn't exist in the Mono for Android profile?

This matches e.g. the Xamarin.Android.Build.Tests.BuildTest.BuildMkBundleApplicationRelease / Debug error, so we're on the right track.

Reading the build log, this seems odd: @(_ResolvedAssemblies) is incomplete, as it is missing Mono.Android.dll:

GenerateJavaStubs Task
  ...
  ResolvedAssemblies:
    obj/Release/android/assets/HelloWorld.dll
    obj/Release/android/assets/System.dll
    obj/Release/android/assets/System.Xml.dll
    obj/Release/android/assets/System.Core.dll
    obj/Release/android/assets/mscorlib.dll
    obj/Release/android/assets/System.Runtime.Serialization.dll

@(_ResolvedAssemblies) comes from the _PrepareAssemblies target, and is basically the contents of obj/Release/android/assets/*.dll. There is no obj/Release/android/assets/Mono.Android.dll.

The contents of obj/Release/android/assets in turn comes from the _LinkAssembliesShrink target/<LinkAssemblies/> task:

LinkAssemblies Task
  ...
  ResolvedAssemblies:
    ...
    obj/Release/linksrc/Mono.Android.dll
    ...

The <LinkAssemblies/> task reads @(ResolvedAssemblies), which does include Mono.Android.dll, and obj/Release/linksrc/Mono.Android.dll does exist.

We can thus conclude that, for whatever reason, the <LinkAssemblies/> task is not creating the output file obj/Release/android/assets/Mono.Android.dll.

To validate this conclusion, if we use a stable Xamarin.Android:

$ xbuild  /p:Configuration=Release /p:BundleAssemblies=True /p:TargetFrameworkVersion=v8.0 /v:diag > b.txt

Then the file obj/Release/android/assets/Mono.Android.dll is created.

@jonpryor
Copy link
Contributor Author

As an educated shot-in-the-dark, I suspect that the linker bump within mono/2f18e7dd is responsible. If I leave the mono/external/cecil commit as-is, but reset mono/external/linker to use linker/7c4dab5f, then <LinkAssemblies/> works as it should, an no error is generated.

Fortunately there are only 14 commits in the linker bump. One manual "bisect" later, and the breakage appears to be due to linker/4d2362d8.

Looking at that commit...it's not immediately apparent why that commit breaks things, but the preceding commit -- linker/7904df9f -- works as desired.

Some manual testing later, and the cause is this line:

@@ -247,7 +253,7 @@ namespace Mono.Linker {
                        } else if (IsCore (name)) {
                                action = _coreAction;
                        } else {
-                               action = AssemblyAction.Link;
+                               action = _userAction;
                        }

Revert that one line, so that action = AssemblyAction.Link, and the <LinkAssemblies/> task behaves as it used to.

jonpryor added a commit to jonpryor/linker that referenced this pull request Dec 16, 2017
Commit 4d2362d [broke the xamarin-android linker][0], as assemblies
which should have been linked were no longer being linked.

[0]: dotnet/android#1112 (comment)

The apparent cause is that before commit 4d2362d, the "default"
action would be `AssemblyAction.Link`. Starting with 4d2362d, the
default action was instead `AssemblyAction.Skip`, as
`LinkContext._userAction` would be zero-initialized during
construction, and `AssemblyAction.Skip` is the default (0) enum value.

Set `AssemblyAction._userAction` in the constructor to
`AssemblyAction.Link`, so that pre-4d2362d8 linker behavior is
retained. This should allow the Xamarin.Android linker to work without
further changes.
@jonpryor
Copy link
Contributor Author

jonpryor commented Dec 16, 2017

PR for fix: dotnet/linker#203

Unfortunately, xamarin-android/master doesn't build against linker/master, as files were moved:

CSC: error CS2001: Source file '…/xamarin-android/external/mono/external/linker/linker/Mono.Linker.Steps/SweepStep.cs' could not be found.

Looks like it's linker/10f4163f which kills us. :-(

That said, my PR applies cleanly atop linker/404f7316, and if I rebuild Xamarin.Android.Build.Tasks then I'm able to build HelloWorld:

$ (cd src/Xamarin.Android.Build.Tasks/ ; xbuild)
$ (cd samples/HelloWorld ; ../../bin/Debug/bin/xabuild /p:Configuration=Release /p:BundleAssemblies=True /p:TargetFrameworkVersion=v8.1 /v:diag > b.txt)

The above results in no build errors.

Bumps to cecil/mono-2017-10/76ffcdab.
Bumps to linker/master/404f7316

Set the new [`LinkContext.UserAction` property][0] to
`AssemblyAction.Link`, as the default value of `AssemblyAction.Skip`
causes [98 unit tests to fail][1].

[0]: dotnet/linker@4d2362d
[1]: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2264/testReport/
@jonpryor jonpryor force-pushed the jonp-bump-mono-63e8dc6e branch from 2f367a6 to 845816a Compare December 16, 2017 14:39
@jonpryor
Copy link
Contributor Author

dotnet/linker#203 was rejected, so the correct fix is for Xamarin.Android to set the LinkContext.UserAction property to AssemblyAction.Link (the previous behavior).

PR has been updated accordingly.

@jonpryor jonpryor merged commit 4f82b6a into dotnet:master Dec 16, 2017
jonpryor added a commit that referenced this pull request Dec 17, 2017
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants