Simplified MSBuild Package

[Code-Grump]

🤔 What's changed?

• Modified the MSBuild generation packing process to use SDK-style packing and minimal dependency payload.
• Removed deps.json from package
• Switch packages from using file-based license to SPDX identifier.

⚡️ What's your motivation?

Resolves issues with loading assemblies which aren't included within the MSBuild process. Should unblock #803

🏷️ What kind of change is this?

• 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

---

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

[304NotModified]

@Code-Grump could you please change this pr to draft until the build and tests are green? Currently I get an notification after every push 😅

[Code-Grump]

Damn, sorry. I really need the pipelines to run, but I don't want to drown you in notifications.

[gasparnagy]

Damn, sorry. I really need the pipelines to run, but I don't want to drown you in notifications.

The pipelines are also run for draft PRs as well... So this should work either way,.

[Code-Grump]

I don't know where I got the idea that draft PRs didn't run the checks. Glad not to generate more noise.

[Code-Grump]

I'm having trouble understanding the test-failures for this branch. It's all related to Reqnroll.Formatters, but I've not knowingly touched the project or its associated test suite and the test failures are non-obvious to how they could be related to packing changes. I'd appreciate any insight that could be offered.

[clrudolphi]

I'm having trouble understanding the test-failures for this branch. It's all related to Reqnroll.Formatters, but I've not knowingly touched the project or its associated test suite and the test failures are non-obvious to how they could be related to packing changes. I'd appreciate any insight that could be offered.

The generated assemblies do not contain the embedded ndjson file (per feature) that the code generation process creates. There is a run-time check and if the number of embedded ndjson records does not match the number created at code gen time, then the feature is considered as 'disabled' for the purposes of Messages. (ie, silently fails; perhaps we should improve that).

These embedded ndjson files were recently added as embedded items (in 3.0.1) in the last <ItemGroup> of this target:

  <Target
    Name="IncludeCodeBehindFilesInProject"
    DependsOnTargets="UpdateFeatureFilesInProject"
    BeforeTargets="CoreCompile">
    <ItemGroup Condition="'$(UsingMicrosoftNETSdk)' != 'true'">
      <Compile Include="@(ReqnrollGeneratedFiles)" Exclude="@(Compile)" />
    </ItemGroup>

    <ItemGroup Condition="'$(ReqnrollEmbedFeatureFiles)' == 'true'">
      <EmbeddedResource Include="@(ReqnrollFeatureFiles)">
        <LogicalName>%(ReqnrollFeatureFiles.NormalizedLogicalName)</LogicalName>
      </EmbeddedResource>
    </ItemGroup>
    
    <!-- TODO: Remove this Message after Messages Development -->
    <Message Text="ReqnrollGeneratedNdjsonFiles: @(ReqnrollGeneratedFiles->'%(EmbeddedMessagesStoragePath)')" Importance="high" />

    <ItemGroup>
      <EmbeddedResource Include="@(ReqnrollGeneratedFiles->'%(EmbeddedMessagesStoragePath)')" Condition="'%(ReqnrollGeneratedFiles.EmbeddedMessagesStoragePath)' != ''">
        <LogicalName>%(ReqnrollGeneratedFiles.EmbeddedMessagesResourceName)</LogicalName>
      </EmbeddedResource>
    </ItemGroup>
  </Target>

Were changes made to the MsBuild.Generation c# code in this PR or this only changes to the targets and props? I'm not quite sure where to start looking...

[clrudolphi]

I created a sample project using this build. Interesting results.
The build output shows this:

Rebuild started at 9:45 AM...
1>------ Rebuild All started: Project: RNR_812, Configuration: Debug Any CPU ------
Restored C:\Users\clrud\source\repos\scratch\RNR_812\RNR_812\RNR_812.csproj (in 1.43 sec).
1>ReqnrollGeneratedNdjsonFiles: obj\Debug\net8.0\Features\Calculator.feature.ndjson
1>RNR_812 -> C:\Users\clrud\source\repos\scratch\RNR_812\RNR_812\bin\Debug\net8.0\RNR_812.dll
========== Rebuild All: 1 succeeded, 0 failed, 0 skipped ==========
========== Rebuild completed at 9:45 AM and took 06.951 seconds ==========

The 4th line is created by the <Message> in the <Target> shown above that has the TODO on it (it was subsequently removed in 3.0.2).
So that provides evidence that the target is getting invoked and that there are ndjson results to be embedded in the assembly.
The generated ndjson file is present in the obj\Debug\Features folder (from which the EmbeddedResource task is meant to read the content of the file).
But the the file is not embedded in the assembly.
When I add <ReqnrollEmbedFeatureFiles>true</ReqnrollEmbedFeatureFiles> to the project's .csproj file, the feature file is not embedded in the assembly.
(Both of those embedments work correctly in r3.0.1)

So, I'm at a bit of a loss as to why the ndjson file is not present as an embedded resource in the assembly.

[Code-Grump]

Embedding may have been affected, but why do these tests fail citing an incorrect number of lines in the ndjson files, rather than indicating the embedded resources are missing? I definitely don't understand the failure mode and would like to.

[clrudolphi]

why do these tests fail citing an incorrect number of lines in the ndjson files

There are three phases of activity here: Code Generation time, test execution runtime, and post-run Acceptance test validation of the results of the run.

In this failure, the code generation fails to embed the set of static messages for a given Feature.

At test execution time, the runtime attempts to pull them from the embedded resource; this does not succeed, so the runtime proceeds but disables Formatters for that Feature. This is a silent failure (probably not a good handling of that failure scenario, but at best we could emit a warning as we don't want Messages internal plumbing failures to kill execution of the test). The formatters runtime proceeds and it does generate a set of messages that are based upon the available Bindings (ie, StepDefinition messages)

Specific to Formatters Tests, the third phase are acceptance tests. They pull in a golden copy of expected ndjson entries from an embedded resource, deserialize them to Message Envelopes and then run a set of Fluent Assertion tests comparing these expected messages against the actuals (which have been read from the test's ndjson and html files).

Thus, when the Formatters test runs, it finds a few messages, but not as many as were expected.

[Code-Grump]

This explanation lines up with what I see. A very strange failure-mode indeed.

Am I right thinking this should be impossible for most users to encounter unless they're mucking about with the build chain?

[clrudolphi]

Oh yeah, this is not something that would happen under usual circumstances.

[gasparnagy]

@Code-Grump Maybe you have mentioned, but I'm a bit out of sync: this one is NOT a breaking change, right? So we could add this to a simple 3.1 or even 3.0.3?

[Code-Grump]

There's some material changes to our MSbuild targets; I'm not clear on whether we maintain that as a public API.

[gasparnagy]

There's some material changes to our MSbuild targets; I'm not clear on whether we maintain that as a public API.

I don't think they are part of the public API but I will consider this when doing the detailed review.

[Code-Grump]

The targets didn't change much, but when they execute has shifted slightly. Very few customisations would be affected provided they use our targets in a standard fashion.

[gasparnagy]

This is cool. I have also made a few exploratory tests with it (also on .NET Fw projects) and it works fine.
I have made a few smaller comments.

[gasparnagy]

We will need to merge the main branch into this and make sure that we also apply the change to the xunit3 project that has been just added.

[304NotModified]

Oops, My PR (#844) will give some (small) conflicts on this PR I guess

This warning is fixed and is still here:

[Code-Grump]

I had noticed all the missing Readme warnings via this PR; it was going to be a follow-up to add them.

Once this PR is complete, you'll just need to reference the Readme files from the csproj.

[304NotModified]

Let's merge first this one and then I will update #844 :)

[Code-Grump]

@gasparnagy xUnit3 project updated.

Once this PR is done, I might take a look at swapping over the plugin packages to use csproj-based packing too.

[gasparnagy]

I did some intensive exploratory testing and haven't found any problems, so this is good to go. (I have added a line to the CHANGELOG though, because if it turns out that we broke something, it should be easier to track.)

[gasparnagy]

Released in v3.1.0 today (https://github.com/reqnroll/Reqnroll/releases/tag/v3.1.0)