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

$(OS) should return only two values - Windows_NT and Unix . #538

Merged
merged 1 commit into from
Apr 11, 2016

Conversation

radical
Copy link
Member

@radical radical commented Mar 22, 2016

Based on the discussion in issue #539, $(OS) property will return
Windows_NT whenever running on windows and Unix for any other OS.

NativeMethodsShared.OSName is used to set that property, hence this is
being changed.

@cdmihai
Copy link
Contributor

cdmihai commented Mar 24, 2016

@dotnet-bot test this please

@cdmihai
Copy link
Contributor

cdmihai commented Mar 24, 2016

Should we revert this only for Mono?

@radical
Copy link
Member Author

radical commented Mar 25, 2016

No, IMO, we should have uniform behavior w.r.t this. Having different behavior will likely cause unnecessary pain. And it doesn't affect compatibility with existing projects.

If the proposed approach in #539 sounds acceptable, then I'll change this PR accordingly. The proposed approach being:

  1. $(OS) will have values of Windows_NT and Unix only. On Windows - $(OS)==Windows_NT and only any IsUnixLike platform - $(OS)==Unix.
  2. Introduce a new property $(OSName) which would have more specific values like Windows_NT, OSX and Unix. I'm not sure whether Unix is enough here or it should be more specific like Linux, FreeBSD etc.

If the user's requirement is to simply check for "windows or unix-like", then $(OS) can be used. If the requirement is to have more specific behavior, then they can use $(OSName).

@akoeplinger @jonpryor ^

If this is acceptable, then I can update the PR to not revert the patch, hence retain NativeMethodsShared.OSName, set the new property with this and set the $(OS) property explicitly.

@cdmihai
Copy link
Contributor

cdmihai commented Mar 25, 2016

@joperezr Does this affect corefx in any way? Please also take a look at #539

@joperezr
Copy link
Member

@joperezr Does this affect corefx in any way? Please also take a look at #539

In corefx we currently do the 'hack' of setting OSX to Unix because we were seeing issues for this so I don't think that this would cause issues for now, but I'm not sure until I test it.

@AndyGerlicher
Copy link
Contributor

Based on discussion in #539 this should change to only return Windows_NT when Windows, else Unix. You want to update this PR?

@radical
Copy link
Member Author

radical commented Apr 6, 2016

Sure, I'll do that.

Based on the discussion in issue dotnet#539, $(OS) property will return
`Windows_NT` whenever running on windows and `Unix` for any other OS.

NativeMethodsShared.OSName is used to set that property, hence this is
being changed.
@radical radical changed the title Revert "NativeMethodsShared.OSName: Fix the case of OSX" $(OS) should return only two values - Windows_NT and Unix . Apr 7, 2016
@AndyGerlicher
Copy link
Contributor

LGTM

@cdmihai
Copy link
Contributor

cdmihai commented Apr 8, 2016

@dotnet-bot test this please

@rainersigwald rainersigwald merged commit 5e01f07 into dotnet:xplat Apr 11, 2016
@radical radical deleted the revert-osname branch April 11, 2016 20:38
aslakhellesoy pushed a commit to cucumber/gherkin-dotnet that referenced this pull request Sep 6, 2017
On Windows, the $(OS) is "WIndows_NT" since dotnet/msbuild#538
Also update .gitignore and .sln to latest VS 2017.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants