Skip to content

Conversation

@MichalStrehovsky
Copy link
Member

This reverts commit 759fecb.

Reverting it to fix the fallout in extra-platforms for now: #81409 (comment)

There are two problems that I can see:

  • Some tests are looking for the resource stream. This is not a customer scenario and not worried about it. We can just make it so that it's obvious to the compiler that the optimization needs to be disabled (e.g. add Type.GetType("System.SR, blah").GetMethod("GetResourceString"); to the test - this will trick the compiler into thinking the optimization has been defeated).
  • Event source looks like doesn't work. This is a bigger issue. I wonder if this also doesn't work with <UseSystemResourceKeys>true</UseSystemResourceKeys> specified. Will need to look into that more.

@ghost
Copy link

ghost commented Feb 1, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

This reverts commit 759fecb.

Reverting it to fix the fallout in extra-platforms for now: #81409 (comment)

There are two problems that I can see:

  • Some tests are looking for the resource stream. This is not a customer scenario and not worried about it. We can just make it so that it's obvious to the compiler that the optimization needs to be disabled (e.g. add Type.GetType("System.SR, blah").GetMethod("GetResourceString"); to the test - this will trick the compiler into thinking the optimization has been defeated).
  • Event source looks like doesn't work. This is a bigger issue. I wonder if this also doesn't work with <UseSystemResourceKeys>true</UseSystemResourceKeys> specified. Will need to look into that more.
Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Feb 1, 2023
Addresses first bullet of dotnet#81455.

Addresses 211 test failures to the tune of:

```
[FAIL] System.Xml.XmlSchemaValidatorApiTests.TCValidateWhitespace_String.WhitespaceInsideElement
System.Xml.Tests.VerifyException : GetManifestResourceStream() failed
   at System.Xml.Tests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x4c9
   at System.Xml.XmlSchemaValidatorApiTests.TCValidateWhitespace_String..ctor(ITestOutputHelper output) + 0x48
```
@MichalStrehovsky MichalStrehovsky added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 1, 2023
@MichalStrehovsky
Copy link
Member Author

Let's not merge this if #81463 and #81466 can get in instead. Leaving open just in case there's pushback.

MichalStrehovsky added a commit that referenced this pull request Feb 1, 2023
Addresses first bullet of #81455.

Addresses 211 test failures to the tune of:

```
[FAIL] System.Xml.XmlSchemaValidatorApiTests.TCValidateWhitespace_String.WhitespaceInsideElement
System.Xml.Tests.VerifyException : GetManifestResourceStream() failed
   at System.Xml.Tests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x4c9
   at System.Xml.XmlSchemaValidatorApiTests.TCValidateWhitespace_String..ctor(ITestOutputHelper output) + 0x48
```
@MichalStrehovsky MichalStrehovsky deleted the revertinline branch February 2, 2023 00:21
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-NativeAOT-coreclr NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants