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

Invalid Project Format Exception if Project is new project file format #11

Closed
ManfredLange opened this issue Aug 1, 2017 · 10 comments
Closed
Milestone

Comments

@ManfredLange
Copy link
Contributor

ManfredLange commented Aug 1, 2017

If a solution contains at least one project (*.csproj) that is in the file format for .NET Core applications, vs-project-loader fails with exception. This prevents migrating scripts that invoke nunit3-console with the solution file where the solution file contains a mix of projects (e.g. .NET Core/.NET Standard and full .NET Framework).

I would like to suggest two possible solutions:

  1. Addin vs-project-loader would indicate that it cannot handle the new file format and that way allow the implementation of other addins who can.
  2. Addin vs-project-loader is modified to also being able to handle the new project file format

Note: The project was created using the latest template for a .NET Core Console application in Visual Studio 2017 with all the latest patches/updates/etc applied.

Perhaps if I have overlooked something please let me know, e.g. if you are planning to take a different route to support the new project file format. I have a locally patched version of this addin that works for us but I do not think that it is good enough for a pull request. Happy to make the modified source file available without warranty that it will work in other environments. Let me know. It's more like a hack than anything.

@ChrisMaddock
Copy link
Member

if you are planning to take a different route to support the new project file format.

I don't think we have a plan yet! 😄

This extension often faces issues when new project formats are released, as it reads the xml manually, and therefore falls over if this xml changes. This seems to be the best path of attack, in .NET 2.0, which this extension currently supports.

I've considered before trying to write a non-.NET-2.0 version, using the Microsoft Build NuGet packages - this is never something I've actually made progress on however. Theoretically, we could package both assemblies in the NUnit.Vs.Project.Loader package, and the engine could resolve the highest possible extension, for the platform it is currently running on.

That's a much bigger idea though - and something I've been thinking of for a couple of years, and still haven't made the time for. It may never happen! What's your hack - would you put up a PR, and @nunit/engine-team can see what we think? In my opinion, a well commented hack is better than nothing!

@ManfredLange
Copy link
Contributor Author

ManfredLange commented Aug 3, 2017

Thanks, Chris. I'll create a fork and create a PR. I have never done a PR, so would like to ask for some patience if I don't get it right the first time. Thanks! I won't be able to do the PR right away but hopefully next weekend allows me to set aside some time for this.

I probably would not want to use the msbuild package at the moment. msbuild has a bug as well with mixed solutions in that it turns build dependencies into project references which doesn't go down well if you have a net45 assembly "referencing" a netcoreapp1.1 .... ;-) Therefore my solution does not make use of it. At the moment it's inserting one line into existing code in VSProject.cs and then adding another function with about 15 lines of code. Nothing major. I'll have to think about appropriate tests. The existing tests have already a few sample project files, so will follow that example. Should be fun!

@ChrisMaddock
Copy link
Member

ChrisMaddock commented Aug 3, 2017

@ManfredLange - Great! No problem - expect us not to 'get it right first time' too! There's often a bit of back and forth while we all figure out the best approach to a problem - but there's a good team here to help. 🙂

I didn't know that about MSBuild - nightmare! Maybe I can put this off for another year or two, once all the changes have settled down a little. ;-)

@jnm2
Copy link

jnm2 commented Aug 3, 2017

@ChrisMaddock I've tried the MSBuild NuGet package route too but that's a dead end until we get the SDKs published on NuGet (see dotnet/msbuild#1493 and dotnet/msbuild#1439 and NuGet/Home#5220).

@ChrisMaddock
Copy link
Member

@jnm2 - I thought the Microsoft.Build package I linked to was that! Sounds like you've dug into this more than I have...I'll take it I'm wrong. 😞

@jnm2
Copy link

jnm2 commented Aug 3, 2017

@ChrisMaddock Yep, I played with it for an evening. 😞 That's just MBuild itself with no C# .targets.

@ManfredLange
Copy link
Contributor Author

ManfredLange commented Aug 3, 2017

Here is the link to the msbuild issue when build dependencies are treated as project references dotnet/msbuild#2274. I didn't check if this impacts the behavior of the msbuild-nuget package as well.

We have worked around this issue by building a little tool that reads the solution file, all project files in the solution then create our own build order.

@ManfredLange
Copy link
Contributor Author

Just sent a PR with a fix for this issue. Please let me know what you think.

@ManfredLange
Copy link
Contributor Author

ManfredLange commented Aug 6, 2017

Four of the tests I added are failing on Travis. I can't reproduce locally at the moment. If you could give me a hint for how to find out what environment Travis is using I might be able to reproduce and then fix those tests.

Update 07 Aug 2017: The tests are now passing on Linux and MaOS as well with the latest commit on the PR.

@CharliePoole
Copy link
Contributor

Closed by PR #12

@ChrisMaddock ChrisMaddock added this to the 3.7 milestone Nov 16, 2017
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

No branches or pull requests

4 participants