Enable registry properties#7621
Conversation
On non-Windows OSes, return empty, as we had been doing in .NET Core before. This enables more project types to evaluate and match .NET Framework MSBuild's results when on Windows.
15c4dc2 to
13079b1
Compare
While looking at registry properties I noticed these were also disabled. Same logic applies for enabling them.
|
|
||
| [Fact] | ||
| [PlatformSpecific(TestPlatforms.Windows)] | ||
| [SupportedOSPlatform("windows")] |
There was a problem hiding this comment.
What's the difference between these?
There was a problem hiding this comment.
The former disables the test on non-Windows platforms (via Arcade magic). The latter silences the warnings about using Windows-only APIs. As far as I could tell from the API docs, there's not a way to tell the OS-checking analyzer about another attribute that has the same meaning as SupportedOSPlatform.
| // .NET Core MSBuild used to always return empty, so match that behavior | ||
| // on non-Windows (no registry), and with a changewave (in case someone | ||
| // had a registry property and it breaks when it lights up). | ||
| if (!NativeMethodsShared.IsWindows || !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4)) |
There was a problem hiding this comment.
Do you need the ifdef here? People are opted in anyway, so it won't affect anyone unless they disable the change wave, and I don't think people normally do that. Or maybe just plan to remove the ifdef with the change wave? If so, that should be in a comment so we remember.
There was a problem hiding this comment.
Why bother doing the runtime check when we're on .NET Framework and know a priori that we're on Windows?
There was a problem hiding this comment.
We could also be on mono, not that you necessarily care. I imagine the win32 check was a real "on windows" check. Other reason is to unify the code paths, which I generally think positive.
On non-Windows OSes, return empty, as we had been doing in .NET Core before.
This enables more project types to evaluate and match .NET Framework MSBuild's results when on Windows.
Fixes an issue reported via IM by @dfederm.