Reqnroll icon indicating copy to clipboard operation
Reqnroll copied to clipboard

Compile Reqnroll.Tools.MsBuild.Generation only with netstandard2.0

Open obligaron opened this issue 1 year ago • 4 comments

🤔 What's changed?

Change Reqnroll.Tools.MsBuild.Generation to only compile to netstandard2.0 and droped compiling to .net462.

⚡️ What's your motivation?

In #373 we got compilation errors because our netstandard2.0 projects reference assemblies with other versions then the one compiled .net462. This also simplifies the logic.

🏷️ What kind of change is this?

  • :bank: Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
  • :bug: Bug fix (non-breaking change which fixes a defect)
  • :boom: Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • [ ] Users should know about my change
    • [ ] I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

obligaron avatar Jan 09 '25 17:01 obligaron

This would simplify the things a lot and maybe this is the way to go. The only thing that comes to my mind is when someone uses a generator that needs to be cross compiled (because a dep does not support .NET Standard). But maybe that works too. We need to do intensive manual testing for this.

gasparnagy avatar Jan 10 '25 09:01 gasparnagy

@obligaron I have played with this solution and many things work fine, including the nasty VS 17.8.2 build, but...

I have a plugin that I use for a work project that shows a special issue (the plugin loading is added to our external plugin test suite to get a compatibility indication, this is why the build fails now for this PR).

So the plugin is a simple generator plugin (has a class (IterateThroughExamplesWrapperTestClassDecorator) that implements Reqnroll ITestClassDecorator and a ITestMethodDecorator interfaces). The plugin follows the previous standards it is cross-compiled to netstandard2.0 and net472. Following the usual pattern, the appropriate one is used from the package depending on the execution platform:

  <PropertyGroup>
    <_SpecSyncReqnroll_GeneratorPlugin Condition=" '$(MSBuildRuntimeType)' == 'Core'">netstandard2.0</_SpecSyncReqnroll_GeneratorPlugin>
    <_SpecSyncReqnroll_GeneratorPlugin Condition=" '$(MSBuildRuntimeType)' != 'Core'">net472</_SpecSyncReqnroll_GeneratorPlugin>
    <_SpecSyncReqnroll_GeneratorPluginPath>$(MSBuildThisFileDirectory)$(_SpecSyncReqnroll_GeneratorPlugin)\SpecSync.AzureDevOps.TestSuiteBasedExecution.ReqnrollPlugin.dll</_SpecSyncReqnroll_GeneratorPluginPath>
  </PropertyGroup>

With this PR, once the compilation is done through MsBuild (or done from Visual Studio), the .NET Framework 4.* will be used to perform the generation. With the PR, this uses out .NET Standard build, but we load the net472 compilation of the plugin from it.

I don't really understand why it is a problem, because essentially the execution is on .NET Framework 4, but somehow if this happens it fails with strange errors, like it could not find the method implementing the interface method.

[Reqnroll] System.TypeLoadException: Method 'CanDecorateFrom' in type 'SpecSync.AzureDevOps.TestSuiteBasedExecution.ReqnrollPlugin.Generator.IterateThroughExamplesWrapperTestClassDecorator' from assembly 'SpecSync.AzureDevOps.TestSuiteBasedExecution.ReqnrollPlugin, Version=3.4.23.0, Culture=neutral, PublicKeyToken=null' does not have an implementation. 

My plugin has no .NET 4 specific stuff and when I switch it to simple compilation to .NET Standard only, it would work again. I could make this fix, but this shows a more generic issue.

Based on that my understanding is the following:

  1. This PR is essentially a breaking change for generator plugins that are cross-compiled.
  2. If a plugin uses a dependency that does not have a .NET Standard compilation, then this plugin is forced to use cross-compilation, therefore the problem cannot be solved.

So basically the question is whether we should move on with the PR and say that only .NET Standard compatible generator plugins should be allowed or drop the PR.

What do you think? (CC @Code-Grump @ajeckmans)

gasparnagy avatar Feb 05 '25 12:02 gasparnagy

What's the motivation for targeting the .NET 4 runtime specifically in the first place? As you stated already: anything compiled for .NET Standard would run there.

This specific error smells like binary incompatibility, like an interface changed in one assembly and the version being shipped alongside your assembly doesn't match. We'd need to grab all the binaries as they sit on the server (very hard based on context clues) to know what was actually being loaded, or otherwise dump out all the module information.

Code-Grump avatar Feb 05 '25 13:02 Code-Grump

What's the motivation for targeting the .NET 4 runtime specifically in the first place? As you stated already: anything compiled for .NET Standard would run there.

This might be the result of a dependency chain issue. I can imagine two scenarios:

  1. Your plugin does something with CSV file and you would like to use a library X to parse it. The library X is compiled to .NET 4 and .NET 6 (but not to .net standard). In this case you cannot compile your plugin to a single target.

  2. You use your plugin internally for your legacy .NET Fw solution. Your plugin is specific to .NET Fw so you only compile it to net471. Since you use it only for the legacy solution, this would not be a problem.

This specific error smells like binary incompatibility, like an interface changed in one assembly and the version being shipped alongside your assembly doesn't match.

The concrete example is not that interesting, because that could be simply recompiled to .NET Standard. But just to answer the question: it is exactly the same simple code that is compiled to .NET Fw and .NET Standard and the interface that is in the error is one of the Reqnroll interfaces that is sitting in a .NET Standard dll - so there is no obvious reason for it.

gasparnagy avatar Feb 05 '25 13:02 gasparnagy

Closing this to clean up the repo. We can reopen if we want to consider it again.

gasparnagy avatar Aug 08 '25 19:08 gasparnagy