stryker-mutator / stryker-net

Mutation testing for .NET core and .NET framework!
https://stryker-mutator.io
Apache License 2.0
1.75k stars 176 forks source link

Passing build variables to MSBuild.exe #466

Open oicguy opened 5 years ago

oicguy commented 5 years ago

I was referred to stryker by a co-worker who is really excited about mutation testing in general. After reading the documentation I decided to get it a try. My project is using the .Net Framework, not .Net Core so I expected some hiccups.

After installing the global tool and specifing the sln file, msbuild finally ran but failed. The reason is because a targets file isn't in the expected location. How can this be since I can run it in Visual Studio without a problem? Turns out, Visual Studio modifies some of the build variables so keep certain consistencies.

In my case, I've installed the DacFx nuget package and it adds a Microsoft.Data.Tools.Schema.SqlTasks.targets files that allows me to build a SQL Server Data Tools project. This file is placed by the nuget install in the C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\MSBuild\Microsoft\VisualStudio\v15.0\SSDT because the MSBuildExtensionsPath was modified to be C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\MSBuild. Normally the path is C:\Program Files (x86)\MSBuild.

If there were a way to set the property values from either the command line or the configuration file the issue could be resolved with very little change to the stryker code itself. While I'm approaching this from a .Net Framework perspective, I've used the dontnet cli as well and am aware that it can use the MSBuild properties as well. It seems this would be a powerful addition that would allow the developers to harness more of the build process without having to build in specific support for something.

Notes

This feature should probably be implemented as a config file only option.

rouke-broersma commented 5 years ago

Could you give an example of the arguments you need passed to msbuild to be able to use stryker?

oicguy commented 5 years ago

If I were using MSBuild directly, I'd need to use msbuild.exe /p:MSBuildExtensionPath="C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\MSBuild". Obviously that specific for my computer since not everyone would use the C: drive or even the Program Files directory. Allowing us to pass in variable like this would allow use to maybe use different configurations /p Configuration=Debug or even different Preprocessor directive /p:DefinedConstants=Trace;Debug;Analysis.

If I may make a suggestion, I'd like to see in the configuration file an msbuild section where you can define variables and other msbuild sections like the following: msbuild:{ properties: { MSBuildExtensionsPath: "..." }, switches:["m"] } With this layout, you'll only have to format the output and this setup would become /p:MSBuildExtensionsPath="..." /m

rouke-broersma commented 5 years ago

I am hesitant to add such configuration as our compilation mechanism is already more complex than we would like, but we i'll talk to my coworkers about this and see what we can do to support this.

oicguy commented 5 years ago

That's all I can really ask for. Thanks for at least considering it.

richardwerkman commented 5 years ago

It would have my preference to solve this without introducing new stryker config options. Could we read the msbuild properties from your project file? We have buildalyzer analysing the whole project before we execute msbuild. Buildalyzer gives us all msbuild properties found in the project file and .target files. If your MSBuildExtensionPath is somewhere in there we can pass it along to msbuild :+1:

richardwerkman commented 5 years ago

@oicguy You could try to set the MSBuildExtensionsPath variable explicitly for now by placing it in your csproj file or as environment variable.

See: https://stackoverflow.com/questions/2876824/overriding-msbuildextensionspath-in-the-msbuild-task-is-flaky

If that works that is a workable workaround for now. We'll look for a better solution in the future.

oicguy commented 5 years ago

I'm not sure if you read the solution for that issue link but it says that flaky behavior is a bug in MsBuild 3.5 and the post itself is over 8 years old. I would hardly call that justification for not adding functionality. But I'm not trying to fight your decision.

Placing the variable in the csproj file doesn't feel like a good workaround because that property was designed to be different depending on the compilation environment. It seems that it will be different based on if it's compiled in Visual Studio or not and event different in different versions of Visual Studio. Hard coding it in the csproj would take away the flexibility the system was designed to have.

Again, I appreciate you guys looking into this matter and if there's anything I can do to help feel free to ask.

richardwerkman commented 5 years ago

Sorry the link was mainly meant to show how to override the variable, not the flaky msbuild behaviour.

I didn't understand it was so important that the variable stays flexible. Now I see why you'd want to be able to pass the variable at runtime. However I'm not so fond of passing individual msbuild parameters. Maybe we could create an option to pass extra parameters as a full string. It could look like:

dotnet stryker --msbuild-params "/p:MSBuildExtensionPath='C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\MSBuild' /p:Configuration=Debug"

And we would always place it behind the solution path here: https://github.com/stryker-mutator/stryker-net/blob/4530ff5c251cf7b139eb4ff0c58fd60065a9bb22/src/Stryker.Core/Stryker.Core/Initialisation/InitialBuildProcess.cs#L38

If the option is optional an empty string would be added by default so there are no extra parameters passed to msbuild. If the option has a value it would be passed to msbuild like:

msbuild.exe ../mysolutionfile.sln /p:MSBuildExtensionPath='C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\MSBuild' /p:Configuration=Debug

I think this would be a future proof solution without much effort 👍

Of course we should handle errors better for when wrong parameters are passed through stryker to msbuild.

oicguy commented 5 years ago

I think this solution is fine and I can't wait to test it against my application. Thanks!

molekp commented 2 years ago

Hi, I have similar problem with using stryker on my .net framework application. I've notice that this feature is still in To do. Are there any chances to implement this?

rouke-broersma commented 2 years ago

Sure, are you willing to pick up the work @pawelhevy? If so I'll happily review and merge it.

molekp commented 2 years ago

@rouke-broersma I thought a lot of this feature but finnally (I think) I have better solution. Can we add a flag like --no-build (simillar as in dotnet test)?

Why it is better? I have a CI pipeline, wchich builds my code. So, I don't want to build it again when I run stryker as next step of pipeline. I would like to use previously compiled code. Is it possible?

richardwerkman commented 2 years ago

@pawelhevy I have thought about that before too. But I saw it as a performance enhancement. We currently always build the project to ensure all builded files are in place and to make sure the project is buildable so we won't end in an endless loop once we start placing mutations.

But I thought just like you: if one is certain the project is buildable and all files are in place we can skip the build in stryker, that will save some runtime for stryker. But if the project turns out to be not buildable we will encounter some strange errors during mutating for sure. So we should print a warning when using this feature.

Now that we have a second reason to implement this I think it would be a nice addition to stryker. So to answer your question: yes it should be possible. Could you work on a PR for this? If you need any help please let us know.

molekp commented 2 years ago

Nice to hear this. I would try to implement this. I will let you know when it works. Thanks!

molekp commented 2 years ago

I've notice that method .Build() from Buildalyzer is called only to receive IAnalyzerResult. Nextly tests projects are builded again (in InitialBuildProcess.InitialBuild) using received informations (like TargetFramework).

If I'm right, I think it would by nice if Buildalyzer have option to receive IAnalyzerResult without expensive .Build() method. This is mentioned in this Buldylizer issue. I think it would speed up stryker.

Also, now when I have added option --no-build it would not work, cuz this .Build() method will clear my previously builded test project by his own (new) build, which ends as GeneralStrykerException

Please let me know. Thanks.

rouke-broersma commented 2 years ago

We need information from buildalyzer that for sure can only be found using a Build command so that cannot be skipped.

Buildalyzer actually does not perform a compete build so it is actually very fast already.

What we might be able to skip is the clean target that Buildalyzer calls, and we can also skip the initial build performed after Buildalyzer.

molekp commented 2 years ago

I've tried to solve this by prevent clean target in Buildalyzer. It worked, but finally ended as CompilationException.

I found that someone has similarly issue with stryker & Buildalyzer.

I will try another approach that is mentioned there (set different output paths). Wish me luck ;)

bcollamore commented 2 years ago

Is Buildalyzer really needed? Would it be too drastic to instead use Roslyn APIs directly?

rouke-broersma commented 2 years ago

@bcollamore what do you mean by using roslyn api's directly? Roslyn is the compiler, it does not know anything about the project structure (msbuild). Msbuild (buildalyzer is a wrapper for msbuild) is used to gather the project data and pass this along to Roslyn. To be able to do this you first have to know the project data. We are already using Roslyn directly, with the project data we gather using Buildalyzer. Or am I missing something significant here that Roslyn can do for us?

bcollamore commented 2 years ago

@rouke-broersma I confess it has been ~18 months since I looked at this specific issue, so I do not recall if we would need the Roslyn APIs (e.g., https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.projectinfo?view=roslyn-dotnet-4.0.1) or the MsBuild APIs (e.g., https://github.com/dotnet/msbuild/tree/main/src/Build/Construction). As an analogy, my team has tools (unfortunately not open-sourced yet, so I can't show you specific code examples of what I have in mind) that inspect project settings, such as (1) Does the project have required References (e.g., for Analyzers)? or (2) Does the project use the highest Compiler Warning Level? or (3) Does the project Treat Warnings As Errors? The tool doesn't parse the .csproj file directly (because it is too complex wrt overridden settings, .props files, .targets files, etc.) but rather queries the MS APIs to get to the meta information. (Presumably Buildalyzer is using the same APIs internally.) My open-ended question is, if Buildalyzer is going to decimate the output folders, is the overhead of Buildalyzer really necessary/desired? (Maybe it is, idk. Maybe the levels of abstraction it provides is a good thing. Or, maybe it's an unnecessary layer.)

rouke-broersma commented 2 years ago

@bcollamore Buildalyzer does not use MSBuild API's per se. What it does is call MSBuild.exe with a specific set of instructions to register eventhandlers for specific events msbuild outputs. From these events buildalyzer can build up the complete project structure as msbuild reads it as far as I understand.

This is necessary because during building the required msbuild targets are called in to find all project level information such as required assemblies. I would love an example of how we can do this without buildalyzer, but to me it sounds like we would just be remaking buildalyzer and/or msbuild at that point, and would be hitting the same issues buildalyzer is hitting when clean is removed.

molekp commented 2 years ago

Bad news. Changing Buildalyzer output paths will provide some problems in Stryker. Especially these (who knows more): image

which will fail:

  1. as System.IO.DirectoryNotFoundException in CsharpProjectComponentsBuilder, cuz I've removed temp folder for Buildalyzer after getting his results
  2. as GeneralStrykerException

I think this is bad way to do it, but currently I've no other idea how to achieve this.

rouke-broersma commented 2 years ago

@bcollamore came to that same conclusion in the linked buildalyzer issue.