fsprojects / TickSpec

Lean .NET BDD framework with powerful F# integration
Apache License 2.0
134 stars 23 forks source link

Enable compilation in VS2017 #3

Closed michalkovy closed 6 years ago

michalkovy commented 6 years ago

Currently it isn't possible to build TickSpec solution in Visual Studio 2017.

I am trying to make the change as minimal as possible, at the end adding FSharp.Core using Nuget galley by Paket is what is the solution. It still is .NET 4.0 with F# 3.1.

This change also contains removal of one app.config file. That file was there but wasn't part of .csproj file, so it wasn't actively used and basically it was empty config anyway.

mchaloupka commented 6 years ago

I guess that it would be needed to also modify the post build steps, so the nuget will be created correctly.

Moreover, does this work with the other solution files? Like VS 2010 or Silverlight. Not sure whether they are needed anymore, but in that case they should be rather removed than kept not working.

bartelink commented 6 years ago

Hi @michalkovy - thanks for taking the time to work on these ideas; I'm very hopeful we'll get some very helpful improvements in there over time.

While the chances are that Silverlight and 2008 support can indeed be archived without anyone feeling any pain, I'm less sure about forcing 4.5 on people if it's avoidable. In terms of supporting other targets, for me .NETStandard 2 is more interesting in the medium term. I'd really like to hear @ptrelford's ideas as to a good base version to support and/or have his ack that there's nobody that's going to be inconvenienced if we outright remove the older solution files.

Getting the NUnit integration to work with an in-date version of NUnit obtained via Paket is absolutely a win - if we can do a minimal approach to that first and then worry about supporting other environments / 2017 as second step/discussion that'd be great.

One thing though; looking at the other PR's I'm also keen to a) get them in - these are largely good things; please don't take this as me batting it all back b) take it slowly - it would be good to work on one thing at a time

In terms of DI approaches, I think @mchaloupka's work is within touching distance of being ready for inclusion; as mentioned over on #2, if we can get it rejigged slightly such that the changes to the core of tickspec are minimal yet still afford us a good general extension point, then this is a good basis to start from. Perhaps you and Miloš might work together to get to a point where we have: 1) a small incision in the core tickspec (I think having something with a constructor selection strategy in the core is a step too far) 2) a good example for NUnit that >1 person will use for real things 3) ideally an example of using the same mechanism cleanly with xUnit Then that can be merged.

Finally, I'd like to reiterate that I'm about as busy as I'm going to be this year for the next month - if stuff is not minimal and easy to review, I can't commit to being able to respond in a timely fashion unless we work in very small crisp steps.

The other thing worth pointing out is that I personally am not au fait with the NuGet publishing steps wrt TickSpec and am keen to make sure we keep the releases in step withmaster if at all possible, so I personally will be relying on others to ensure that's covered adequately..

michalkovy commented 6 years ago

I have added removal of Silverlight, VS2008 and VS2010 solutions. I haven't found any problem with .nuspec files but Milos was correct that build.bat would be failing on machine with VS2017 as VS2017 doesn't support Silverlight. After the removal Silverlight the build.bat works.

I would like to avoid need to upgrade to .NET 4.5 but unfortunately I haven't found a way how to build it in VS2017 with .NET 4.0. One option would be to leave VS2010 projects in place and try for future to leave the code compatible with F# 3. However, I feel that .NET 4.0 may be needed just for old projects which can use older version and the overhead of taking care of old platform isn't worth it.

You can see that I made this as first step before the other pull requests, one reason was that I didn't want to install old Visual Studio, but other is that they would be more work/re-work. E.g. Paket today is already setup the way that paket.dependencies contains: framework: >= net45 and the xunit example project uses .NET 4.5.2. So, it made better sense to get all projects to .NET 4.5 first and then start adding NUnit using Paket.

Thank you

bartelink commented 6 years ago

I def agree compiling with 2017 is not optional. Removing Silverlight makes sense (maybe add a paragraph to the README mentioning the commit and/or NuGet version where it's removed with the justification above).

I'm not suggesting there's necessarily value in supporting F# 3.0, but unquote manages to support 3.1 (admittedly with lots of gymnastics).

I do agree that in terms of balancing reasonable time investment with broad support, F#4, .NET 4.5 may be tenable. Also, speaking of being a devil's advocate: I'm also the person who added the 4.5 dep on the other side (xUnit 2 mandates that and there's no point in even talking about xUnit 1).

So, in short - as long the PR discussion and/or overview mentions the reasons these updates happened, they are valid to do in my eyes.

Ping me when you're happy this PR is as minimal as it can be and I'll check it on 2015 and 2017 - a README update mentioning your rationale would be especially valuable.

bartelink commented 6 years ago

Sorry, one final thing though: while the xUnit example requires 452, that change for example did not force core to require 4.5 IIRC

While in general one wants to avoid groups in paket.dependencies, they may be appropriate here (also perhaps framework: autodetect ?)

michalkovy commented 6 years ago

Good news, I was able to get VS2017 working without a need to change neither .NET version nor F# version. The real problem was that FSharp.Core for F# 3 isn't available in VS2017 installation. However, it is in Nuget gallery, so the solution is simply to take the FSharp.Core from there.

To minimize the size of this change I have also removed the removal of Silverlight and removal of old VS projects from this pull request, let's process it separately, it isn't needed for VS2017 to be able to compile it using IDE. The build.bat fails on VS2017 computers because of the Silverlight compilation as Silverlight SDK isn't in place.

bartelink commented 6 years ago

Thanks, the diffs on this look great from scanning.

I don't have time to do a NuGet push or play with/verify the build scripts today but am happy to merge; I assume @mchaloupka is good with that too.

You ok with a squash merge or do you want to remove the reverts etc?

mchaloupka commented 6 years ago

Looks good to me. In the future, it may be probably migrated to newer F# and .NET Core if possible, but this is a nice change as it enables usage of VS 2017.

bartelink commented 6 years ago

@michalkovy OK, will be AFK for a bit but assuming I don't hear anything from you I'll squash merge later; thanks for doing the legwork (and for bearing with me!)