Skip to content
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

Clean-up usage of $(OS) property in targets #19963

Closed
weshaggard opened this issue Jan 19, 2017 · 3 comments · Fixed by dotnet/corefx#15728
Closed

Clean-up usage of $(OS) property in targets #19963

weshaggard opened this issue Jan 19, 2017 · 3 comments · Fixed by dotnet/corefx#15728

Comments

@weshaggard
Copy link
Member

We should try and limit the usage of $(OS) property in all the targets files. In places we need it we should use it in a limited fashion to setup other properties like RunningOnUnix/Windows and we should use that as the conditions in all our targets.

Examples we should clean-up.
test.targets (https://github.com/dotnet/corefx/pull/15177/files#diff-b8c5f6b5d9e571e8187355199b2a1680R33), in this case we should use TargetOS.
https://github.com/dotnet/corefx/blob/master/Tools-Override/ConstructSharedFx.targets#L25

We should grep through all our targets files and update them in a consistent manner.

@mellinoe mellinoe self-assigned this Feb 1, 2017
@mellinoe
Copy link
Contributor

mellinoe commented Feb 1, 2017

I think we should be able to all of these patterns with a few properties:

  • 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.
  • OSGroup: This should be able to supplant all other uses of OS, OSEnvironment, and TargetOS. This property is already defaulted to the correct value (based on the local environment) if it is not set.

I'm going to try to take a crack at cleaning our targets above following the above plan. A goal will be to allow using .NET Core MSBuild on Windows, at least as a proof of concept.

@weshaggard

@weshaggard
Copy link
Member Author

Seems like a reasonable plan.

@mellinoe
Copy link
Contributor

mellinoe commented Feb 1, 2017

I experimented a bit. I believe we do need to keep around TargetOS. The problem is that OSGroup doesn't actually represent the OS you are running on, but rather the build configuration you are building for. Most of the time, it is just AnyOS, because most tests are not different per-platform. So we do need the TargetOS property to differentiate.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants