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

Unity3d version support #2268

Merged
merged 5 commits into from Apr 24, 2017
Merged

Unity3d version support #2268

merged 5 commits into from Apr 24, 2017

Conversation

@ivaylo5ev
Copy link

@ivaylo5ev ivaylo5ev commented Apr 20, 2017

This is a proposed fix for issue #2005

Unity3D Projects have the following specifics:

  • They all use TargetFrameworkVersion = v3.5
  • They use up to 4 different TargetFrameworkProfile values:
    • Unity Full v3.5
    • Unity Subset v3.5
    • Unity Micro v3.5
    • Unity Web v3.5
  • The above profiles are recognised by Nuget if the following (corresponding) framework names are used:
    • net35-Unity Full v3.5
    • net35-Unity Subset v3.5
    • net35-Unity Micro v3.5
    • net35-Unity Web v3.5

NB.
I must admit I am a newbie in F# and it is possible to have messed up something with the version and framework detection. So far, all existing tests are passing and the executable I've built does work for me with Unity.
Still, I'd appreciate a somewhat thorough code review.

@cloudRoutine
Copy link
Member

@cloudRoutine cloudRoutine commented Apr 20, 2017

I don't think this is worth investing the necessary energy into validating, integrating, and supporting, right now. I'd been using paket with fsproj that I was compiling for unity without issue or needing to add any additional framework support for well over a year.

More recently I started playing with taking some of the typical packages I use in dev off of Nuget to try out in Unity with F#4.1
Unity3D .net 4.6 Runtime Beta

Once they've made their runtime upgrade and have adopted .netstandard for their .net impl, any additional work we need to do for compatibility will be much simpler than it is right now.

@ivaylo5ev
Copy link
Author

@ivaylo5ev ivaylo5ev commented Apr 24, 2017

Hello Jared,

Thanks for your update. It is a good thing for Unity moving on and I am hoping to see the .netstandard support soon in action. However, the latest version to date is still suggesting the old net35-Unity*** platforms. Thus, I will continue to use my own fork of Paket for my projects until Unity officially catches up to date.

There is just one more thing that maybe you could help me with -- can you give me some clues on how to bundle a single Paket executable? I do not want to use all the files in the bin directory that are produced during the local build. Is it possible to get it somehow from the appveyor build of my own branch or use locally the same mechanism you employ in creating the standalone paket.exe?

Cheers,
Ivaylo Slavov

@inosik
Copy link
Contributor

@inosik inosik commented Apr 24, 2017

Is it possible to get it somehow from the appveyor build of my own branch or use locally the same mechanism you employ in creating the standalone paket.exe?

I think it should be enough to run the MergePaketTool target after building the project.

@ivaylo5ev
Copy link
Author

@ivaylo5ev ivaylo5ev commented Apr 24, 2017

Thanks, that seems simple enough! I will close the issue now, as it does not seem to be addressing a widely requesed feature.

@matthid
Copy link
Member

@matthid matthid commented Apr 24, 2017

I restartet the build. If it is green I think we should integrate this. The change is small enough imho. Managing you own branch is pia.

I agree though that nobody will probably take care that no stuff is breaking (besides the existing tests of course) or add new Unity Features...
/cc @forki @cloudRoutine

@matthid matthid reopened this Apr 24, 2017
@forki
Copy link
Member

@forki forki commented Apr 24, 2017

good!

@ivaylo5ev
Copy link
Author

@ivaylo5ev ivaylo5ev commented Apr 24, 2017

Strange, now appveyor failed. I believe it is temporary as it passed the first time I submitted the PR.

@forki forki merged commit 521588a into fsprojects:master Apr 24, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ivaylo5ev
Copy link
Author

@ivaylo5ev ivaylo5ev commented Apr 24, 2017

Thanks guys, I really appreciate merging this. I hope that I will learn F# in the near future well enough to be of more use to this project. Paket really made a difference for me, I don't see myself returning to direct use of Nuget anytime soon.

@matthid
Copy link
Member

@matthid matthid commented Apr 24, 2017

To be honest the failures might even be related to this PR. Quite often in the code-base we enumerate the "AllProfiles" list. So adding new items there might have resulted in a performance degration.
However its really strange that first travis and now AppVeyor deadlocked because of these changes... Sadly the old Travis Build seems to be gone. Would be interesting to see if it was the same test as well..

Thanks @ivaylo5ev and have a good time with F#!

@baronfel
Copy link
Contributor

@baronfel baronfel commented Apr 24, 2017

@ivaylo5ev consider joining the slack channel for the F# Foundation when you want to learn more: http://fsharp.org/guides/slack/

@ivaylo5ev
Copy link
Author

@ivaylo5ev ivaylo5ev commented Apr 24, 2017

From what I remember, there was a connection timeout during one of the tests, which by that time justified the passing of appveyor - I thought it was a temporary Travis problem. Not sure if this is related to a deadlock, you might be right as well.

@matthid
Copy link
Member

@matthid matthid commented Apr 24, 2017

The AppVeyor log says it started the Integration Test at around the 11 minute mark and then no more output until the build exceeds the 60 minute limit... Yeah it could be a temporary connection problem as well. We should lower our limits within Paket such that we see a correct error message...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.