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

Migrating our projects from net46 to netstandard 2.0 #1963

Closed
6 tasks
vicancy opened this issue Aug 16, 2017 · 69 comments
Closed
6 tasks

Migrating our projects from net46 to netstandard 2.0 #1963

vicancy opened this issue Aug 16, 2017 · 69 comments
Assignees

Comments

@vicancy
Copy link
Contributor

vicancy commented Aug 16, 2017

https://github.com/dotnet/standard/blob/master/docs/versions/netstandard2.0.md
https://gist.github.com/davidfowl/8939f305567e1755412d6dc0b8baf1b7
Steps:

  • 1. Upgrade projects to net461
  • 2. Update documentation that VS 2017 Preview Update 3 is required
  • 3. Upgrade base projects to netstardard2.0 one by one

Blockers:

@vicancy vicancy self-assigned this Aug 16, 2017
@vwxyzh
Copy link
Contributor

vwxyzh commented Aug 16, 2017

v3.0?

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Hi @vicancy, do you have any particular place you'd prefer that I start? Otherwise, I'm happy to just dive in at the deep end and see where I get up to :)

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Work starting here.

tintoy added a commit to tintoy/docfx that referenced this issue Sep 29, 2017
…Microsoft.DocAsCode.YamlSerialization projects to .NET Standard 2.0.

dotnet#1963.
@vicancy
Copy link
Contributor Author

vicancy commented Sep 29, 2017

@tintoy Looks cool!

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

A couple of tests are failing, but I'm just checking to see if they actually succeed without these changes :)

@vicancy
Copy link
Contributor Author

vicancy commented Sep 29, 2017

@tintoy If you are using VS 15.3 and the tests are Metadata related, it is possibly an env issue with MSBuild, opening All.sln from VS2017 Developer Command can fix it.

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Ah yes, thanks - those are the ones that are failing :)
I can probably fix that in another PR, I think (ran into a similar problem on another project).

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

All good, with your suggested workaround, all tests pass so far. I'll keep going.

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Ok, bit of a blocker - all dependencies except Nustache support .NET Standard (project seems to be somewhat dead - jdiamond/Nustache#121). I will see if I can find a compatible alternative (the project owner suggests stubble).

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Wow, OK, yak-shaving time. There is no Mustache implementation currently compatible with .NET Standard (any version!). Looks like I'll have to create a .NET Standard port of Nustache (or similar).

Anyone else who'd like to help - now's the time to pitch in :)

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Actually, let me see if it'll still run on .NET Standard 2.0 first (I get the usual NU1701 but it may still work correctly).

@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Ok, a more serious blocker: dotnet/roslyn#17968. There's currently no MSBuildWorkspace for .NET Standard. I might have another poke around the OmniSharp source code to see how hard it would be to create an interim workaround for this.

tintoy added a commit to tintoy/docfx that referenced this issue Sep 29, 2017
@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Have ported everything except CLI, tests, and Microsoft.DocAsCode.Metadata.ManagedReference for now. Will have a think about what it would take to make Microsoft.DocAsCode.Metadata.ManagedReference work on .NET Standard but it sounds like this library in particular may be a bit trickier to port.

tintoy added a commit to tintoy/docfx that referenced this issue Sep 29, 2017
@tintoy
Copy link
Contributor

tintoy commented Sep 29, 2017

Probably have a solution for Microsoft.DocAsCode.Metadata.ManagedReference:

https://gist.github.com/tintoy/76b48591e2a34429c1f5c02556b8b4d2#file-program-cs

Essentially, we can use an AdhocWorkspace and populate it using the MSBuild engine to examine the project's properties and items; a slightly-less-sophisticated version of what MSBuildWorkspace does.

This may not work for every single project out there (e.g. if a project has <Compile> items that are added by custom tasks that can only run on the full .NET framework because they're AppDomain-isolated) but it should be fine for 95% of use-cases (chances are, the existing metadata extractor doesn't work for 100% of projects right now anyway).

@vicancy - before I go any further on this can I get some feedback from you on whether you find this approach acceptable?

@vicancy
Copy link
Contributor Author

vicancy commented Oct 10, 2017

@tintoy Sorry for late response. I was on vacation and am back now. I will take a look at it today.

@tintoy
Copy link
Contributor

tintoy commented Oct 10, 2017

@vicancy no problem, take your time - I've got plenty of other stuff to keep me busy :)

@vicancy
Copy link
Contributor Author

vicancy commented Oct 10, 2017

@tintoy Thinking of changing from Mustache to Handlebars, https://www.nuget.org/packages/Handlebars.Net/ suports .NET standard

@tintoy
Copy link
Contributor

tintoy commented Oct 10, 2017

Nice - I totally forgot about Handlebars; how compatible is the syntax between the 2? It's been ages since I used Handlebars, but I seem to remember it's basically a superset of Mustache...

@vicancy
Copy link
Contributor Author

vicancy commented Oct 10, 2017

@tintoy It generally compatible with Mustache, the main difference is that it does not search up when context does not found, which is exactly what we expect. There may be other differences that leads to a breaking change (which is my main concern) so I need more time to test before switching. 😢

@tintoy
Copy link
Contributor

tintoy commented Oct 10, 2017

Yeah, I see what you mean, and it looks like there are only a couple of tests for the template processor at the moment. Still, that's the great thing about Xunit theories - write 1 test and then just keep adding test data 😎

@vicancy
Copy link
Contributor Author

vicancy commented Oct 17, 2017

@tintoy with the 95% support, is this <Comiple case the only case that the AdhocWorkspace does not support?

@tintoy
Copy link
Contributor

tintoy commented Oct 17, 2017

It can support it, we just have to use Microsoft.Build.Evaluationto evaluate the Compile item group :) I can whip up an example if you like

@tintoy
Copy link
Contributor

tintoy commented Oct 17, 2017

(just may not work for every project e.g. the exceptions listed above)

@tintoy
Copy link
Contributor

tintoy commented Oct 17, 2017

@vicancy - this demonstrates evaluating the <Compile/> items and adding them to the workspace.

@tintoy
Copy link
Contributor

tintoy commented Nov 24, 2017

Ok - I'm about to go camping for the weekend but I'll get onto it early next week :)

@Romanx
Copy link

Romanx commented Nov 24, 2017

@vicancy We're having issues with the changed tests not working on @tintoy's environment and working on mine. Hoping to find some time this weekend to get to it

tintoy added a commit to tintoy/docfx that referenced this issue Nov 26, 2017
Remove support for project.json (we only support MSBuild-style projects now).
@tintoy
Copy link
Contributor

tintoy commented Nov 26, 2017

@vicancy have a look at 6864ff2 - that should give you an idea of the scope of changes I have to make (it compiles but I haven't tried any of the tests yet).

@tintoy
Copy link
Contributor

tintoy commented Nov 26, 2017

Actually, I'm thinking I might create extension methods for AdhocWorkspace to emulate the old OpenProjectAsync APIs. What do you reckon?

@tintoy
Copy link
Contributor

tintoy commented Nov 26, 2017

Turns out AdhocWorkspace is mutable and therefore may not be thread-safe. Will investigate :)

@tintoy
Copy link
Contributor

tintoy commented Nov 26, 2017

Ok turns out it's thread-safe.

tintoy added a commit to tintoy/docfx that referenced this issue Nov 26, 2017
…ble versions of Microsoft.NET.Test.Sdk and Xunit).

dotnet#1963
@tintoy
Copy link
Contributor

tintoy commented Nov 28, 2017

@vicancy would you mind pulling down the latest code from my fork and running the tests? There are a couple I don't understand well enough to work out why they are failing... It's the feature/netstandard branch.

@vicancy
Copy link
Contributor Author

vicancy commented Nov 28, 2017

@tintoy Sure, I will take a look

@vicancy
Copy link
Contributor Author

vicancy commented Nov 29, 2017

Hi @tintoy Can build pass with the feature/netstandard branch? Mine can't pass build, looks like System.Runtime.Remoting.Lifetime; is not available in netstandard.
image

@tintoy
Copy link
Contributor

tintoy commented Nov 29, 2017

Which project was that? I didn't see any build errors but I was just running tests from within VS. I'll have a look at that first thing tomorrow, should be easy to fix either way.

@tintoy
Copy link
Contributor

tintoy commented Nov 29, 2017

I haven't tried porting the main executable yet, only the libraries it depends on. The exe will take some more work because it currently relies on Appdomain but I do have a proposed design that will work cross-platform. In the meanwhile, would you mind trying to run the unit tests ignoring that executable build error?

@tintoy
Copy link
Contributor

tintoy commented Nov 29, 2017

BTW, am I correct in assuming that use of Appdomains is mainly to control base path for loading assemblies? If so, AssemblyLoadContext is the replacement.

@vicancy
Copy link
Contributor Author

vicancy commented Nov 30, 2017

@tintoy AppDomain is to isolate assemblies loaded from plugins. We are thinking of creating a new process instead.

@tintoy
Copy link
Contributor

tintoy commented Nov 30, 2017 via email

@vicancy
Copy link
Contributor Author

vicancy commented Nov 30, 2017

@tintoy I changed docfx and docfx.test projects back to use base.props and can pass build now. I am running test and meet some runtime assembly loading errors. I am looking into it.

@tintoy
Copy link
Contributor

tintoy commented Nov 30, 2017

@vicancy
Copy link
Contributor Author

vicancy commented Dec 1, 2017

Hi @tintoy I also meet several kind of ut failures with your changes. However that branch is quite behind the latest branch and contains a lot of changes. I'd like to make small moves step by step for easy debugging and code review, so I am cherry-picking your changes and sending out PR for every commit.

vicancy pushed a commit to vicancy/docfx that referenced this issue Dec 1, 2017
…Microsoft.DocAsCode.YamlSerialization projects to .NET Standard 2.0.

dotnet#1963.
vicancy added a commit that referenced this issue Dec 1, 2017
* Migrate Microsoft.DocAsCode.Common, Microsoft.DocAsCode.Plugins, and Microsoft.DocAsCode.YamlSerialization projects to .NET Standard 2.0.

#1963.

* Some format change
@tintoy
Copy link
Contributor

tintoy commented Jan 4, 2018

@vicancy - I recently stumbled across Buildalyzer, which seems to have fairly full-featured support for building a Roslyn AdhocWorkspace from project files in most formats:

image

Might be worth considering?

@vicancy
Copy link
Contributor Author

vicancy commented Jan 5, 2018

Hi @tintoy thank you for the info, I will take a look. With a first glance, it looks like also dependent on msbuild.dll to load and parse the project, so I am now a little confused why we not use MSBuildWorkspace, it is from https://www.nuget.org/packages/Microsoft.CodeAnalysis.Workspaces.Common/ and has a netstardard1.3 version...

@tintoy
Copy link
Contributor

tintoy commented Jan 5, 2018

I think MSBuildWorkspace is in the package Microsoft.CodeAnalysis.Workspaces.Desktop, which does not target .NET standard (yet)?

@tintoy
Copy link
Contributor

tintoy commented Jan 5, 2018

AdhocWorkspace is in Microsoft.CodeAnalysis.Workspaces.Common though.

@vicancy
Copy link
Contributor Author

vicancy commented Jan 5, 2018

@tintoy Oh yes, netstardard1.3 does not contain Microsoft.CodeAnalysis.Workspaces.Desktop assembly

@superyyrrzz
Copy link
Contributor

Close as v2 is not likely to support it. v3 will.

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

5 participants