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

Retarget ICS.Decompiler to .NET Standard 2.0 #831

Closed
christophwille opened this issue Aug 30, 2017 · 14 comments
Closed

Retarget ICS.Decompiler to .NET Standard 2.0 #831

christophwille opened this issue Aug 30, 2017 · 14 comments

Comments

@christophwille
Copy link
Member

christophwille commented Aug 30, 2017

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 added a commit that referenced this issue Aug 30, 2017
Update ICSharpCode.Decompiler.Tests.csproj to new project format.
Seems like we cannot make the tests work with .NET Standard/.NET Core because we have some special cases:
- tests for UndocumentedExpressions depend on the full framework
- 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.
- Needed to update all the Nuget packages: most notably DiffLib
@siegfriedpammer
Copy link
Member

siegfriedpammer commented Aug 31, 2017

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 dotnet/project-system#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:

  • We needed to enforce that the netstd project is built before the normal projects, because they are both sharing the same intermediate output folder (see Old and new csproj files don't play nicely together dotnet/project-system#1935 and related issues). This involved adding a build dependency, so the old project depends on the netstd one.
  • The above issue requires us to remove the intermediate output directory after netstd is built (via plain old del command in a post build task), because otherwise the old project cannot be built any longer. We cannot do this the other way round because the project.assets.json is only generated by nuget. Any attempt of using IntermediateOutputPath, BaseIntermediateOutputPath or the new ProjectAssetsFile properties results in either one or the other project "forgetting" about builtin types among other things.
  • 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.

@sharwell
Copy link
Contributor

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

@sharwell
Copy link
Contributor

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.

dotnet/msbuild#2366

@sharwell
Copy link
Contributor

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

@siegfriedpammer
Copy link
Member

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
Copy link
Member

@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
Copy link
Member Author

christophwille commented Aug 31, 2017

@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
Copy link
Member Author

@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
Copy link
Contributor

sharwell commented Sep 1, 2017

@christophwille Thanks, I'll take a look!

@sharwell
Copy link
Contributor

sharwell commented Sep 1, 2017

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
Copy link
Contributor

sharwell commented Sep 1, 2017

I fixed it! 🎉

@christophwille
Copy link
Member Author

Built from commit 949790d 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 #829 - this package has less decompilation support than the officially released one.

@christophwille
Copy link
Member Author

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

@siegfriedpammer
Copy link
Member

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants