Repository navigation
Fix a few more API compat inconsistencies - #68834
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsI encountered a few more Attribute inconsistencies on top of what I previously fixed in #68432 This will be necessary to pick up dotnet/arcade#9168 once it flows to runtime
|
|
|
||
| if (!nextAuthorityPersist) | ||
| { | ||
| #if !ILLUMOS && !SOLARIS |
There was a problem hiding this comment.
I'm not sure if we need to drag it here since neither OS is really supported.
cc: @am11
There was a problem hiding this comment.
If I understand it correctly, we were previously using UnknownUnix.cs pattern and not anymore (in favor of supported / unsupporter platform attributes). The compat tool is indicating that this call shouldn't be compiled on unknown unix.
There was a problem hiding this comment.
@ericstj, was this auto-generated? I know that we define TARGET_{OS} constants for OS specific projects, but I am not sure if we define {OS}.
There was a problem hiding this comment.
There was a problem hiding this comment.
System.Private.CoreLib doesn't use the TargetPlatform feature which requires platform defines to be manually specified.
There was a problem hiding this comment.
If some of our libraries manually pass in TARGETS_* define constants based on the target platform, then those should probably be cleaned-up (except for CoreLib).
Yup, there is #39174 tracking review of defined vs. used constants. We are using TARGET_{OS} and TARGET_{ARCH}, but then we also had examples of TARGETS_XXX (plural) and TargetsXxx. 😁
There was a problem hiding this comment.
The compat tool is indicating that this call shouldn't be compiled on unknown unix.
The compiler is actually flagging the illumos and solaris builds of S.N.Http as calling unsupported API. I chose these defines based on what I saw being passed into the compiler. I had to ifdef to avoid compiler warnings as error.
@am11 @wfurt can you please let me know if you'd like anything different here? I'm trying to keep this change as minimal as possible. I could just pragma suppress the compiler warnings. I chose not to do that since these APIs were actually throwing on the unsupported platforms and I didn't want to ignore that signal.
There was a problem hiding this comment.
Minimal change looks good to me. There are only a few places which require such conditions, the rest of places are filtered out via Supported/Unsupported OS attributes.
There was a problem hiding this comment.
I think it is ok. I wish we did not have to drag #ifs to the HTTP code but I understand the reasoning. It would be nice if the tool can simply ignore unfinished/incomplete targets like Solaris of FreeBSD.
There was a problem hiding this comment.
I was imagining a slightly larger refactor with partial files might also work here but I didn't want to make the diff that large.
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
System.Diagnostics.Process part LGTM, thank you @ericstj !
Not related to your changes, but to the problem that have caused it: I think that we should try to avoid having multiple os-specific .cs files that define public types and properties with [UnsupportedOSPlatform] attributes. It forces us to duplicate all the annotations and also xml docs.
IMO it's better to have a public facade with all the annotations and docs, that calls os-specific internal helper.
|
Merging in as this starts to impact the Arcade ingestion PR. |
ghost
locked as resolved and limited conversation to collaborators
6 participants
I encountered a few more Attribute inconsistencies on top of what I previously fixed in #68432
This will be necessary to pick up dotnet/arcade#9168 once it flows to runtime