Clean up OS-related property uses, update buildtools, update MSBuild #15728
Conversation
Our targets should now be able to use the following properties in all cases: * RunningOnCore : This is determined by MSBuildRuntimeType. This should only control MSBuild-related stuff, like the path to the task assembly being used, or which Roslyn props file is imported (different behavior on .NET Core). * RunningOnUnix: This will be used to control things like path separators, environment variable syntax, executable file suffix, etc. which are the same everywhere except for Windows. This is determined by the value of OS. Any value other than Windows_NT will result in RunningOnUnix being set to true. * TargetOS: This represents the target operating system for tests. "OSGroup" cannot be re-used for this because most tests are generally built for the "AnyOS" group, which does not accurately represent the OS being run on. * OSGroup: This should be able to supplant all other uses of OS and OSEnvironment. This property is already defaulted to the correct value (based on the local environment) if it is not set.
This updates the version of .NET Core MSBuild used. A line was changed in Roslyn.Common.props which has been replicated here in this copy of the file.
|
This is mostly just cleanup and renaming so I'm gonna go ahead and merge it. I can do some further cleanup if there's feedback. |
| <PropertyGroup> | ||
| <TargetOS Condition="'$(TargetOS)' == ''">$(DefaultOSGroup)</TargetOS> |
There was a problem hiding this comment.
This isn't enough to correctly filter for the other OS's. DefaultOSGroup will only ever have OSX, Unix, or Windows_NT. I expect this will break some tests. See TargetOSTrait above for example it has some other targets.
There was a problem hiding this comment.
DefaultOSGroup is set to Linux in cases where $(OS) == Unix and the /Applications/ folder doesn't exist (it should only exist on OSX).
There was a problem hiding this comment.
What about FreeBSD/NetBSD? will those ever be set?
There was a problem hiding this comment.
They will not, but we don't use those anywhere right now. We'd probably need to add some extra logic to auto-detect it, or go back to using the build task for that.
There was a problem hiding this comment.
OK can you clean those out then if they are dead. I would like to keep these things consistent
There was a problem hiding this comment.
I think it's fine to keep them. There are a couple of tests that have [PlatformSpecific(TestPlatforms.FreeBSD)], it's just that we do not have any runs that would ever use that right now. If you were trying to run those tests, you would just need to specify TargetOS=FreeBSD manually.
| @@ -23,10 +23,12 @@ | |||
|
|
|||
| <!-- Platform detection --> | |||
| <PropertyGroup> | |||
| <OsEnvironment Condition="'$(OsEnvironment)'=='' AND '$(OS)'=='Unix' AND Exists('/Applications')">OSX</OsEnvironment> | |||
| <OsEnvironment Condition="'$(OsEnvironment)'=='' AND '$(OS)'=='Unix'">Linux</OsEnvironment> | |||
| <OsEnvironment Condition="'$(OsEnvironment)'==''">$(OS)</OsEnvironment> | |||
There was a problem hiding this comment.
@mellinoe noticed this while looking at the other problem: there are still a few targets in buildtools that rely on OsEnvironment being set, e.g.: https://github.com/dotnet/buildtools/search?utf8=%E2%9C%93&q=OsEnvironment
I think with this change those would be doing the wrong thing.
There was a problem hiding this comment.
Yeah there are a few things in there that would be broken, although most of the interesting ones are being overridden here. I'll file an issue in buildtools to clean those up when we integrate the corefx changes back into there.
There was a problem hiding this comment.
Clean up OS-related property uses, update buildtools, update MSBuild Commit migrated from dotnet/corefx@99511c7
This fixes https://github.com/dotnet/corefx/issues/15305, taking the approach I described over in that issue.
I've also updated buildtools as part of this, which pulls in a new version of .NET Core MSBuild. With all these changes, I'm able to do the full managed build on Windows using the .NET Core version of MSBuild. The native build does not work with .NET Core MSBuild, but that would require some more thought and more changes. I haven't changed the default in any way, just tested that it was possible.
@weshaggard