-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Update SDK #33187
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
Update SDK #33187
Conversation
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.
Just blocked by another test that should be quarantined❔
Rerunning to see if it's flaky before I quarantine |
Test
versus:
Looks like the test has been quarantined before: #24755. It's concerning that it now fails consistently after updating the SDK - @davidfowl looks like last time it might have been related to trimming? #32166 (comment) @dsplaisted are you aware of any potentially related SDK changes? |
@agocke @jeffschwMSFT Do you know if something changed recently with linking that could have impacted this test? |
We certainly wouldn't expect CoreCLR failing to load. Could you gather a log by setting |
Looks like two failure modes are related. In the test run, I see that @agocke I was able to get some corehost trace logs but I couldn't make much sense of it:
Here's the full log if you want more details: |
This one's got me stumped -- no idea how trimming could affect this. @vitek-karas Any ideas? @JunTaoLuo If you comment out |
Guess we'll find out in the next CI run 😄 |
@agocke Looks like the problem is resolved when trimming is disabled. I'm going to revert the commit since it's not something we want to actually check in. |
This reverts commit 766ae32.
Yup, I'm cloning the repo now |
I backed up the linker multiple weeks and it looks like the same failure -- so I think the failure here isn't directly in the linker but might just be the interaction between the linker and the new runtime/sdk. Still investigating |
OK found the problem -- it looks like the CLR is failing to start because it's looking for |
We disable built-in COM support by default when trimming. The feature switch details for this is BuiltInComInteropSupport and can configure to set it to true in your project and see if that makes the test pass. |
@JunTaoLuo Any chance we can set If so, I think the problem is that the test is appropriately failing -- ASP.NET uses COM, but COM is not supported when trimming. This failure experience is obviously unfortunate though. We'll try to improve that. |
Sure thing, let's give it a shot to confirm. Thanks! |
While it might be true that ASP.NET needs COM (would be interesting to know what for - in the simple web app case), it doesn't explain this failure. In fact this is trivial to repro - just plain console hello world when fully trimmed will fail to start the runtime. I filed dotnet/runtime#53898 to track that. Setting |
fyi/ @dotnet/aspnet-build Still seeing some failures: |
Not sure about the Span problem, but this is definitely our bug at the moment, and the |
@@ -41,8 +41,6 @@ public async Task LinkedApplicationWorks() | |||
StatusMessagesEnabled = false | |||
}; | |||
|
|||
deploymentParameters.EnvironmentVariables["COREHOST_TRACE"] = "1"; |
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.
Why?
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.
I believe that was just added to help debug the CI issues: bb2e263 / #33187 (comment)
@@ -568,7 +568,7 @@ private string DebuggerToString() | |||
} | |||
else | |||
{ | |||
return _template.Substring(0, _index) + "|" + _template.Substring(_index); | |||
return string.Concat(_template.Substring(0, _index), "|", _template.Substring(_index)); |
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.
I would think it wants you to do
return string.Concat(_template.Substring(0, _index), "|", _template.Substring(_index)); | |
return string.Concat(_template.AsSpan(0, _index), "|", _template.AsSpan(_index)); |
@@ -151,5 +151,8 @@ dotnet_diagnostic.CA1827.severity = suggestion | |||
dotnet_diagnostic.CA1829.severity = suggestion | |||
# CA1834: Consider using 'StringBuilder.Append(char)' when applicable | |||
dotnet_diagnostic.CA1834.severity = suggestion | |||
# CA1845: Use span-based 'string.Concat' |
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.
FYI @captainsafia I made a small change to your 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.
Ah I see -- didn't realize these were meant to be ordered.
No description provided.