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

Prevent false-positive in EnvironmentHelper.isMacOS on Windows #1204

Merged
merged 1 commit into from
Apr 7, 2016
Merged

Prevent false-positive in EnvironmentHelper.isMacOS on Windows #1204

merged 1 commit into from
Apr 7, 2016

Conversation

allykzam
Copy link
Contributor

@allykzam allykzam commented Apr 6, 2016

If the current-directory is on a mapped drive, e.g. the M:\ drive, calling File.Exists with "/usr/bin/osascript" translates to "M:\usr\bin\osascript" -- this exists in some cases where Windows is running as a virtual machine on an OSX host.

Lots of code-comment to make this clear. Should still cover #1019 while also resolving #1202.

If the current-directory is on a mapped drive, e.g. the M:\ drive,
calling File.Exists with "/usr/bin/osascript" translates to
"M:\usr\bin\osascript" -- this exists in some cases where Windows is
running as a virtual machine on an OSX host.
@inosik
Copy link
Contributor

inosik commented Apr 7, 2016

I think changing the check to

let isMacOS =
    (Environment.OSVersion.Platform = PlatformID.Unix) && // <-- Change OR to AND and OSX to Unix
      // osascript is the AppleScript interpreter on OS X
      File.Exists "/usr/bin/osascript"

should be enough.

The problem is that on Mono, Environment.OSVersion.Platform always returns Unix. This property never returned MacOSX, actually. See #1019 and #1044.

@inosik
Copy link
Contributor

inosik commented Apr 7, 2016

isMacOS = isUnix && File.Exists "/usr/bin/osascript"

would be even simpler.

@forki forki merged commit 8c07854 into fsprojects:master Apr 7, 2016
@allykzam
Copy link
Contributor Author

allykzam commented Apr 7, 2016

@inosik It would be simpler, but it would also behave incorrectly later if anyone ever felt like fixing mono (or .NET Core?) to correctly return MacOSX.

@inosik
Copy link
Contributor

inosik commented Apr 8, 2016

@amazingant you're right, didn't think about this.

Speaking of .NET Core, this API is (probably?) going to change anyways. I remember an issue about it at dotnet/corefx.

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.

3 participants