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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

obligaron
Copy link
Contributor

🤔 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?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
  • 🐛 Bug fix (non-breaking change which fixes a defect)
  • 💥 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.

@gasparnagy
Copy link
Contributor

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

@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)

@Code-Grump
Copy link
Contributor

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.

@gasparnagy
Copy link
Contributor

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.

# Conflicts:
#	Plugins/Reqnroll.Verify/Reqnroll.Verify.ReqnrollPlugin.IntegrationTest/Reqnroll.Verify.ReqnrollPlugin.IntegrationTest.csproj
@gasparnagy gasparnagy added the parked We decided to delay dealing with this label Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parked We decided to delay dealing with this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants