Build bgen and the product assemblies using/referencing netcore3.1. #8070
Conversation
…mblies for .NET 5. I couldn't figure out how to make nuget install these into the system, so I decided to just download the package locally instead. This is just temporary until we get real .NET 5 reference assemblies.
Build bgen as a .netcoreapp 3.1 in addition to the managed executable we're already creating.
IKVM has a bug where it doesn't correctly compare assemblies, which means it can end up loading the same assembly (in particular any System.Runtime whose version > 4.0, but likely others as well) more than once. This is bad, because we compare types based on reference equality, which breaks down when there are multiple instances of the same type. So just don't ask IKVM to load assemblies that have already been loaded.
…turn to) with .NET5 behind a DOTNET_TODO define.
… running in .NET mode. Assemblies will still be found in .NET mode as long as the right -lib:<path> is passed to bgen.
…ves. IKVM will try to use reflection, which doesn't work in .NET 5.
This also meant adding support for running .NET tests in xharness. Some refactoring was done to extract common code to shared members, in order to avoid duplicating a lot of code.
rolfbjarne
requested review from
chamons,
dalexsoto,
mandel-macaque and
spouliot
as code owners
This comment has been minimized.
This comment has been minimized.
src/Compression/Compression.cs
Outdated
| public override IAsyncResult BeginRead (byte[] buffer, int offset, int count, AsyncCallback asyncCallback, object asyncState) => | ||
| TaskToApm.Begin(ReadAsync (buffer, offset, count, CancellationToken.None), asyncCallback, asyncState); | ||
|
|
||
| public override int EndRead (IAsyncResult asyncResult) => | ||
| TaskToApm.End<int> (asyncResult); | ||
| #endif // !DOTNET_TODO |
There was a problem hiding this comment.
So, this was a similar implementation to what it was done back in the day in the compression implementations of mono for zips. The APIs is not very used. If the base class implementation works (sync) is not a big issues since they should be using the Task version. I think it might be a good opportunity to get this out as outdated code.
There was a problem hiding this comment.
It is actually problematic. Sorry I didn't notice your comment and started new thread below.
src/Compression/Compression.cs
Outdated
| public override IAsyncResult BeginWrite (byte[] array, int offset, int count, AsyncCallback asyncCallback, object asyncState) => | ||
| TaskToApm.Begin(WriteAsync (array, offset, count, CancellationToken.None), asyncCallback, asyncState); | ||
|
|
||
| public override void EndWrite (IAsyncResult asyncResult) => TaskToApm.End (asyncResult); | ||
| #endif // !DOTNET_TODO |
There was a problem hiding this comment.
Same, I think it is safe to remove since we do not change the API. is less efficient but users should have moved (if we do have any of them in this API).
There was a problem hiding this comment.
If the base implementation works then let's add [Obsolete] to point customers into the best direction and call base (for compatibility)
src/Makefile
Outdated
| @@ -720,7 +932,6 @@ $(TVOS_BUILD_DIR)/AssemblyInfo.cs: $(TOP)/src/AssemblyInfo.cs.in $(TOP)/Make.con | |||
| $(Q) rm -f [email protected] | |||
| $(Q) touch $@ | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
extra white space change.
| yield return Path.Combine (GetSDKRoot (), "lib", "reference", "full"); | ||
| yield return Path.Combine (GetSDKRoot (), "lib", "mono", "4.5"); | ||
| } else if (target_framework == TargetFramework.Xamarin_Mac_4_5_System) { | ||
| yield return "/Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5"; |
There was a problem hiding this comment.
same code as before under a new if condition
There was a problem hiding this comment.
Add ?w=1 to the end of your URL to get a better diff in cases like this.
src/Compression/Compression.cs
Outdated
| public override IAsyncResult BeginWrite (byte[] array, int offset, int count, AsyncCallback asyncCallback, object asyncState) => | ||
| TaskToApm.Begin(WriteAsync (array, offset, count, CancellationToken.None), asyncCallback, asyncState); | ||
|
|
||
| public override void EndWrite (IAsyncResult asyncResult) => TaskToApm.End (asyncResult); | ||
| #endif // !DOTNET_TODO |
There was a problem hiding this comment.
If the base implementation works then let's add [Obsolete] to point customers into the best direction and call base (for compatibility)
| yield return Path.Combine (GetSDKRoot (), "lib", "reference", "full"); | ||
| yield return Path.Combine (GetSDKRoot (), "lib", "mono", "4.5"); | ||
| } else if (target_framework == TargetFramework.Xamarin_Mac_4_5_System) { | ||
| yield return "/Library/Frameworks/Mono.framework/Versions/Current/lib/mono/4.5"; |
There was a problem hiding this comment.
same code as before under a new if condition
This comment has been minimized.
This comment has been minimized.
This way we don't have to change PATH on bots. It always makes it easier to use a different location for the executable in the future if we wished to.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/Compression/Compression.cs
Outdated
| [Obsolete ("Use 'ReadAsync' instead. This method is synchronous.")] | ||
| #endif | ||
| public override IAsyncResult BeginRead (byte[] buffer, int offset, int count, AsyncCallback asyncCallback, object asyncState) | ||
| { |
There was a problem hiding this comment.
A similar Mono change recently broke our product. This is quite problematic because the base implementation of BeginRead/BeginWrite/EndRead/EndWrite. It is calling ReadAsync/WriteAsync internally but it is geared towards seekable streams where Position matters and it only allows single async call at a time. This is particularly problematic with [wrapped] network streams where a common coding pattern expect both async read and write to be working at the same time.
Note that in .NET Core all non-seekable network streams or streams that wrap around network streams (GzipStream, HttpBaseStream, QuicStream, PipeReaderStream, PipeWriterStream, ReadOnlyMemoryStream, BrotliStream, SslStream, ...) use the TaskToApm pattern.
I think the base implementation in Stream.BeginRead/BeginWrite could probably be fixed/extended to work for these scenarios but that's not the case today. Until that happens I think it's better to use a private copy of TaskToApm here.
There was a problem hiding this comment.
I filed an issue at dotnet/runtime#33447 to seek better solution. If it gains some traction it may make sense to remove the overrides of BeginRead/BeginWrite/EndRead/EndWrite altogether.
src/Makefile
Outdated
|
|
||
| GENERATOR_FLAGS=-process-enums -core -nologo -nostdlib -noconfig -native-exception-marshalling --ns:ObjCRuntime | ||
|
|
||
| DOTNET_FLAGS=/noconfig /nostdlib+ /deterministic /features:strict /nologo /target:library /debug /unsafe /define:DOTNET /define:DOTNET_TODO \ |
There was a problem hiding this comment.
Any particular reason to go with DOTNET define instead of the standard NETCOREAPP one?
There was a problem hiding this comment.
It seems just NET will be the defined for the future: https://github.com/terrajobst/designs/blob/net5/accepted/2020/net5/net5.md#preprocessor-symbols, and NETCOREAPP is only defined for backwards compatibility. I'll change it to NET to be consistent.
There was a problem hiding this comment.
Fixed to use NET (and NET_TODO).
|
Build failure Test results71 tests failed, 23 tests passed.Failed tests
|
src/Makefile
Outdated
| GENERATOR_FLAGS=-process-enums -core -nologo -nostdlib -noconfig -native-exception-marshalling --ns:ObjCRuntime | ||
|
|
||
| DOTNET_FLAGS=/noconfig /nostdlib+ /deterministic /features:strict /nologo /target:library /debug /unsafe /define:DOTNET /define:DOTNET_TODO \ | ||
| /r:$(DOTNET_BCL_DIR)/mscorlib.dll \ |
There was a problem hiding this comment.
This doesn't sound right. There should be no mscorlib, it is System.Private.CoreLib and it should not be referenced directly. Last time I tried the assemblies needed were only those:
- System.Collections.dll
- System.Drawing.Primitives.dll
- System.Linq.dll
- System.Net.Http.dll
- System.Runtime.dll
- System.Runtime.InteropServices.dll
- System.Security.Cryptography.X509Certificates.dll
- System.Xml.dll
- System.Xml.ReaderWriter.dll
There was a problem hiding this comment.
mscorlib.dll is there:
$ ls -la /usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1/mscorlib.dll
-rw-r--r-- 1 root wheel 55880 Nov 15 09:36 /usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1/mscorlib.dllthat said, I did not try to minimize the list of assemblies, I found a set that worked, and went with that. I'll try your list to see if that works.
There was a problem hiding this comment.
It is there as a facade for compatibility with .NET Framework libraries, not for consumption from new code.
There was a problem hiding this comment.
I wasn't able to remove many assemblies from the list, I had to add most of them back when I tried your list of assemblies. This is what happened after the initial removal and after adding them back one by one: https://gist.github.com/rolfbjarne/7f515ae2ac7c0f712b3d70f0ff985549
There was a problem hiding this comment.
Interesting. I'll check later what I did differently, perhaps I disabled OpenTK and used slightly different build options. Nevertheless, as long as it doesn't depend on compatibility facade assemblies I am happy. 👍
…s compatible implementation of [Begin|End]|[Read|Write].
|
|
||
| // | ||
| // Xamarin: | ||
| // This is a copy of this file: https://github.com/dotnet/runtime/blob/25d316cefafc4c3884f38acf7bdef971d2b9fe4b/src/libraries/Common/src/System/Threading/Tasks/TaskToApm.cs |
There was a problem hiding this comment.
Since this file is a copy of a file from elsewhere, it does not follow our style guidelines. This is expected.
|
Build failure |
|
Build failure Test results3 tests failed, 91 tests passed.Failed tests
|
|
build |
|
Build failure Test results2 tests failed, 92 tests passed.Failed tests
|
|
Build failure Test results2 tests failed, 92 tests passed.Failed tests
|
|
Test failures are unrelated
|
Build bgen and the product assemblies using/referencing netcore3.1. This is opt-in for now (although bots have been opted in by default), and won't affect normal developer builds (it slows it down).
A few minor changes to the product assemblies were required, some will be revisited, some are expected. Issues will be created for the ones that will be revisited (unless an issue already exists).
This pull request is best consumed commit-by-commit.