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.

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

[backport] Bump to mono:2019-10 (#7192) #7547

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

VincentDondain
Copy link
Contributor

@VincentDondain VincentDondain commented Dec 5, 2019

Miscellaneous

  • Fixed
    /Users/builder/jenkins/workspace/xamarin-macios/xamarin-macios/builds/mono-ios-sdk-destdir/ios-sources/external/linker/src/linker/Linker.Steps/OutputStep.cs(110,15): error CS0246: The type or namespace name ‘OutputException’ could not be found (are you missing a using directive or an assembly reference?) [/Users/builder/jenkins/workspace/xamarin-macios/xamarin-macios/tools/mmp/mmp.csproj]
  • Changed the name of the method that is used from linker. Because of this commit dotnet/linker@6be2677
  • Added OutputException.cs file on mtouch.csproj.
  • Removing enter_gc_safe and exit_gc_safe because now it's already gc_safe in this part of code, after a mono change.
  • Added known exceptions to LLVM exception list.
  • Needs ifdef because of this [System.Net.Http] Make HttpClientHandler on WatchOS always fallback mono/mono#17260.
  • Bump MIN_MONO_VERSION to 6.8.0.41 and point MIN_MONO_URL to the PR.
  • Add ENABLE_IOS=1 and ENABLE_MAC=1.
  • Added switch to disable packaged mono build
  • [Tests] Ignore tests that fail on 32b.
    Ignore the test on 32b, and filled issue: [2019-10][iOS] System.Reflection.Tests.RuntimeReflectionExtensionsTests.GetRuntimeMethod fails due to memory restrictions. mono/mono#17752
  • [Tests] Ignore a couple of tests causing OOM.
    Hopefully fixes https://github.com/xamarin/maccore/issues/1659 for good.
  • Ignore MM0135 test on Catalina+ because it needs Xcode 9.4.
  • [monotouch-test] Add null checks for teardown when test didn't run because of a too early OS version.
  • [CFNetwork]: Http 2.0 requires OS X 10.11 or later.
    Check whether _HTTPVersion2_0 is available and fallback to HTTP 1.1 otherwise.

Bring HttpClient from CoreFX

SocketsHttpHandler

CoreFX uses a completely new HttpClientHandler implementation called SocketsHttpHandler,
which you can find at https://github.com/dotnet/corefx/tree/release/3.0/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler.

Since this is not based on the web stack anymore, it does not use any of the related APIs such
as ServicePointManager or WebException.

Certificate Validation Changes

There is a new API called HttpClientHandler.ServerCertificateCustomValidationCallback.

The ServicePointManager.ServerCertificateValidationCallback is no longer invoked and on
certificate validation failure, AuthenticationException (from System.Security.Authentication)
is thrown instead of WebException.

At the moment, the NSUrlSessionHandler still uses it's own validation callback and also still
throws WebException on failure; we should probably look into making this consistent with the
other handlers.

Minor adjustments related to internal Mono APIs

  • HttpContent.SerializeToStreamAsync() is now protected (changed from protected internal).

    • src/Foundation/NSUrlSessionHandler.cs: changed overload accordingly.
    • src/System.Net.Http/CFContentStream.cs: likewise.
  • HttpHeaders.GetKnownHeaderKind() is an internal Mono API.
    There is a new internal API called System.Net.Http.PlatformHelper.IsContentHeader(key)
    which exists in both the old as well as the new implementation.
    The correct way of doing it with the CoreFX handler is
    HeaderDescriptor.TryGet (key, out var descriptor) && descriptor.HeaderType == HttpHeaderType.Content

Minor adjustments to tests

  • HttpClientHandler.MaxRequestContentBufferSize is now longer supported, you can set it to
    any non-negative value, the getter will always return 0.
    See https://github.com/dotnet/corefx/blob/c1778515a3bee34cc09c757b5563d0af0c8b1e99/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Core.cs#L18.

    • tests/linker/ios/link sdk/HttpClientHandlerTest.cs: removed assertion from test.
  • HttpMessageInvoker.handler is a protected private field - in the CoreFX handler, it is
    called _handler and private. This is accessed via reflection by some of the tests, which are
    now using the new name.

    • tests/mmptest/src/MMPTest.cs: here
    • tests/mtouch/MTouch.cs: here
  • tests/monotouch-test/System.Net.Http/MessageHandlers.cs:
    Adjust RejectSslCertificatesServicePointManager to reflect the certificate validation
    changes described above.

    • FIXME: There was an Assert.Ignore() related to NSUrlSessionHandler and macOS 10.10;
      I removed that to reenable the test because the description linked to an old issue in
      the private repo that was referenced by several "Merged" PR's, so it looked to me that
      this might have already been fixed - and I also didn't see why it would fail there.

* Fixed
`/Users/builder/jenkins/workspace/xamarin-macios/xamarin-macios/builds/mono-ios-sdk-destdir/ios-sources/external/linker/src/linker/Linker.Steps/OutputStep.cs(110,15): error CS0246: The type or namespace name ‘OutputException’ could not be found (are you missing a using directive or an assembly reference?) [/Users/builder/jenkins/workspace/xamarin-macios/xamarin-macios/tools/mmp/mmp.csproj]`
* Changed the name of the method that is used from linker. Because of this commit dotnet/linker@6be2677
* Added `OutputException.cs` file on `mtouch.csproj`.
* Removing enter_gc_safe and exit_gc_safe because now it's already gc_safe in this part of code, after a mono change.
* Added known exceptions to LLVM exception list.
* Needs `ifdef` because of this mono/mono#17260.
* Bump MIN_MONO_VERSION to 6.8.0.41 and point MIN_MONO_URL to the PR.
* Add ENABLE_IOS=1 and ENABLE_MAC=1.
* Added switch to disable packaged mono build
* [Tests] Ignore tests that fail on 32b.
    Ignore the test on 32b, and filled issue: mono/mono#17752
* [Tests] Ignore a couple of tests causing OOM.
    Hopefully fixes xamarin/maccore#1659 for good.
* Ignore `MM0135` test on Catalina+ because it needs Xcode 9.4.
* [monotouch-test] Add null checks for teardown when test didn't run because of a too early OS version.
* [CFNetwork]: Http 2.0 requires OS X 10.11 or later.
    Check whether `_HTTPVersion2_0` is available and fallback to HTTP 1.1 otherwise.

* xamarin#7346
* This bumps Mono to use mono/mono#17645 (which is the 2019-10 backport
of mono/mono#17628).
* The big user-visible change is in regards to certificate validation, everything below are just
some minor adjustments to tests.

CoreFX uses a completely new `HttpClientHandler` implementation called `SocketsHttpHandler`,
which you can find at https://github.com/dotnet/corefx/tree/release/3.0/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler.

Since this is not based on the web stack anymore, it does not use any of the related APIs such
as `ServicePointManager` or `WebException`.

There is a new API called `HttpClientHandler.ServerCertificateCustomValidationCallback`.
- https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclienthandler.servercertificatecustomvalidationcallback?view=netframework-4.8
- https://github.com/dotnet/corefx/blob/c1778515a3bee34cc09c757b5563d0af0c8b1e99/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Unix.cs#L154
- https://github.com/dotnet/corefx/blob/c1778515a3bee34cc09c757b5563d0af0c8b1e99/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Windows.cs#L383

The `ServicePointManager.ServerCertificateValidationCallback` is no longer invoked and on
certificate validation failure, `AuthenticationException` (from `System.Security.Authentication`)
is thrown instead of `WebException`.

At the moment, the `NSUrlSessionHandler` still uses it's own validation callback and also still
throws `WebException` on failure; we should probably look into making this consistent with the
other handlers.

* `HttpContent.SerializeToStreamAsync()` is now `protected` (changed from `protected internal`).
  - src/Foundation/NSUrlSessionHandler.cs: changed overload accordingly.
  - src/System.Net.Http/CFContentStream.cs: likewise.

* `HttpHeaders.GetKnownHeaderKind()` is an internal Mono API.
   There is a new internal API called `System.Net.Http.PlatformHelper.IsContentHeader(key)`
   which exists in both the old as well as the new implementation.
   The correct way of doing it with the CoreFX handler is
   `HeaderDescriptor.TryGet (key, out var descriptor) && descriptor.HeaderType == HttpHeaderType.Content`

* `HttpClientHandler.MaxRequestContentBufferSize` is now longer supported, you can set it to
  any non-negative value, the getter will always return 0.
  See https://github.com/dotnet/corefx/blob/c1778515a3bee34cc09c757b5563d0af0c8b1e99/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Core.cs#L18.
  - tests/linker/ios/link sdk/HttpClientHandlerTest.cs: removed assertion from test.

* `HttpMessageInvoker.handler` is a `protected private` field - in the CoreFX handler, it is
  called `_handler` and `private`.  This is accessed via reflection by some of the tests, which are
  now using the new name.
  - tests/mmptest/src/MMPTest.cs: here
  - tests/mtouch/MTouch.cs: here

* tests/monotouch-test/System.Net.Http/MessageHandlers.cs:
  Adjust `RejectSslCertificatesServicePointManager` to reflect the certificate validation
  changes described above.
  - FIXME: There was an `Assert.Ignore()` related to `NSUrlSessionHandler` and macOS 10.10;
    I removed that to reenable the test because the description linked to an old issue in
    the private repo that was referenced by several "Merged" PR's, so it looked to me that
    this might have already been fixed - and I also didn't see why it would fail there.
@spouliot spouliot added this to the d16-5 milestone Dec 5, 2019
@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    Failed to build src/
    Search for Comparing API & creating generator diff in the log to view the complete log.

@mandel-macaque
Copy link
Member

build

@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    Failed to build src/
    Search for Comparing API & creating generator diff in the log to view the complete log.
🔥 Test run failed 🔥

Test results

1 tests failed, 151 tests passed.

Failed tests

  • monotouch-test/watchOS 32-bits - simulator/Debug: Crashed

@spouliot spouliot added the do-not-merge Do not merge this pull request label Dec 10, 2019
@spouliot
Copy link
Contributor

2019-10 shows some failures, post merge (unclear why it was not hit before)

@VincentDondain
Copy link
Contributor Author

@spouliot so your comment here mono/mono#18221 (comment) seems to indicate this is not a new regression. Therefore should we still be holding the merge on the 32 bit tests?

@spouliot spouliot removed the do-not-merge Do not merge this pull request label Dec 17, 2019
@spouliot
Copy link
Contributor

@VincentDondain confirm with @chamons but, right now, there's nothing that seems specific to this bump (it's older or we hidden by other failures)

@chamons
Copy link
Contributor

chamons commented Dec 17, 2019

Yeah, I just talked to Sebastien but the new data suggests we're no longer worse that what we've released.

@chamons
Copy link
Contributor

chamons commented Dec 17, 2019

So I think this can go in and we can clean up more as we get fixes. Objections - @spouliot @rolfbjarne @VincentDondain @mandel-macaque @dalexsoto ?

@mandel-macaque
Copy link
Member

There is one known issue: https://github.com/xamarin/maccore/issues/581

I'm ok with merging this.

@mandel-macaque mandel-macaque merged commit fd3f86d into xamarin:d16-5 Dec 18, 2019
@VincentDondain VincentDondain deleted the d16-5-mono-2019-10 branch December 18, 2019 18:50
@rolfbjarne rolfbjarne added the not-notes-worthy Ignore for release notes label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-notes-worthy Ignore for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants