Fixing failing Reqnroll build by using TaskHostFactory

Tiberriver256 commented 4 months ago

This seems to fix the problem described in this discussion: https://github.com/orgs/reqnroll/discussions/38

It should prevent developers from setting the MSBUILDDISABLENODEREUSE environment variable and speed up builds slightly as the msbuild nodes will be re-used.

More details on the fix here: https://learn.microsoft.com/en-us/visualstudio/msbuild/how-to-configure-targets-and-tasks?view=vs-2022#task-factories

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue).
[ ] New feature (non-breaking change which adds functionality).
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected).
[ ] Performance improvement
[ ] Refactoring (so no functional change)
[x] Other (docs, build config, etc)

ajeckmans commented 4 months ago

Can you also check by removing https://github.com/reqnroll/Reqnroll/blob/main/Directory.Build.rsp This is still setting the node reuse to false for any invocation of msbuild.

Tiberriver256 commented 4 months ago

@ajeckmans - I tried it out, and it now seems to work fine without that file. Added the deletion of that file to this PR.

Tiberriver256 commented 4 months ago

It would be good for someone else to test this out and confirm the fix locally.

gasparnagy commented 4 months ago

@Tiberriver256 I will also try. Do you know why it causes the build failure? I seems to cause the test projects failing to build.

ajeckmans commented 4 months ago

@Tiberriver256 I will also try. Do you know why it causes the build failure? I seems to cause the test projects failing to build.

He included a link in the description of the PR that states why it happens.

Tiberriver256 commented 4 months ago

Yeah... I'm trying to figure out why adding it breaks the tests and if there's another workaround for that. We'll have to hold off on merging this until we figure that out.

For now... it solved one problem and caused another, haha.

Tiberriver256 commented 4 months ago

@gasparnagy @ajeckmans

The new issue appears to have been a bug with MSBuild that was fixed in .NET 8.

I bumped the SDK version used to run the specs-xunit and everything passed. Do you guys think this is a problem? or should I update the unit and mstest to .NET 8 SDK as well?

ajeckmans commented 4 months ago

I'd just update all of the sdks. As long as you still keep targeting net6.0 I don't see an issue.

gasparnagy commented 4 months ago

I agree. Feel free to target net 8 sdk for all.

Tiberriver256 commented 4 months ago

Alrighty 👍 everything seems to be happy. Just did rebase to tidy up the commits but should be ready for a final review.

gasparnagy commented 4 months ago

@Tiberriver256 great job. I will review it in detail on Monday. After a quick look, I am a bit scared that we had to modify the common msbuild package, although this locking problem is only specific to the Reqnroll solution. The question is whether the net6 bug would cause issues with others as well. Maybe our own solution should use a special version of that package and keep the common one untouched.

gasparnagy commented 4 months ago

@Tiberriver256 Based on the linked MsBuild issue, I wonder whether we really need to ship the MsBuild assemblies in our build task (ie whether we really need to include all these assemblies...) Currently we include the following system/msbuild assemblies:

Microsoft.Build.dll
Microsoft.Build.Framework.dll
Microsoft.Build.Tasks.Core.dll
Microsoft.Build.Utilities.Core.dll
Microsoft.Extensions.DependencyModel.dll
Microsoft.VisualStudio.Composition.dll
Microsoft.VisualStudio.Composition.NetFxAttributes.dll
Microsoft.VisualStudio.Threading.dll
Microsoft.VisualStudio.Validation.dll
System.CodeDom.dll
System.ComponentModel.Composition.dll
System.Composition.AttributedModel.dll
System.Composition.Convention.dll
System.Composition.Hosting.dll
System.Composition.Runtime.dll
System.Composition.TypedParts.dll
System.Configuration.ConfigurationManager.dll
System.Security.Cryptography.ProtectedData.dll
System.Security.Permissions.dll

As we have a good repro now, could you please revert the build fix temporarily and change the line 29 in Reqnroll.Tools.MsBuild.Generation.nuspec from

<file src="bin\$config$\net6.0\*.dll" target="tasks\$Reqnroll_Net6_TFM$" />

to

<file src="bin\$config$\net6.0\BoDi.dll" target="tasks\$Reqnroll_Net6_TFM$" />
<file src="bin\$config$\net6.0\CucumberExpressions.dll" target="tasks\$Reqnroll_Net6_TFM$" />
<file src="bin\$config$\net6.0\Gherkin.dll" target="tasks\$Reqnroll_Net6_TFM$" />
<file src="bin\$config$\net6.0\SpecFlow.Internal.Json.dll" target="tasks\$Reqnroll_Net6_TFM$" />
<file src="bin\$config$\net6.0\Reqnroll*.dll" target="tasks\$Reqnroll_Net6_TFM$" />

(probably there is a smarter way and we would need to do it for the other frameworks as well, but as a quick test it is OK)

Tiberriver256 commented 4 months ago

Good thought @gasparnagy.

Oddly, changing the net6 only caused the net6 tests to produce the following error:

/home/runner/.nuget/packages/reqnroll.xunit/1.0.2-ci20240302-142/build/Reqnroll.xUnit.targets(35,5): error MSB4062: The "Reqnroll.Tools.MsBuild.Generation.ReplaceTokenInFileTask" task could not be loaded from the assembly /home/runner/.nuget/packages/reqnroll.tools.msbuild.generation/1.0.2-ci20240302-142/build/../tasks/netcoreapp3.1/Reqnroll.Tools.MsBuild.Generation.dll. The assembly 'Microsoft.Build.Utilities.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' has already loaded been loaded into this MetadataLoadContext. Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [/tmp/RR/S3da5f87b/DefaultTestProject/DefaultTestProject.csproj]

This seemed to indicate that I should also change the rules for netcoreapp3.1 so I also made the same change there and now the build appears to be passing.

gasparnagy commented 4 months ago

Good. I will do some extra checks on Monday, but I think this will be good to go.

gasparnagy commented 4 months ago

@Tiberriver256 I did some intensive testing with all positive results:

I haven't got any locking issue! I was even able to call a "Rebuild" in VS that has never worked yet with Reqnroll so far. Wonderful!
I tested the NuGet packages with different .NET frameworks (.net 6,8,462,472,481) and it worked for all, even with .NET 6 SDK forced.

I have found that the package .dll configuration we could do in an easier/safer ways, so instead of including the .dll-s, we can do exclusions:

<file src="bin\$config$\net6.0\*.dll" exclude="bin\$config$\net6.0\System.*;bin\$config$\net6.0\Microsoft.*" target="tasks\$Reqnroll_Net6_TFM$" />

and

<file src="bin\$config$\netcoreapp3.1\*.dll" exclude="bin\$config$\netcoreapp3.1\System.*;bin\$config$\netcoreapp3.1\Microsoft.*" target="tasks\$Reqnroll_Core_Tools_TFM$" />

that produces the same package, so I suggest changing it to that.

Tiberriver256 commented 4 months ago

Looks like the exclusions worked @gasparnagy! Would you like for me to squash this into one commit before merging?

gasparnagy commented 4 months ago

@Tiberriver256 yes, please squash & merge

reqnroll / Reqnroll

Fixing failing Reqnroll build by using TaskHostFactory #64

Types of changes