Skip to content

Conversation

@stewartmiles
Copy link
Contributor

This patch introduces:

  • Type forwarding DLL for Parse implementation of System.Threading.Tasks.
  • Project which builds a Unity plugin package that can be used to redistribute
    Parse's implementation of System.Threading.Tasks.

Unity 2017 adds experimental support for .NET 4.5 (upgrading from a partially
compatible version of .NET 3.5). With .NET 4.5, the Parse Task implementation
is no longer required as System.Threading.Task is present in the .NET
framework. For users distributing assemblies built against the Unity.Compat.dll
and Unity.Tasks.dll components, this results in Unity failing to compile
assemblies when .NET 4.5 is selected due to duplicate implementations of
System.Threading.Tasks.

So for the .NET 4.5 configuration in Unity, the Unity.Tasks.dll assembly (built
by Unity.Tasks.TypeForwards) has been introduced which simply type forwards to
the system implementation of System.Threading.Tasks.

To standardize the location of Unity.Tasks.dll and Unity.Compat.dll for plugin
developers reusing these components, a project has been introduced
(Unity.Tasks.UnityPlugin) which builds a ParseTasks.unitypackage containing
both the .NET 3.5 Parse implementation of System.Threading.Tasks and the
.NET 4.5 type forwarding DLL.

Finally, asset labels are used to mark .NET compatibilty of assemblies in the
ParseTasks.unitypackage and asset metadata is used to disable the .NET 4.5
type forwarding DLL by default allowing successful compilation when .NET 3.5 is
selected in Unity. The asset labels utilized to mark .NET compatibility of
assemblies can be used by Unity editor plugins (e.g Google.VersionHandler.dll
distributed as part of https://github.com/googlesamples/unity-jar-resolver) to
enable / disable assemblies based upon the .NET framework version selected
smoothing out the end user experience.

…n build.

This patch introduces:
* Type forwarding DLL for Parse implementation of System.Threading.Tasks.
* Project which builds a Unity plugin package that can be used to redistribute
  Parse's implementation of System.Threading.Tasks.

Unity 2017 adds experimental support for .NET 4.5 (upgrading from a partially
compatible version of .NET 3.5).  With .NET 4.5, the Parse Task implementation
is no longer required as System.Threading.Task is present in the .NET
framework.  For users distributing assemblies built against the Unity.Compat.dll
and Unity.Tasks.dll components, this results in Unity failing to compile
assemblies when .NET 4.5 is selected due to duplicate implementations of
System.Threading.Tasks.

So for the .NET 4.5 configuration in Unity, the Unity.Tasks.dll assembly (built
by Unity.Tasks.TypeForwards) has been introduced which simply type forwards to
the system implementation of System.Threading.Tasks.

To standardize the location of Unity.Tasks.dll and Unity.Compat.dll for plugin
developers reusing these components, a project has been introduced
(Unity.Tasks.UnityPlugin) which builds a ParseTasks.unitypackage containing
both the .NET 3.5 Parse implementation of System.Threading.Tasks and the
.NET 4.5 type forwarding DLL.

Finally, asset labels are used to mark .NET compatibilty of assemblies in the
ParseTasks.unitypackage and asset metadata is used to disable the .NET 4.5
type forwarding DLL by default allowing successful compilation when .NET 3.5 is
selected in Unity.  The asset labels utilized to mark .NET compatibility of
assemblies can be used by Unity editor plugins (e.g Google.VersionHandler.dll
distributed as part of https://github.com/googlesamples/unity-jar-resolver) to
enable / disable assemblies based upon the .NET framework version selected
smoothing out the end user experience.
@stewartmiles
Copy link
Contributor Author

FYI: I've signed the CLA. Anything else you need me to do here to start the review process?

@montymxb
Copy link
Contributor

montymxb commented Aug 4, 2017

@stewartmiles nope nothing more needed, you're good there! Thanks for putting this up, I apologize but I've been a bit busier than expected. I need to sit down and go through this and a few other things, once I get to this I'll give you some feedback and we can go from there.

@stewartmiles
Copy link
Contributor Author

@montymxb thanks for the response, please let me know if you need me to go anything in more detail.

Cheers,
Stewart

@stewartmiles
Copy link
Contributor Author

@montymxb any time to review this?

@montymxb
Copy link
Contributor

@stewartmiles not at the immediate moment, but this did fall though the cracks I must admit...I'll go over this in the next few days. So I'll have something for you by this Saturday. If for some reason I do not get back to you by then raise me again and I'll take of it at that moment. I don't want to leave you hanging again, sorry for that.

My test environment isn't behaving quite right, so I need to reconfigure some stuff. There's the other issue that it seems CI only runs on the master (or release). Ideally I want to reconfigure that so the individual PRs get their own CI results from Appveyor. From there the testing process of these individual PRs can be done, leaving just me or another member to sign off on things.

@montymxb
Copy link
Contributor

Just to you apprised I did attempt to run this but I had some issues with my windows -_-. Basically no environment to test on at the moment for me. I'll have to address this sometime around next week.

@montymxb montymxb self-assigned this Nov 2, 2017
@flovilmart
Copy link

@montymxb any news on this one?

@montymxb
Copy link
Contributor

@flovilmart Yep. I had an issue getting things running initially, something peculiar regarding the build. My local is looking good now and I'm ready to start taking a look at these PRs (particularly this one, apologies @stewartmiles ). I expect to have time this upcoming weekend or before to give this a proper look.

Part of this process is going to get getting CI in place for these PRs so we don't have to guess what the state of these are before we look at them. Partly why I nudged you about appveyor access the other day 😄 . This used to have it setup, but it was privately managed by one of the former team members it appears and is no longer accessible.

@flovilmart
Copy link

I’ll set it up for this SDK too

@flovilmart flovilmart closed this Nov 29, 2017
@flovilmart flovilmart reopened this Nov 29, 2017
@flovilmart
Copy link

It’s activated, you probably need to configure further as the build is failing. The original ci logs (pre migration) are still available and they could help us configure it again.

@montymxb
Copy link
Contributor

Nice! Thanks a TON for setting that up @flovilmart 🥇 . Yeah I've seen the old logs and pretty much have it down to how the CI should run. In addition I'm thinking we should run our release asset generation through this as well. I've done that in the past and it's incredibly helpful, just saves a step we would be doing by hand.

Did you set this up with an internal build config through the account or is it still looking for .appveyor.yml? If it's the later I'll introduce a separate PR where I can work out the CI.

@flovilmart
Copy link

@montymxb let's put an appveyor yaml in, that would be easier. For assets generation, let's see how it goes for this 1st step first no?

@montymxb
Copy link
Contributor

That's what I was thinking, then we actually have community suggestions/PRs available on CI as we go. Step one will just be CI, need to get the tests to actually pass first. We can merge that in and then we can start addressing these issues/PRs. I figure once we get to a release point (probably 1.8.0, maybe 2.0.0 if we change enough) then we can worry about release asset generation.

@montymxb montymxb closed this Dec 2, 2017
@montymxb montymxb reopened this Dec 2, 2017
@codecov
Copy link

codecov bot commented Dec 2, 2017

Codecov Report

Merging #267 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #267   +/-   ##
======================================
  Coverage    67.2%   67.2%           
======================================
  Files         127     127           
  Lines       10139   10139           
  Branches     1455    1455           
======================================
  Hits         6814    6814           
  Misses       3127    3127           
  Partials      198     198

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d50fab...59e0312. Read the comment docs.

@montymxb
Copy link
Contributor

montymxb commented Dec 2, 2017

@flovilmart we have appveyor & codecov in place now. Codecov is just using the existing parse-community account so that's good to go. It originally was under coveralls, but I figure having coverage reports all under the same account makes more sense. Although there is no history the existing coverage matches what we're getting now, so we're in the right spot in terms of continuity.

At the moment appveyor is running under my scope instead of what you setup initially. This was primarily so I could just tweak with settings in appveyor. There's not a lot configured from the account side of things, allowing us to make a change to something more official when convenient (like a specific parse-community account).

@flovilmart
Copy link

How do we shift to the community scope? How do I give you access to it?

@montymxb
Copy link
Contributor

montymxb commented Dec 3, 2017

@flovilmart You would have to setup the project from the given user account, like parse-community. Once it's setup you can add a team member to that account.

@montymxb
Copy link
Contributor

montymxb commented Dec 3, 2017

In the meantime I deleted my project so there's no more confusion there. I'll wait until you set yours up and grant access.

@flovilmart
Copy link

Shoot me your email?

@montymxb
Copy link
Contributor

montymxb commented Dec 3, 2017

Done, over on slack.

@montymxb
Copy link
Contributor

montymxb commented Dec 3, 2017

Alright, this has taken way too long. Apologies @stewartmiles , it's been too long for getting back on this sdk to get things going. I'm hoping we can setup some additional tests for checking against 2015 and 2017 before we finish up a release. Lots of work to do though, hopefully by the end this year we'll have something new for everyone to work with 👍 .

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

:octocat: approved

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