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

Migrate from MSBuild to Roslyn #149

Closed
AndreyAkinshin opened this issue Apr 27, 2016 · 16 comments
Closed

Migrate from MSBuild to Roslyn #149

AndreyAkinshin opened this issue Apr 27, 2016 · 16 comments
Milestone

Comments

@AndreyAkinshin
Copy link
Member

Problem

Currently ClassicBuilder uses a combination of BuildManager.DefaultBuildManager and BuildBenchmark.bat for project building. There are a lot of troubles with this approach:

  • It is hard to detect correct version of MSBuild
  • On Linux and MacOS, it is also hard to detect correct version of XBuild
  • It is impossible to move a benchmark runner to another machine without installed MSBuild/XBuild.

Suggestion

There is a cool NuGet package Microsoft.Net.Compilers which contains full Roslyn compiler (csc.exe + additional dll files). Yep, it is a dependency, but It is not too terrible. Benefits of the suggested approach:

  • We shouldn't detect MSBuild/XBuild
  • We can run benchmark without installed MSBuild/XBuild
  • We even shouldn't generate csproj
  • We always have good stable compiler, we don't depend on user environment
  • It is a cross-platform way.

@adamsitnik, @mattwarren, what do you think?

@adamsitnik
Copy link
Member

@AndreyAkinshin To be honest it sounds great! The only disadvantage I see right now is .NET 4.5+ support which means that we have to drop .NET 4.0 support. But on the other hand this should not be a big deal since most people that write benchmarks know that each .NET framework release brings performance updates and they are up to date.

I am going to spend few hours at the airports tomorrow and think more about this!

@AndreyAkinshin
Copy link
Member Author

Yes, you need installed .NET 4.5 (or mono) but only for compiling. You still can target your benchmark methods to .NET 4.0.

@mattwarren
Copy link
Contributor

Yeah I like this idea, it's the approach that I originally took with MiniBench before I stopped that and started working with you on BenchmarkDotNet.

I agree with @adamsitnik, the only issue I can see is that it forces people to have .NET 4.5 when building the benchmarks, but as he says, that's probably only a small amt of users.

BTW you should take a look at CodeGenerator.cs, it's using Roslyn directly, but should give you some ideas.

@AndreyAkinshin AndreyAkinshin added this to the v0.9.x milestone May 6, 2016
@AndreyAkinshin AndreyAkinshin modified the milestones: v0.9.7, v0.9.x May 11, 2016
@damageboy
Copy link
Contributor

Wouldn't you like to also take a dependency on a MSBuild nuget package while at it? Or is xbuild now considered safe enough?

@AndreyAkinshin
Copy link
Member Author

@damageboy, I don't want to have the MSBuild dependency. I want to compile Program.cs by Roslyn.

@damageboy
Copy link
Contributor

Understood, so you want to abolish the current build generators and go straight to the compiler?

@AndreyAkinshin
Copy link
Member Author

I think, we will keep current toolchain system. We should just write another Builder (e.g. RoslynBuilder instead of ClassicBuilder which based on MSBuild).

@AndreyAkinshin AndreyAkinshin changed the title Suggestion: migrate to Microsoft.Net.Compilers [Suggestion] migrate to Microsoft.Net.Compilers May 15, 2016
@AndreyAkinshin AndreyAkinshin self-assigned this May 15, 2016
@jskeet
Copy link

jskeet commented May 25, 2016

One small request while we're changing this: could we give the generated C# files a different extension? If the runner is killed while executing tests, the C# code stays behind, which then confuses dotnet build. Happy to file that as a separate feature request if it would be better.

@AndreyAkinshin
Copy link
Member Author

Yes, I think we need a separate issue for this feature.

@jskeet
Copy link

jskeet commented May 25, 2016

Filed #192.

@AndreyAkinshin AndreyAkinshin modified the milestones: v0.9.8, v0.9.7 May 29, 2016
@AndreyAkinshin
Copy link
Member Author

I have tried 3 different approaches. Here are some results:

Approach 1

Add a NuGet reference to Microsoft.Net.Compilers and use csc.exe from the command line.

Advantages

  • It is a very nice way, BDN creates a script which you can run manually. Everything works like in a case of the old MSBuild toolchain.

Disadvantages

  • It is hard to detect location of csc.exe (tools folder of the Microsoft.Net.Compilers package)
  • It affects compilation of the host project (it will be built using the given version of the C# compiler)

Approach 2

Bundle csc.exe and its dependencies as resources files in BDN. (Currently implemented in the Roslyn branch.)

Advantages

  • It solves problems of the approach 1 and allows to create csc.exe in the right folder.

Disadvantages

  • It looks ugly and increase size of BenchmarkDotNet.dll (about 14MB for each moniker).
  • There are still some troubles with detecting a folder with framework libraries (like C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.6\). It looks solvable for Win+MS.NET, but it's hard to implement for Linux/MacOS+Mono (especially, if you are using custom mono instead of default) .
  • Possibly there are some license issues because Roslyn uses Apache 2 and we are using MIT.

Approach 3

Add a NuGet reference to Microsoft.CodeAnalysis.CSharp and compile Program.cs via API (like in minibench).

Advantages

  • It looks nice and solves all of our problems.
  • It allows to easily use framework references of the host process without additional magic for any runtime (MS.NET/Mono include custom Mono) because now we can write something like this:
var compilation = CSharpCompilation
    .Create("Program.dll")
    .AddReferences(
        MetadataReference.CreateFromFile(typeof(object).GetTypeInfo().Assembly.Location))
    .AddSyntaxTrees(tree);

Disadvantages

  • We will have an additional NuGet dependency (but I don't think that it is a problem).
  • We need to upgrade BDN to .NET 4.5. However, I don't think that it is a problem anymore:
    • We already drop .NET 3.5 support
    • We are going to drop Framework settings for Jobs (see also Framework settings in Jobs #194)
    • If someone has a .NET 4.0 project, it is possible to create a separated .NET4.5 project for benchmarks.
    • BTW, latest version of mono doesn't support .NET 4.0.

Summary

I spent a lot of time with the approaches 1 and 2. Unfortunately, I failed, it is too hard to solve described problems and possible solutions include a lot of hacks, ugly code, and magic strings. For now, only the approach 3 makes sense to me: we need to upgrade BDN to .NET4.5 and add a reference to the Microsoft.CodeAnalysis.CSharp package.

@mattwarren, @adamsitnik, what do you think?

P.S. @adamsitnik, am I right, that with .NET4.5 we can drop portability tools and use the same Reflection API on the Full Framework and CoreCLR?

@niemyjski
Copy link

#3 is the best

@adamsitnik
Copy link
Member

@AndreyAkinshin I totally agree with you, #3 is the best for me, I can implement it right away Is that ok if I still create script for compilation but with call to csc.exe (no abasolute path)?

am I right, that with .NET4.5 we can drop portability tools and use the same Reflection API on the Full Framework and CoreCLR?

I am not sure, will verify that.

@AndreyAkinshin
Copy link
Member Author

I can implement

Ok, great.

@adamsitnik
Copy link
Member

@AndreyAkinshin Ok, it's done. And it is much more faster now (for my PC running all classic tests went from 250s to 70s)! I have also removed the portability api for reflection.

It should also work with Mono, because I use same predefined assemblies as the host process. But you need to verify that ;/

@AndreyAkinshin
Copy link
Member Author

Awesome! Can you also investigate, why do we have a failed build?
https://ci.appveyor.com/project/PerfDotNet/benchmarkdotnet/build/0.9.7.37

@adamsitnik adamsitnik changed the title [Suggestion] migrate to Microsoft.Net.Compilers Migrate from MSBuild to Roslyn Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants