-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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 .NET SDK to 10.0.100-alpha.1.24609.2 #59389
Conversation
Update .NET SDK to version 10.0.100-alpha.1.24609.2. --- updated-dependencies: - dependency-name: Microsoft.NET.Sdk dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
Auto-approving SDK update.
These failures seem related to #58721. Looking into it. |
New build errors:
@adityamandaleeka, could someone from your team decide how to handle this? |
Issue for the networking team to look into this more and double check the changes made here to unstuck the build are good: |
@@ -118,11 +118,17 @@ internal unsafe IISHttpContext( | |||
public SslProtocols Protocol { get; private set; } | |||
public TlsCipherSuite? NegotiatedCipherSuite { get; private set; } | |||
public string SniHostName { get; private set; } = default!; | |||
[Obsolete("Obsolete on SslStream.")] |
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.
Do we want to use the same DiagId on these obsoletions as the ones used on SslStream? That way we (and users) don't need to suppress both 618
and SYSLIB0058
.
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.
We don't use DiagId with obsolete attributes in ASP.NET Core: https://github.com/search?q=repo%3Adotnet%2Faspnetcore+%22%5BObsolete%22&type=code&p=1
I don't think so.
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.
Has there ever been a case where we need to Obsolete our API because of an underlying Obsoletion in the runtime libraries?
It feels like this is just an extension of obsoleting the underlying SslStream enums, so it feels like we should use the same DiagId.
cc'ing some devs who might have opinions - @terrajobst @bartonjs @rzikm @wfurt @halter73
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 agree with @eerhardt. There are two options to deal with obsoletions in underlying APIs:
- Suppression. If the obsoletion is in the implementation and we intend to shield the consumer by eventually moving off of the obsolete API, then suppression on our end is fine.
- Mark as obsolete. If the obsoletion is part of the API or conceptually tied to its semantics, then we should obsolete the higher level API.
For (2) it makes little sense to me to use a different diagnostic ID, because they are logically tied to the same obsoletion. If a customer suppresses one, I think they have reasonable expectation to suppress the other.
And generally speaking I believe we should virtually never obsolete without a diagnostic ID. The standard diagnostic for obsoletion applies way too broadly so we generally want to avoid cases where customers feel like should suppress all obsoletions.
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.
Ok, I added the diagnostic ID.
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.
Thanks, appreciate it!
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
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.
The SslStream change looks good to me.
Updates the .NET SDK to version
10.0.100-alpha.1.24609.2
, which includes version10.0.0-alpha.1.24570.9
of the .NET runtime.This pull request was auto-generated by GitHub Actions.