icsharpcode / ILSpy

.NET Decompiler with support for PDB generation, ReadyToRun, Metadata (&more) - cross-platform!
21.45k stars 3.35k forks source link

Retarget ICS.Decompiler to .NET Standard 2.0 #831

Closed christophwille closed 7 years ago

christophwille commented 7 years ago

Please see https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support

Minimum Framework compat will be 4.6.1 and Core 2.0. Dev dependency: VS2017 15.3 minimum plus .NET Core 2.0 SDK.

siegfriedpammer commented 7 years ago

Please see #833 for our final resolution, which introduces an independent project file for .netstandard. This is a workaround in order to avoid all the problems with multiple DLLs copied to the output folder and so on. Please be aware that the resulting .net std package is built, but not yet tested. The tests still use the old-style projects.

First of all, migrating to .NET Standard was not as simple as it is promoted by MS. Wasted more than 20 hours on different quirks. And yes, it was very frustrating...

Attempt no. 1:

First we migrated the ICSharpCode.Decompiler.csproj to the new format and actually planned to use only that in the whole solution. We added all the necessary nuget packages and except for one small change, we could use our codebase.

Important: If you use AssemblyInfo.cs in your project and have some tooling attached to generate version information, I'd recommend adding <GenerateAssemblyInfo>false</GenerateAssemblyInfo> to your new .net standard project, that way you can keep using the old-style AssemblyInfo.cs.

The next thing was getting the unit tests to work: We followed Scott Hanselman's advice, which worked fine until we tried to actually run the unit tests in Visual Studio. We got lots of strange exceptions, like MissingMethodException or FileNotFoundException telling us that something was missing. Except it wasn't, just the .NET Standard implementation got confused it seems.

Attempt no. 2:

Then we migrated one project after another from good ol' .NET to the new project format, hoping that it would solve our problems. But, we couldn't quite port our unit tests, because we are doing some advanced stuff there, like compiling C#, assembling IL and running nested unit tests (We're talking about a decompiler, testing it is not simple). One strange thing was: The .NET Core Test Runner wants to inject a custom Main method and implicitly turns the project into a console app (tooling bug?), which in turn fails to compile because we use a lot of Main methods in our test cases which get compiled during tests.

Then we added a second target using the TargetFrameworks attribute, so we just could use net461 for all the other projects. We ended up even converting our WPF UI to the new project format, despite the fact it does clearly not yet officially support WPF, using some tricks (https://stackoverflow.com/questions/43693591/how-to-migrate-wpf-projects-to-the-new-vs2017-format and https://github.com/dotnet/project-system/issues/1467). And it worked! Except for the XAML/BAML unit tests, we could not get those to compile, because the InitializeComponent methods were not recognized. But we don't understand why, because it worked with the UI project and we were using the same tricks in both project files.

At that point I decided to revert the XAML/BAML tests to the old project format and tried to get it to play nicely with the other projects. And in the end it worked, I had to remove some references and some code. And then I pushed my changes, to see if the build server was configured properly. And it failed...

Attempt no. 3:

Please see #833. First we introduced a new project with .netstd.csproj as suffix. It resides in the same directory as the old-style Decompiler library and compiles the same source files. While trying to get it to work, we were running into the following issues:

sharwell commented 7 years ago

:memo: I'm reading through your comments. Is this the best place to chat about it if I have questions on the approach(es)?

sharwell commented 7 years ago

AppVeyor/msbuild: yes, it does not respect any build dependencies properly, that's why we had to disable parallel builds, in our case this is not a big deal.

Microsoft/msbuild#2366

sharwell commented 7 years ago

❓ Do you have branches with "Attempt 1" and "Attempt 2"? I'm assuming #833 represents Attempt 3.

siegfriedpammer commented 7 years ago

Unfortunately I killed the other attempts by resetting (push --force) everything. We'll try to set up separate branches for the other attempts as well.

siegfriedpammer commented 7 years ago

@sharwell After upgrading the Mono.Cecil submodule in the repo to the latest commit, AppVeyor fails again, guess this is because Mono.Cecil now uses .NET Standard as well. On my machine this does not happen and in the ILSpy.sln file we specify net_4_0_Debug/net_4_0_Release as configurations for Cecil, but somehow msbuild/appveyor don't get it.

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\Microsoft\NuGet\15.0\Microsoft.NuGet.targets(178,5): error : Your project is not referencing the ".NETFramework,Version=v4.0" framework. Add a reference to ".NETFramework,Version=v4.0" in the "frameworks" section of your project.json, and then re-run NuGet restore. [C:\projects\ilspy\cecil\Mono.Cecil.csproj]
christophwille commented 7 years ago

@sharwell I luckily had copied the directory to the side from yesterday's attempt 2. The branch for you is https://github.com/icsharpcode/ILSpy/commits/netstandard-build-troubles - the last commit was us trying to get AppVeyor to work.

If I remember correctly: on the first build in VS, it doesn't find Longset private member (although AssemblyInfo.cs has the necessary definition). Right-click, build ics.decompiler and it is fine. From then on, VS locally is fine (except for a rare race condition on Clean for Cecil).

AppVeyor was more stubborn: never found Longset and totally didn't understand RevisionClass from AssemblyInfo.cs. The pertaining ci link: https://ci.appveyor.com/project/icsharpcode/ilspy/build/1.0.405

christophwille commented 7 years ago

@siegfriedpammer running the commands Appveyor uses (dotnet restore ilspy.sln, msbuild "ILSpy.sln" /verbosity:minimal ) exhibits the same problem. So it is definitely a msbuild tooling problem.

sharwell commented 7 years ago

@christophwille Thanks, I'll take a look!

sharwell commented 7 years ago

dotnet restore ilspy.sln

There's the problem. I totally overlooked it originally. I'll fix the PR.

Edit: Nope, AppVeyor is using the correct command already (nuget, not dotnet).

sharwell commented 7 years ago

I fixed it! 🎉

christophwille commented 7 years ago

Built from commit 949790d126f35c866f2be4580e712ff68d587574 I have uploaded a test Nuget package for testing purposes (Nuget, xplat): https://www.nuget.org/packages/ICSharpCode.Decompiler/3.0.0-alpha1 Please note the language support at https://github.com/icsharpcode/ILSpy/issues/829 - this package has less decompilation support than the officially released one.

christophwille commented 7 years ago

Super-small sample https://github.com/christophwille/ilspy-console-netcoreapp but please see https://github.com/icsharpcode/ILSpy/issues/845

siegfriedpammer commented 7 years ago

This was fixed in the new decompiler engine (which just landed on the master branch).