-
Notifications
You must be signed in to change notification settings - Fork 731
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
Retarget Solution to use the New CSPROJ Format #2380
Conversation
…y are still needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes don't need close review, just a skim. The big changes are in the added project files.
@@ -32,13 +32,13 @@ | |||
[assembly: AssemblyTrademark("NUnit is a trademark of NUnit Software")] | |||
|
|||
#if DEBUG | |||
#if NET_4_5 | |||
#if NET45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These defines are automatically included now and this is the default format, so I switched all code to use the defaults.
<PropertyGroup> | ||
<AssemblyName>nunit.framework</AssemblyName> | ||
<RootNamespace>NUnit.Framework</RootNamespace> | ||
<TargetFrameworks>net40;net45;netstandard1.3;netstandard1.6</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the new format csproj
files. It is much shorter and only includes files that are outside of the project directory.
@@ -239,7 +239,7 @@ public string RunTests(string filter) | |||
return result.OuterXml; | |||
} | |||
|
|||
#if !NET_2_0 | |||
#if !NET20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes in this PR are of this nature and can be skimmed over.
global.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"projects": [ "src" ], | |||
"sdk": { | |||
"version": "1.0.0-preview2-003121" | |||
"version": "1.0.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also pegged this at the 2.0.0 tools and it works fine if we decide to go with that.
Ahh, I was so looking forward to multi-targeting! Shame it comes with so many problems. 😞 I use VS 2015 at work, so not being able to build there wouldn't be ideal. We pull in a NuGet package so we can use C#7 features in VS 2015 - is there no equivalent for building the new csproj in VS 2015?
The latter's a real pig. I can imagine many contributors making changes in the main solution, which then fail in .NET 2.0, when caught by PR CI. We saw occasional problems when we had the .NET CF build outside the main solution - I think .NET 2.0 will likely catch people out even more. If they could live in the same solution, I'd be happier. If not, we best make sure our documentation is up to scratch! Overall - I think we need to solve #2331 - and I don't think we should be relying on this unsupported shim we're currently using long-term. I don't particularly like how this solution looks, but do think it's an improvement on where we are currently. Shame on Microsoft for their lack of backwards-compat consideration! 😄 |
@ChrisMaddock I agree with you on all points, it really sucks that there isn't a better solution. I've been working on this problem for several weeks now and I've tried numerous hack to get the NuGet packages to restore, but nothing worked. I am not wed to this idea/PR, it is just the first thing I got working, so I wanted to put it up for discussion. Personally, I don't think we should merge this at this time, but it is likely the direction we want to go to eventually. Beyond this, I can only think of two alternatives that would work for restoring the packages,
The second is probably the best option, but still not ideal. |
I think a pre-build step would be a good solution. I'd love to use the multi-targeting feature - it would make things so much easier to manage. But I feel like removing .NET 2 and 3.5 from the main solution would currently cause us more problems than it solves. It's one more reason to consider our ongoing development of the older platforms however... |
🎉 I've successfully multitargeted I'm still loathe to give up the pre-net40 builds.
That's pretty significant, a core team member not being able to use VS2017 some of the time. It points to the possibility of this being a wider impediment. I'm the last person that wants to say this, but should we consider waiting? Or leaving the old projects in existence, renaming them
Do we? I didn't think we did but that would be great news and we should update our But as far as I know, no, there is no equivalent. This goes back to our other conversation on building projects via NuGet bin-deployed MSBuild without VS. They are still figuring out the architecture for putting the SDKs on NuGet and resolving them, and without those SDK |
If we could do this, I think this would be great. I think it's reasonable to ask people to install the latest (free!) IDE to build the solution. Splitting out certain projects is the bit that's a potential blocker for me.
Ironically, I've just been given a VS2017 licence today! :-D Our issue is that we purchase licences outright, rather than on subscription. The updates between 2015-2017 just didn't justify the cost of buying everyone new licences, for the stuff we work on. I do like the idea of having separate 2017 and pre-2017 solutions though - that should be an easy solution to keep things accessible to everyone. 😄
Sorry, @jnm2 - that 'we' was referring to 'we' at work! I'm not sure if there would be an issue doing it for NUnit or not - I'm pretty sure 'we at NUnit' don't currently. |
@jnm2 I would be curious how you managed to get Another disadvantage of the multi-targeting is that I don't know if it works in MonoDevelop yet. |
To be honest though, I didn't even try |
I think we will have problems keeping the project files for both of those in sync. The new |
Good point. 😞 |
@rprouse We could have build.cake autogenerate the appropriate legacy sdk csproj files– Yeah, what could possibly go wrong? 😈 Thanks for the dotnet/msbuild#1333 link. IIRC I built using desktop msbuild (Cake's
Oh I see! Haha sorry! |
I've pushed changes that might just make this a viable solution. I took @jnm2's advice and added the At this point I am going to try on Mac and on Linux and see what works. |
On Linux, MonoDevelop doesn't support .NET Core or the new CSPROJ format yet. Some people say that Visual Studio Code will work, but I am not sure with the mix of Mono and .NET Core. On Mac, the projects load, but NuGet restore fails when you attempt to build. That might just be some mistakes in the NuGet packages and might be fixable. VS for Mac is using Mono 5.2 under the covers for the .NET Framework support. |
Too bad about MonoDevelop and VS for Mac. Have you found or started issues to follow yet? |
Just a small comment, at my workplace, we're considered lucky for even being able to develop in VS2015 because moving to new versions of anything is incredibly slow. I can't see us needing to build NUnit at all though, if it were an open source toolset allowable by our overly bloated beaurocratic structure. :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
BUILDING.md
Outdated
MonoDevelop is updated, it should start working again. Until then, we recommend | ||
[Visual Studio Code](https://code.visualstudio.com/) and compiling using the build | ||
scripts on non-Windows platforms. | ||
The framework is built using a single Visual Studio solution, `nunit.sln`, which may be built with [Visual Studio 2017](https://www.visualstudio.com/vs/) on Windows and [Visual Studio for Mac](https://www.visualstudio.com/vs/) on macOS. Currently, MonoDevelop does not support the new multi-targteted `csproj` project format. Once MonoDevelop is updated, it should start working again. Until then, we recommend [Visual Studio Code](https://code.visualstudio.com/) and compiling using the build scripts on non-Windows platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Missed 'targteted' typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the spelling mistake. Good catch.
@rprouse You and I are both happy with this and the magnitude is somewhat daunting so I'm not sure if Chris and Mikkel are going to be happy reviewing... :D Shall we merge or is there a specific review we should look for or summary we should write up first? |
Feel free to go ahead without my review. If you'd prefer me to take a look, I may be able to tomorrow night. (GMT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's so much work in here - looks amazing! I'll try to pull it down and build it tomorrow evening, feel free to merge without that if you'd prefer to get it done however!
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="'$(TargetFramework)' == 'net20' OR '$(TargetFramework)' == 'net35'"> | ||
<DefineConstants>TRACE;NUNIT_FRAMEWORK;PARALLEL</DefineConstants> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One for the clean-up, @jnm2 - I think we could probably remove the NUNIT_FRAMEWORK
constance? I think that's left from when framework and engine we in the same repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisMaddock Also on my list, but in this case, there are files linked into the NUnit and NUnitLite projects so I should double check that none of our code uses these constants.
|
||
var AllFrameworks = IsRunningOnWindows() ? WindowsFrameworks : LinuxFrameworks; | ||
var AllFrameworks = new string[] { | ||
"net45", "net40", "net35", "net20", "netstandard1.6", "netcoreapp1.1" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to maintain this list of frameworks anymore? Or could we just iterate through all the folders in BIN_DIR
? This way is belt and braces, I guess, but extra maintenance.
|
||
// Packages | ||
var ZIP_PACKAGE = PACKAGE_DIR + "NUnit.Framework-" + packageVersion + ".zip"; | ||
|
||
bool isDotNetCoreInstalled = false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This packages
in no longer uses. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(There are also some helper methods which are unused, but unrelated to this PR e.g. RunGitCommand. Might be worth a look while we're in the area, but it's a separate issue!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages
is on my list of cleanup activities :D Did I add RunGitCommand
? If so it had a target at one point to test whether the repo could build after a git clean -xfd
.
Framework = "netcoreapp1.1", | ||
Configuration = configuration, | ||
OutputDirectory = BIN_DIR + "netcoreapp1.1/" | ||
}); | ||
}); | ||
|
||
////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, but the two Test
and Test Framework
headers here seem out of place, and the CheckForError task - mind tidying up while we're here? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice. Rob, do you want to handle fixing the headers or should I?
Btw Chris your comments are all on the line above rather than the line below so I have to follow the link to the full changes to see what you're commenting on. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - because the lines I’m commenting on are unchanged this seemed to be as close as I could get them! 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point!
.OnError(exception => { ErrorDetail.Add(exception.Message); }) | ||
.Does(() => | ||
{ | ||
var runtime = "net-4.5"; | ||
var runtime = "net45"; | ||
var dir = BIN_DIR + runtime + "/"; | ||
RunNUnitTests(dir, FRAMEWORK_TESTS, runtime, ref ErrorDetail); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given all this is rewritten and working on different platforms, could we check if the InProcess workaround flag is still needed for Travis, which is inside this RunNUnitTests method? Again - could wait till clean up if we'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added to my list. I'd prefer to wait and do it with a later cleanup PR.
@jnm2 and @ChrisMaddock this PR has been merged 🎉 I have a PR coming in a few minutes removing the unused DEFINES. @jnm2 do you want to do the Cake cleanups that @ChrisMaddock flagged? |
🎆 🍾 No seriously, 🍾 is not a bad idea. Thanks everyone for your valuable input! @rprouse Yes. This is my text file tracking my perfectionist tendencies for cleanup PR(s):
(Edit: Rob moved this list to https://github.com/orgs/nunit/teams/framework-and-engine-team/discussions/3 where it is discussed and updated.) |
Great work 🥇 . Sorry I did not have time to review, but I've only had time to do very little the last couple of weeks, so just keeping track of all the work done in the projects has eaten most of my time. Hopefully, I'll now have time to contribute some more. |
Do not merge
Fixes #2331
Fixes #2515
This might be a controversial change, so I want to put it in front of the team before I do too much work. First, a bit of history, when we first added .NET Standard support, we knew that
project.json
would be replaced with the newcsproj
format and that the two formats would be incompatible. The newcsproj
format also wasn't available. Instead of using thexproj/project.json
format which would have a short shelf life, someone at Microsoft recommended that we use a little known hack and compile the .NET Standard projects using PCL style CSPROJ files.This worked, but it isn't supported or well tested and we have run into numerous issues with this approach like #2331 where NuGet packages are not restored unless you restore them at the command line, or the numerous package downgrade warnings.
Now that Visual Studio 2017 has been out for months and the second version of the CSPROJ tools for .NET Core/Standard has been released, I decided to see what it would take to convert our projects to the new format. The advantages of the new format is,
This means that the solution now looks like this,
And if you want to edit with a specific target, you select it at the top of the editor,
This requires Visual Studio 2017 15.3 or 15.4 or Visual Studio for Mac along with the .NET Core 2.0 SDK.
This requires that you have Visual Studio 2017 or Visual Studio for Mac installed along with .NET Core 1.1.2 with SDK 1.0.4. This is an update from the previous version of .NET Core that was required because the previous version did not support the CSPROJ format. I didn't use .NET Core 2.0.0 with SDK 2.0.0 because it was still in preview when I started this. We could change and use it, I think it is the easiest install to explain, but it requires Visual Studio 2017 15.3 which was only released a week ago.Unfortunately, multi-targeting does not work with .NET 2.0 or 3.5, so I had to leave those solutions as they were. They also conflict with the other projects in Visual Studio, so they are in a new solution called nunit.legacy which replaces the Linux solution. This solution compiles on all platforms and in all IDEs, but only includes 2.0 and 3.5.Both of these solutions are building in Visual Studio without warnings or errors. I still need to redo the
build.cake
to build, test and package at the command line, but I wanted to get feedback before putting the work in. We weren't planning on moving to Visual Studio 2017 this soon, but it does clean the build up and users with other IDEs can still contribute with the legacy solution.Thoughts? Rants? 😄