Skip to content
This repository has been archived by the owner on Nov 1, 2018. It is now read-only.

Detect OS using DNX #123

Merged
merged 1 commit into from
Sep 1, 2015
Merged

Detect OS using DNX #123

merged 1 commit into from
Sep 1, 2015

Conversation

natemcmaster
Copy link
Contributor

Workaround aspnet/dnx#2594.
Fixes #121

The code is right and should work, but cannot verify on OS X until DNX fixes other major issues with osx/coreclr.

@bricelam
Copy link
Contributor

bricelam commented Sep 1, 2015

Hmm, we may want to wait for those "other major issues" to be fixed before reacting to anything locally in our repository. Is OSX still more or less broken after this change? Does it make things more usable on Linux at least?

@natemcmaster
Copy link
Contributor Author

It makes it more usable on Linux. And in theory, the blocking issues I am seeing now shouldn't require more code changes.

@natemcmaster
Copy link
Contributor Author

Also, this actually works on beta7 on OS X (but not beta8)

{
return;
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code should stay. When DNX finishes implementing the runtime artifacts feature, it should start working correctly. The code in NativeLibraryLoader for dnx451 & dnxcore50 will also be removed when that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should remove it as the "System.Runtime.InteropServices.RuntimeInformation" package is fickle. As of yesterday, our CI feed publishes a Windows version. At other times, our CI have contained the Linux version. I don't want CI to break this code.

This may change if we unpublish System.Runtime.InteropServices.RuntimeInformation altogether because I believe this publication is actually a mistake in our CI settings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can defer until #128...

@bricelam
Copy link
Contributor

bricelam commented Sep 1, 2015

:shipit:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants