Repository navigation
Missing [un]supported APIs annotations in System.Net.NetworkInformation #51301
Description
System.Net.NetworkInformation assembly on platforms where it's supported has over 80 PNSE methods but no [un]supported APIs annotation. This degrades user experience when working with the APIs as a significant portion of the assembly is not supported cross-platform.
It should be part of #47228
/cc @jeffhandley @buyaa-n
Activity
marek-safar commentedon Apr 20, 2021
@buyaa-n any thoughts on why your tool didn't find these?
buyaa-n commentedon Apr 20, 2021
I run the tool only for cross-platform builds, seems for System.Net.NetworkInformation there is no source built for cross-platform (or when
Condition="'$(TargetsAnyOS)' != 'true'") that is why there is nothing detected. From the existing TFMs seems it is supported onwindows; Linux; OSX; iOS; tvOS; FreeBSD; illumos; Solaris;so we can add those platforms into the assembly level supported list. If you are expecting individual APIs supposed to be annotated then i have not detected them for now, I am updating the tool to detect incompatible PNSEs in targeted builds, almost finished then, need to run it and analyze the results.karelz commentedon May 6, 2021
@buyaa-n assigning to you as you are working on the tool update. Is there any ETA or work item tracking it on Analyzer side?
buyaa-n commentedon May 6, 2021
The issue for adding the analyzer is closed in API review as adding it in SKD or runtime repo will be very noisy, so the analyzer I am running is only for one-time detection. But there is a related issue for detecting unsupported APIs and annotating them
karelz commentedon Jun 25, 2021
@buyaa-n @jeffhandley is it still on your radar for 6.0? (just double checking it was not forgotten as it is in Networking area ...)
karelz commentedon Jun 30, 2021
@buyaa-n @jeffhandley ping?
jeffhandley commentedon Jul 8, 2021
@karelz This is still on our radar, but we're not going to be able to look at it again before the Preview 7 snap; we'll have to follow up before RC1.
buyaa-n commentedon Jul 24, 2021
Somehow when running build from runtime root folder no warnings logged for this project. But i still see plenty of PNSEs with manual search, so run the build from the project src folder and see the warnings. There is 2 public APIs needs
UnsupportedOSPlatformannotation for specific platforms (top 2 rows), and 71 APIs defined in internal class and doesn't seem to have any reference internally (remaining rows). (Did not check all but all i have checked has not referenced). I am not sure what to do with them, so i will leave them up to the area owners. Also seems the project is only supported on specific platforms and throws on all other, probably we want to add project levelSupportedOSPlatformattribute for each support. So here is the list of APIs (might) need annotation:CC @karelz @marek-safar @jeffhandley @ManickaP
ManickaP commentedon Aug 11, 2021
Hi @buyaa-n, can I ran the tool locally on my own? If so, than how? I'd like to make sure that my change gets rid of of these issues.
ManickaP commentedon Aug 11, 2021
So I'm hitting an issue that illumos and solaris are unknown:
I see in #49323 (comment):
So what's the guideline here @buyaa-n ?
Note that using
SupportedOSdoesn't help here either since that would beUnknownUnixwhich has the same problem as the two OSes above.buyaa-n commentedon Aug 11, 2021
Thanks, that is not necessary, but if you still want to do it i can give you the analyzer's dll which you can replace with your local NuGet copy and run the build
buyaa-n commentedon Aug 11, 2021
Not sure why do you think the
<SupportedPlatform>hook would not work and what isUnknownUnix? Did already you tried adding<SupportedPlatform Include="illumos"/>and<SupportedPlatform Include="solaris "/>(might need to add them conditional so that it is not added in unnecessary builds)Another thing you might want to consider is if the API is only supported on certain platforms better to add SupportedOSPlatform attribute(s) instead of UnsupportedOSPlatform attribute(s)
ManickaP commentedon Aug 11, 2021
That's what I've meant with "using SupportedOS doesn't help here either", because the file is conditionally compiled for these 3 platforms: UknownUnix, Solaris, illumos:
runtime/src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj
Lines 184 to 188 in 0ea8653
Neither of those strings works with
SupportedOSPlatform/UnsupportedOSPlatform. So reversing the condition doesn't help here.What is that? Do we have an example of this? I've never heard or seen this.
buyaa-n commentedon Aug 11, 2021
I see, with
Note that using SupportedOS doesn't helpi thought you are talking about<SupportedPlatform Include="illumos"/> and <SupportedPlatform Include="solaris "/>propertiesIt is a used to add unknown platforms to the
MSBuildSupportedPlatforms list, you can add<SupportedPlatform Include="illumos"/> and <SupportedPlatform Include="solaris "/>item group in your project. Probably with a conditionCondition="'$(TargetsUnknownUnix)' == 'true'"ManickaP commentedon Aug 12, 2021
Please, could you send me the dll? I don't see any difference with or without my change, so it's really hard for me to know if I'm actually fixing anything.
BTW, the
<SupportedPlatform ...>helped so thanks for that 👍buyaa-n commentedon Aug 13, 2021
JFYI: the analyzer is not perfect it could produce lots of false positives (have fixed some false positives recently, but still you could see some, though do not remember if and how many false positives the
System.Net.NetworkInformationhad). Anyway will send the dll via email, meanwhile, the source is here if you want to take a look