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

Update to support CoreCLR #308

Closed
wants to merge 10 commits into
base: develop
from

Conversation

Projects
None yet
7 participants
@manu-st

manu-st commented Oct 28, 2015

This is a set of changes to get the minimal part of OpenTK to compile against CoreCLR. To do so, there is a dependence on the CoreCLR reference assemblies. I updated the solution and project files so that all targets can be compiled. I haven't been able to test it under Mono.

The main changes are with respect to the interop of strings. My understanding is that most of the time the C strings we get back are UTF8 encoded, so I provided my own helper class to perform the conversion and I also use it in the rewrite tool. Let me know if this correct way to do this.

A few things are not supported on CoreCLR (e.g. some missing exception types) and did my best to provide a meaningful replacement.

Thanks,
Manu

#if _NET_CORECLR
namespace System
{
public class ApplicationException : Exception

This comment has been minimized.

@mellinoe

mellinoe Oct 29, 2015

I went the route of putting this into "Minimal.cs", it seemed like that was where this sort of thing went (basically mock implementations of stuff for platforms that don't already have them). Doesn't really matter in the end, though, I think. Perhaps @thefiddler has guidance.

@mellinoe

mellinoe Oct 29, 2015

I went the route of putting this into "Minimal.cs", it seemed like that was where this sort of thing went (basically mock implementations of stuff for platforms that don't already have them). Doesn't really matter in the end, though, I think. Perhaps @thefiddler has guidance.

@@ -33,7 +33,11 @@ namespace OpenTK
/// <summary>
/// This exception is thrown when a GraphicsContext property cannot be changed after creation.
/// </summary>
#if _NET_CORECLR
public class ContextExistsException : Exception

This comment has been minimized.

@mellinoe

mellinoe Oct 29, 2015

I think you shouldn't need this since you have a definition for ApplicationException (the one I commented on below).

@mellinoe

mellinoe Oct 29, 2015

I think you shouldn't need this since you have a definition for ApplicationException (the one I commented on below).

This comment has been minimized.

@manu-st

manu-st Oct 29, 2015

Right, it was done before I felt uneasy about my changes to replace ApplicationException into just a mundane Exception.

@manu-st

manu-st Oct 29, 2015

Right, it was done before I felt uneasy about my changes to replace ApplicationException into just a mundane Exception.

if (System.Environment.OSVersion.Version.Major >= 6)
#else
// For the time being we assume that we are running on anything above Windows Vista.

This comment has been minimized.

@mellinoe

mellinoe Oct 29, 2015

Should be fine, we actually only support Win7+

@mellinoe

mellinoe Oct 29, 2015

Should be fine, we actually only support Win7+

@@ -58,7 +58,9 @@ public WinInputBase()
WndProc = WindowProcedure;
InputThread = new Thread(ProcessEvents);
#if !_NET_CORECLR
InputThread.SetApartmentState(ApartmentState.STA);

This comment has been minimized.

@mellinoe

mellinoe Oct 29, 2015

I wonder if this is problematic at all? I believe that STA is the default, though, and since we don't expose any way to set this, I guess it shouldn't be an issue.

@mellinoe

mellinoe Oct 29, 2015

I wonder if this is problematic at all? I believe that STA is the default, though, and since we don't expose any way to set this, I guess it shouldn't be an issue.

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Oct 29, 2015

Awesome! Very glad to see a PR here; really hoping we can get official support for CoreCLR soon so I can stop using my private version that I copy around everywhere.

Looks good overall to me, but others are certainly much more familiar with the OpenTK codebase than I am. I think your changes and mine are pretty much the same in most places, but you have a much more comprehensive handling of UTF8 strings, and you changed a lot more in the generator to support those as well. I've only tested my version briefly on Ubuntu, but my test program didn't really use a big portion of the library, so I must have not hit any of the issues arising from bad string marshaling. I did notice that my program crashed on OSX, though, so maybe that was why. 😄

@thefiddler How do you feel about pulling in the git submodule for referencing the .NET Core reference assemblies? In my fork, I just used the new project template in MSBuild/VS 2015 and grabbed the necessary references from NuGet. I think it's a bit cleaner that way, but obviously it would force people to use VS2015 in order to build the .NET Core version, and I'm not sure what the support for building that on other platforms is like (probably not good right now).

mellinoe commented Oct 29, 2015

Awesome! Very glad to see a PR here; really hoping we can get official support for CoreCLR soon so I can stop using my private version that I copy around everywhere.

Looks good overall to me, but others are certainly much more familiar with the OpenTK codebase than I am. I think your changes and mine are pretty much the same in most places, but you have a much more comprehensive handling of UTF8 strings, and you changed a lot more in the generator to support those as well. I've only tested my version briefly on Ubuntu, but my test program didn't really use a big portion of the library, so I must have not hit any of the issues arising from bad string marshaling. I did notice that my program crashed on OSX, though, so maybe that was why. 😄

@thefiddler How do you feel about pulling in the git submodule for referencing the .NET Core reference assemblies? In my fork, I just used the new project template in MSBuild/VS 2015 and grabbed the necessary references from NuGet. I think it's a bit cleaner that way, but obviously it would force people to use VS2015 in order to build the .NET Core version, and I'm not sure what the support for building that on other platforms is like (probably not good right now).

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Oct 29, 2015

As an aside, one of the guys on the corefx team has worked a bit on an experimental UTF8 string library. I don't really know how much what you have here overlaps, but you may find it interesting anyways:

https://github.com/dotnet/corefxlab/tree/master/src/System.Text.Utf8

mellinoe commented Oct 29, 2015

As an aside, one of the guys on the corefx team has worked a bit on an experimental UTF8 string library. I don't really know how much what you have here overlaps, but you may find it interesting anyways:

https://github.com/dotnet/corefxlab/tree/master/src/System.Text.Utf8

@manu-st

This comment has been minimized.

Show comment
Hide comment
@manu-st

manu-st Oct 29, 2015

@mellinoe My UTF8String static class is very minimal and I'm not sure how efficient it is. I'm merely getting the C string pointer and use the Encoding.UTF8.GetString method to perform the conversion. My statics are checking for NULL when before there was no such checks, maybe this could be improved, and the rewriter changed to directly reference Encoding.

I don't think that the string conversions are in a performance critical path but I might be wrong. If this needs to be optimized, we will have to change the code in the rewriter.

manu-st commented Oct 29, 2015

@mellinoe My UTF8String static class is very minimal and I'm not sure how efficient it is. I'm merely getting the C string pointer and use the Encoding.UTF8.GetString method to perform the conversion. My statics are checking for NULL when before there was no such checks, maybe this could be improved, and the rewriter changed to directly reference Encoding.

I don't think that the string conversions are in a performance critical path but I might be wrong. If this needs to be optimized, we will have to change the code in the rewriter.

Emmanuel Stapf added some commits Oct 27, 2015

Emmanuel Stapf
Updated code to compile against CORECLR. Added missing classes such a…
…s ApplicationException, SuppressUnmanagedCodeSecurity, ISerializable, Serializable. Most of the work was done with interop to convert UTF8 strings into .NET string.
@manu-st

This comment has been minimized.

Show comment
Hide comment
@manu-st

manu-st Dec 15, 2015

I've squashed the commits to reflect the latest changes in the develop branch. Could this be merged? If not, any feedback? Thanks.

manu-st commented Dec 15, 2015

I've squashed the commits to reflect the latest changes in the develop branch. Could this be merged? If not, any feedback? Thanks.

@Frassle

This comment has been minimized.

Show comment
Hide comment
@Frassle

Frassle Dec 16, 2015

Contributor

The big blocker to this is taking on a dependency to another repo (the submodule git@github.com:manu-silicon/corefxref.git).

The rest I haven't had a chance to fully look at.

Contributor

Frassle commented Dec 16, 2015

The big blocker to this is taking on a dependency to another repo (the submodule git@github.com:manu-silicon/corefxref.git).

The rest I haven't had a chance to fully look at.

@manu-st

This comment has been minimized.

Show comment
Hide comment
@manu-st

manu-st Dec 16, 2015

The other repo is really just for convenience as it enables you to keep .csproj based solution without having to fiddle with project.json. Note that the code compiles fine in normal mode without the other repo. So I can remove the _CoreCLR solution and project and the external module from the PR. I would keep those on my end. What I'm looking for is to be able to compile the C# code without modification.

Thanks.

manu-st commented Dec 16, 2015

The other repo is really just for convenience as it enables you to keep .csproj based solution without having to fiddle with project.json. Note that the code compiles fine in normal mode without the other repo. So I can remove the _CoreCLR solution and project and the external module from the PR. I would keep those on my end. What I'm looking for is to be able to compile the C# code without modification.

Thanks.

@Frassle

This comment has been minimized.

Show comment
Hide comment
@Frassle

Frassle Dec 17, 2015

Contributor

So I can remove the _CoreCLR solution and project and the external module from the PR.

That would be easier to merge in.

Contributor

Frassle commented Dec 17, 2015

So I can remove the _CoreCLR solution and project and the external module from the PR.

That would be easier to merge in.

@manu-st

This comment has been minimized.

Show comment
Hide comment
@manu-st

manu-st Dec 18, 2015

Will do. Currently I use _NET_CORECLR to have code that is CoreCLR/CoreFX specific. Are you ok with that name?

manu-st commented Dec 18, 2015

Will do. Currently I use _NET_CORECLR to have code that is CoreCLR/CoreFX specific. Are you ok with that name?

@Frassle

This comment has been minimized.

Show comment
Hide comment
@Frassle

Frassle Dec 21, 2015

Contributor

Yes that's fine

Contributor

Frassle commented Dec 21, 2015

Yes that's fine

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Dec 31, 2015

For what it's worth, instead of having the submodule dependency, we could depend on the new features in VS 2015 for building .NET Core libraries. It would require having VS 2015 and an optional feature to be installed, but it would otherwise work out-of-the-box. https://github.com/mellinoe/opentk/tree/netcore-port is set up to build like that.

mellinoe commented Dec 31, 2015

For what it's worth, instead of having the submodule dependency, we could depend on the new features in VS 2015 for building .NET Core libraries. It would require having VS 2015 and an optional feature to be installed, but it would otherwise work out-of-the-box. https://github.com/mellinoe/opentk/tree/netcore-port is set up to build like that.

@cra0zy

This comment has been minimized.

Show comment
Hide comment
@cra0zy

cra0zy Jan 4, 2016

Contributor

For what it's worth, instead of having the submodule dependency, we could depend on the new features in VS 2015 for building .NET Core libraries. It would require having VS 2015 and an optional feature to be installed, but it would otherwise work out-of-the-box. https://github.com/mellinoe/opentk/tree/netcore-port is set up to build like that.

What about Xamarin Studio/MonoDevelop? They would need to support it so it can be compiled from Mac/Linux as well.

Contributor

cra0zy commented Jan 4, 2016

For what it's worth, instead of having the submodule dependency, we could depend on the new features in VS 2015 for building .NET Core libraries. It would require having VS 2015 and an optional feature to be installed, but it would otherwise work out-of-the-box. https://github.com/mellinoe/opentk/tree/netcore-port is set up to build like that.

What about Xamarin Studio/MonoDevelop? They would need to support it so it can be compiled from Mac/Linux as well.

@cra0zy cra0zy referenced this pull request Jan 4, 2016

Closed

Update to Mono 4.0.4.4+ #3

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jan 4, 2016

What about Xamarin Studio/MonoDevelop? They would need to support it so it can be compiled from Mac/Linux as well.

Yes, that's a fair point. I don't think there's really any tooling right now to build CoreCLR libraries/applications in MonoDevelop as far as I know, but I haven't looked into it lately. The important part that is probably missing is resolving the reference assemblies from NuGet rather than a "Targeting Pack" folder.

mellinoe commented Jan 4, 2016

What about Xamarin Studio/MonoDevelop? They would need to support it so it can be compiled from Mac/Linux as well.

Yes, that's a fair point. I don't think there's really any tooling right now to build CoreCLR libraries/applications in MonoDevelop as far as I know, but I haven't looked into it lately. The important part that is probably missing is resolving the reference assemblies from NuGet rather than a "Targeting Pack" folder.

@manu-st

This comment has been minimized.

Show comment
Hide comment
@manu-st

manu-st Jan 5, 2016

I just updated the PR to exclude the project changes to build against my CoreFX submodule. Sorry it took so long.

manu-st commented Jan 5, 2016

I just updated the PR to exclude the project changes to build against my CoreFX submodule. Sorry it took so long.

@@ -57,7 +57,11 @@ public static class BlittableValueType<T>
static BlittableValueType()
{
Type = typeof(T);
#if _NET_CORECLR
if (Type.GetTypeInfo().IsValueType && !Type.GetTypeInfo().IsGenericType)

This comment has been minimized.

@tzachshabtay

tzachshabtay May 6, 2016

Contributor

Why not just override with Type.GetTypeInfo instead of adding it under the compilation flag? It's supported in dot net 4.5 as well (and the recommended way, as far as I understand). Is there a need to support .Net 4?
Also, maybe it would be better to take Type.GetTypeInfo into a variable to avoid calling it twice inside the if.

@tzachshabtay

tzachshabtay May 6, 2016

Contributor

Why not just override with Type.GetTypeInfo instead of adding it under the compilation flag? It's supported in dot net 4.5 as well (and the recommended way, as far as I understand). Is there a need to support .Net 4?
Also, maybe it would be better to take Type.GetTypeInfo into a variable to avoid calling it twice inside the if.

This comment has been minimized.

@manu-st

manu-st May 9, 2016

I've been upgrading a few projects to CoreCLR and some have a minimum requirement of .NET 4.0 thus the #if. If this is not required for OpenTK, then we could simplify the code as you are suggesting.

@manu-st

manu-st May 9, 2016

I've been upgrading a few projects to CoreCLR and some have a minimum requirement of .NET 4.0 thus the #if. If this is not required for OpenTK, then we could simplify the code as you are suggesting.

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe May 22, 2016

We should try to get this merged in soon, especially since RC2 of .NET Core was just released.

I have another branch of this which I just rebased against OpenTK/develop, so it is fully up-to-date and merged cleanly. https://github.com/mellinoe/opentk/tree/netcore-port-rebase. I've been developing from my forked version for a long time, and it would be nice to be able to get everything merged back upstream, especially with other folks now contributing fixes here.

My branch and @manu-silicon's are probably 99% identical, in terms of code changes. I've made a few extra changes on top of the stuff here, but I wouldn't be surprised if he's done the same after making the PR.

mellinoe commented May 22, 2016

We should try to get this merged in soon, especially since RC2 of .NET Core was just released.

I have another branch of this which I just rebased against OpenTK/develop, so it is fully up-to-date and merged cleanly. https://github.com/mellinoe/opentk/tree/netcore-port-rebase. I've been developing from my forked version for a long time, and it would be nice to be able to get everything merged back upstream, especially with other folks now contributing fixes here.

My branch and @manu-silicon's are probably 99% identical, in terms of code changes. I've made a few extra changes on top of the stuff here, but I wouldn't be surprised if he's done the same after making the PR.

@varon

This comment has been minimized.

Show comment
Hide comment
@varon

varon May 22, 2016

Member

Wow! This has been outstanding for ages - thanks for the heads up, @mellinoe.

At the moment, our core priority is getting the NuGet package up and running (see #386).
This is pretty important, though, so I'll chase down what's happening with this PR, and figure out what's needed to get it merged.

Member

varon commented May 22, 2016

Wow! This has been outstanding for ages - thanks for the heads up, @mellinoe.

At the moment, our core priority is getting the NuGet package up and running (see #386).
This is pretty important, though, so I'll chase down what's happening with this PR, and figure out what's needed to get it merged.

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe May 22, 2016

No problem, I understand lots of things have been in flux lately with the project. I also understand the hesitation to move forward with this in particular since .NET Core is still in pre-release and hasn't been 100% stable since this PR. But with RC2 being released, and RTM just around the corner, I think those concerns shouldn't be as important.

Getting the NuGet package back up is a good priority. If we merge this in, we can hold off in including the new configuration in the Nuget package until things are more stable. The main benefit for getting it merged in is minimizing the number of forks that are out there 😄 .

mellinoe commented May 22, 2016

No problem, I understand lots of things have been in flux lately with the project. I also understand the hesitation to move forward with this in particular since .NET Core is still in pre-release and hasn't been 100% stable since this PR. But with RC2 being released, and RTM just around the corner, I think those concerns shouldn't be as important.

Getting the NuGet package back up is a good priority. If we merge this in, we can hold off in including the new configuration in the Nuget package until things are more stable. The main benefit for getting it merged in is minimizing the number of forks that are out there 😄 .

@feliwir

This comment has been minimized.

Show comment
Hide comment
@feliwir

feliwir Jul 20, 2016

Would be nice to see OpenTK work with .NET core out of the box

feliwir commented Jul 20, 2016

Would be nice to see OpenTK work with .NET core out of the box

@manu-st

This comment has been minimized.

Show comment
Hide comment
@manu-st

manu-st Jul 21, 2016

@feliwir I agree. At the moment, although this PR updates the code of the library to make it compile against .NET Core, the .csproj are not compilable against .NET Core. One can use our branch where this is possible https://github.com/siliconstudio/opentk/tree/coreclr. In any case, OpenTK works great against .NET Core, this what helped us build a Linux version of our game engine easily against .NET Core.

manu-st commented Jul 21, 2016

@feliwir I agree. At the moment, although this PR updates the code of the library to make it compile against .NET Core, the .csproj are not compilable against .NET Core. One can use our branch where this is possible https://github.com/siliconstudio/opentk/tree/coreclr. In any case, OpenTK works great against .NET Core, this what helped us build a Linux version of our game engine easily against .NET Core.

@tzachshabtay

This comment has been minimized.

Show comment
Hide comment
@tzachshabtay

tzachshabtay Jul 21, 2016

Contributor

Based on their roadmap (https://github.com/dotnet/core/blob/master/roadmap.md), csproj should be supported in the next version (1.1).

Contributor

tzachshabtay commented Jul 21, 2016

Based on their roadmap (https://github.com/dotnet/core/blob/master/roadmap.md), csproj should be supported in the next version (1.1).

@cra0zy

This comment has been minimized.

Show comment
Hide comment
@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jul 22, 2016

For what it's worth, it is already possible to build .NET Core projects with MSBuild. I'm doing it in my fork of OpenTK. The main problems are that it requires a new version of VS/MSBuild, and so it probably doesn't build properly outside windows.

I also published a NuGet package that I've been using in some of my personal projects since I got annoyed with copying my local version of OpenTK around. Like @manu-silicon said, OpenTK works great on CoreCLR.

mellinoe commented Jul 22, 2016

For what it's worth, it is already possible to build .NET Core projects with MSBuild. I'm doing it in my fork of OpenTK. The main problems are that it requires a new version of VS/MSBuild, and so it probably doesn't build properly outside windows.

I also published a NuGet package that I've been using in some of my personal projects since I got annoyed with copying my local version of OpenTK around. Like @manu-silicon said, OpenTK works great on CoreCLR.

@varon

This comment has been minimized.

Show comment
Hide comment
@varon

varon Nov 22, 2016

Member

Any updates on this? I'd really like to see OpenTK running on .Net core!

Member

varon commented Nov 22, 2016

Any updates on this? I'd really like to see OpenTK running on .Net core!

@varon

This comment has been minimized.

Show comment
Hide comment
@varon

varon Dec 12, 2016

Member

As a project goal, We are 100% behind supporting .Net core as a target.

However, I'm going to close this PR as it can't be merged anyway, and would need to be redone against the current source tree. We'd really appreciate contributions on this!

Member

varon commented Dec 12, 2016

As a project goal, We are 100% behind supporting .Net core as a target.

However, I'm going to close this PR as it can't be merged anyway, and would need to be redone against the current source tree. We'd really appreciate contributions on this!

@varon varon closed this Dec 12, 2016

@manu-st

This comment has been minimized.

Show comment
Hide comment
@manu-st

manu-st Dec 12, 2016

@varon No issue as all for closing this. If I have some time over christmas I'll get another shot at it.

manu-st commented Dec 12, 2016

@varon No issue as all for closing this. If I have some time over christmas I'll get another shot at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment