-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Src gen: use GetBestTypeByMetadataName instead of GetTypeByMetadataName #59092
Conversation
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsChange remaining source generator locations that use See #57349 This is a simple 1:1 method replacement; has been previously tested in System.Text.Json (which has a unit test) and by Roslyn. This may be ported to 6.0.
|
6e95673
to
2f8b5cd
Compare
...ons/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs
Outdated
Show resolved
Hide resolved
db8fb31
to
129b716
Compare
WASM issue likely caused by #51961 binlog section (crash on
console log:
|
cc @vargaz |
Something is killing the processes here, we need to figure out what and why before we can work on solving it. |
c06550c
to
4dda364
Compare
@steveharter have you checked against main after #59352 landed? |
@@ -19,6 +19,12 @@ | |||
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="'$(TargetOS)' == 'Browser' and '$(BuildAOTTestsOnHelix)' == 'true' and '$(RunDisabledWasmTests)' != 'true' and '$(RunAOTCompilation)' == 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you shouldn't need a new group for this, the exclusions 4 lines above should be enough but it looks like the paths are incorrect? I see
./Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj
./Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests.csproj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the path start with Microsoft.Extensions.Logging.Abstractions\tests
not Microsoft.Extensions.Logging\tests
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it's pointing at the wrong path and passing those old exclusions can probably be removed. I'll do it in a separate pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW Eric fixed that in #59608
f6cf633
to
e08eb3d
Compare
I just did a rebase and the same crash is occuring. |
57b2ae9
to
8cf4af3
Compare
8cf4af3
to
9b20c61
Compare
32c3b4b
to
8e2ca47
Compare
Looks like test failure is in regex source generator on checked coreclr. May not be related to this change, see #10680. cc @stephentoub
|
I see @stephentoub disabled that test here: 5d71e32 |
Yeah, the actual issue is the test is attributed with a SkipOnCoreClr attribute in order to avoid running on checked runtimes but the test isn't being skipped, as the attribute is being processed in a way different from what was expected. I disabled it while the attribute gets fixed. |
@stephentoub while you're here, this PR might be relevant for your RegEx source generator: runtime/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Parser.cs Lines 49 to 51 in 5bfae1c
edit, looks like I should expand my search by a couple lines. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Build failure in |
/backport to release/6.0 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1288124568 |
@steveharter backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Use GetBestTypeByMetadataName instead of GetTypeByMetadataName
Using index info to reconstruct a base tree...
M src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Use GetBestTypeByMetadataName instead of GetTypeByMetadataName
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
Change remaining source generator locations that use
Compilation.GetTypeByMetadataName
to an extension that addresses conflict cases. The guidance and extension implementation is from the Roslyn team.See #57349
This is a simple 1:1 method replacement; has been previously tested in System.Text.Json (which has a unit test) and by Roslyn.
This may be ported to 6.0.