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

Revert "Inline resource strings in the compiler (#80896)" #81455

Closed
wants to merge 1 commit into from

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