Use reflection to implement SettingsEntity.NewInstance #1281

borland · 2023-10-24T11:11:37Z

The current implementation of SettingsEntity.NewInstance uses a roundtrip through BinaryFormatter as a way to clone objects.

When running Nuke on .NET 8 RC2, I observe this warning in the log when using an [OctoVersionAttribute]

[WRN] Could not inject value for Build.OctoVersionInfo
System.NotSupportedException: BinaryFormatter serialization and deserialization are disabled within this application. See https://aka.ms/binaryformatter for more information.
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize(Stream serializationStream, Object graph)
   at Nuke.Common.Tooling.SettingsEntityExtensions.NewInstance[T](T settingsEntity) in /_/source/Nuke.Tooling/SettingsEntity.NewInstance.cs:line 23
   at Nuke.Common.Tools.OctoVersion.OctoVersionGetVersionSettingsExtensions.SetFramework[T](T toolSettings, String framework) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersion.Generated.cs:line 849
   at Nuke.Common.Tools.OctoVersion.OctoVersionAttribute.<>c__DisplayClass56_0.<GetValue>b__0(OctoVersionGetVersionSettings _) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersionAttribute.cs:line 141
   at Nuke.Common.Tools.OctoVersion.OctoVersionTasks.OctoVersionGetVersion(Configure`1 configurator) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersion.Generated.cs:line 98
   at Nuke.Common.Tools.OctoVersion.OctoVersionAttribute.GetValue(MemberInfo member, Object instance) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersionAttribute.cs:line 141
   at Nuke.Common.ValueInjection.ValueInjectionAttributeBase.TryGetValue(MemberInfo member, Object instance) in /_/source/Nuke.Build/Execution/Extensibility/ValueInjectionAttributeBase.cs:line 26
Target 'BuildOctopusServer' requires member 'OctoVersionInfo' to be not null

More discussion and workaround in Issue #1282

Fix

This pull request removes BinaryFormatter from Nuke, and uses reflection to clone ISettingsEntity objects instead.

Quality

I've added some unit tests around the NewInstance method, covering various things that might be contained in an ISettingsEntity object and asserting they are cloned correctly. I would be more than happy to add additional tests if there are additional data types or object shapes that might be of concern.

I confirm that the pull-request:

Follows the contribution guidelines
Is based on my own work
Is in compliance with my employer

…naryFormatter

borland · 2023-10-25T08:00:12Z

Fixes #1282

matkoch · 2023-10-31T21:39:04Z

@borland sorry I haven't gotten to review any of this yet. There is already another issue/pull-request tackling the binary serialization problem (#1247). Are there any reasons why reflection could be preferred?

borland · 2023-11-10T20:20:29Z

@matkoch thanks for the response. I hadn’t noticed that other pull request.
Here’s my thoughts:

I think it’d be good if a released version of Nuke is available with a BinaryFormatter fix before .NET 8 itself releases on the 14th of November. This is more important than which fix you choose or whether you go a third path.
The reason I went with reflection in this PR is that looking at the code, the BinaryFormatter wasn’t actually used for serialisation. It felt like the wrong tool for the job
What the code was actually doing was cloning objects. Both BinaryFormatter and JSON will use reflection under the hood to get this job done so I thought why not just cut straight to that.
However, on the other side: while I feel like the reflection code is a slightly cleaner design, System.Text.JSON is much more battle-hardened than my simple code here. It may do things like cache the reflection it does internally for better performance
If you have future designs where you might actually want to serialise these things to save them to disk or send over a network, then maybe that serialisation approach will fit those better

Practically speaking: I had a quick look at #1247.

It adds pragma warning disables for these legacy serialization API's. This work is required for Nuke itself to compile under .NET 8, but not neccessarily for Nuke to be used on .NET 8. While this is valuable, IMHO it would be better separated into a different PR.
It doesn't have any tests. By merging it, you take the assumption that the Json serializer and BinaryFormatter have the exact same behaviour. Do they? My PR here adds quite a few tests to assert that it behaves as-expected

🤷

thanks

matkoch · 2023-11-19T23:25:59Z

@borland thanks for taking the time, but unfortunately, I'm gonna have to close this PR (and the other too). Both have failing tests (yours in Nuke.Common.Tests.SettingsTest.TestXunit2) and also build.cmd throws. There's even one more complicated thing that currently isn't covered by tests at all (LookupTable<,>).

Generally speaking, I've decided against fixing this for .NET 8 and will see to approach this another time for .NET 9. develop already has a warning suppression, I'm just waiting for feedback if the alpha build works.

If you want to test the alpha build, it's version 9999.0.0 on https://f.feedz.io/nuke/alpha/nuget.

borland · 2023-11-20T04:45:53Z

@matkoch that is dissapointing. It means that many projects which use Nuke will need to work around this and add EnableUnsafeBinaryFormatterSerialization to their csproj files when they upgrade to .NET 8. This goes against all the warnings/recommendations from Microsoft.

I'm not sure if this was clear earlier: This doesn't affect just Nuke itself (if it did, fair enough if you want to defer it), but the error propagates to all projects using Nuke. Additionally, the out-of-box experience for a newly created project on .NET 8 may not work unless the user stumbles into one of the GitHub issues here and discovers the workaround.

If the other failing tests and lookup table code are your main concern, would you reconsider accepting the PR if I fixed them? I'd be more than happy to do so, I simply wasn't aware of these earlier.

matkoch · 2023-11-20T05:31:47Z

@borland, I'm as disappointed as you, but, unfortunately, neither me nor the PRs were able to fix this in time. Right now, I'm knee-deep in my daily job, so I'm not very keen to test and review another attempt. What I would like to do, is to push a version that makes it working with .NET 8. And suppressing the warning is the easiest solution.

the error propagates to all projects using Nuke

I'm well aware of that, but maybe you've missed the commit I referred to. Have you tried the alpha version?

out-of-box experience for a newly created project on .NET 8 may not work unless the user stumbles into one of the GitHub issues here and discovers the workaround

Again, the commit should fix that. That's why I'm asking for feedback on the alpha version. Same as on Slack, but I barely got a single reaction, which is ... odd ... considering how much demand there was here.

There's one caveat to projects that only transitively reference Nuke.Common, but these errors can be fixed in the same way as it's done with Nuke.Common.props.

borland · 2023-11-20T05:49:20Z

Thanks for the further response @matkoch. I wasn’t in position to try the alpha build earlier, I’ll give it a try on our projects and let you know the outcome.

borland · 2023-11-20T08:40:03Z

Hi @matkoch

I updated my project to add https://f.feedz.io/nuke/alpha/nuget as a package source in my NuGet.config, then I changed the NuGet Package reference on Nuke.Common to 9999.0.0

After running a dotnet restore then running nuke, I still get the same error:

PS C:\Dev\MyProject> nuke
PowerShell Desktop version 5.1.22621.2428
Microsoft (R) .NET SDK version 8.0.100

███╗   ██╗██╗   ██╗██╗  ██╗███████╗
████╗  ██║██║   ██║██║ ██╔╝██╔════╝
██╔██╗ ██║██║   ██║█████╔╝ █████╗  
██║╚██╗██║██║   ██║██╔═██╗ ██╔══╝  
██║ ╚████║╚██████╔╝██║  ██╗███████╗
╚═╝  ╚═══╝ ╚═════╝ ╚═╝  ╚═╝╚══════╝

NUKE Execution Engine version 7.1.0-alpha.349 (Windows,.NETCoreApp,Version=v8.0)

21:33:33 [WRN] Could not inject value for Build.OctoVersionInfo
System.NotSupportedException: BinaryFormatter serialization and deserialization are disabled within this application. See https://aka.ms/binaryformatter for more information.
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize(Stream serializationStream, Object graph)
   at Nuke.Common.Tooling.SettingsEntityExtensions.NewInstance[T](T settingsEntity) in /_/source/Nuke.Tooling/SettingsEntity.NewInstance.cs:line 23
   at Nuke.Common.Tools.OctoVersion.OctoVersionGetVersionSettingsExtensions.SetFramework[T](T toolSettings, String framework) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersion.Generated.cs:line 849
   at Nuke.Common.Tools.OctoVersion.OctoVersionAttribute.<>c__DisplayClass56_0.<GetValue>b__0(OctoVersionGetVersionSettings _) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersionAttribute.cs:line 141
   at Nuke.Common.Tools.OctoVersion.OctoVersionTasks.OctoVersionGetVersion(Configure`1 configurator) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersion.Generated.cs:line 98
   at Nuke.Common.Tools.OctoVersion.OctoVersionAttribute.GetValue(MemberInfo member, Object instance) in /_/source/Nuke.Common/Tools/OctoVersion/OctoVersionAttribute.cs:line 141
   at Nuke.Common.ValueInjection.ValueInjectionAttributeBase.TryGetValue(MemberInfo member, Object instance) in /_/source/Nuke.Build/Execution/Extensibility/ValueInjectionAttributeBase.cs:line 26
Member 'OctoVersionInfo' is required to be not null

This makes sense - in your linked commit 63607c5 I can see you've added NoWarn for SYSLIB0050;SYSLIB0051 and EnableUnsafeBinaryFormatterSerialization, however these only apply to building Nuke itself. When I try build and run my own _build.csproj (created by Nuke), the EnableUnsafeBinaryFormatterSerialization does not propagate from one assembly to another, and the build fails unless I also add it to my _build.csproj locally.

For reference: My repository root has a global.json file specifying 8.0.100 of the NET SDK, and my nuke _build.csproj specifies <TargetFramework>net8.0</TargetFramework>

matkoch · 2023-11-20T08:54:02Z

@borland, clean your nuke.* packages. You've got a build from way earlier.

however these only apply to building Nuke itself

This is incorrect. The changes are defined in Nuke.Common.props, which is part of the NuGet package and imported automatically.

If this is still broken for you, then I need a proper repro.

borland · 2023-11-20T11:14:29Z

@matkoch thanks for that. I always forget that the nuke alphas always have the same version so require the cache-clean step.
I did this:

cleaned the cache
configured the nuke alpha
commented out <EnableUnsafeBinaryFormatterSerialization> from my _build.csproj
run nuke from the command line (which does build/restore/etc)

Everything worked, I recieved this output

PS C:\Dev\MyProject> nuke
PowerShell Desktop version 5.1.22621.2428
Microsoft (R) .NET Core SDK version 8.0.100

███╗   ██╗██╗   ██╗██╗  ██╗███████╗
████╗  ██║██║   ██║██║ ██╔╝██╔════╝
██╔██╗ ██║██║   ██║█████╔╝ █████╗  
██║╚██╗██║██║   ██║██╔═██╗ ██╔══╝  
██║ ╚████║╚██████╔╝██║  ██╗███████╗
╚═╝  ╚═══╝ ╚═════╝ ╚═╝  ╚═╝╚══════╝

NUKE Execution Engine version 7.1.0-alpha.373 (Windows,.NETCoreApp,Version=v8.0)

... snip ...

═══════════════════════════════════════════
Target                 Status      Duration
───────────────────────────────────────────
Clean                  Succeeded     < 1sec
Restore                Succeeded       0:01
Compile                Succeeded       0:04
Test                   Succeeded       0:10
───────────────────────────────────────────
Total                                  0:18
═══════════════════════════════════════════

Build succeeded on 21/11/2023 12:03:12 am. ＼（＾ᴗ＾）／

While I am slightly surprised to learn that Microsoft allows NuGet packages to declare themselves exempt from security warnings using a props file like this 🤦, I don't fault you for taking advantage of it to solve the problem 😄.

Thanks for taking the time to discuss!
I'll stick with the workaround of Enabling the unsafe formatter in my local csproj's for now and look forward to removing it when the next non-alpha build of Nuke ships with this change in it

Use reflection to implement SettingsEntity.NewInstance rather than Bi…

26954bd

…naryFormatter

matkoch closed this Nov 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use reflection to implement SettingsEntity.NewInstance #1281

Use reflection to implement SettingsEntity.NewInstance #1281

borland commented Oct 24, 2023 •

edited

Loading

borland commented Oct 25, 2023

matkoch commented Oct 31, 2023

borland commented Nov 10, 2023 •

edited

Loading

matkoch commented Nov 19, 2023 •

edited

Loading

borland commented Nov 20, 2023 •

edited

Loading

matkoch commented Nov 20, 2023

borland commented Nov 20, 2023

borland commented Nov 20, 2023 •

edited

Loading

matkoch commented Nov 20, 2023 •

edited

Loading

borland commented Nov 20, 2023 •

edited

Loading

Use reflection to implement SettingsEntity.NewInstance #1281

Use reflection to implement SettingsEntity.NewInstance #1281

Conversation

borland commented Oct 24, 2023 • edited Loading

borland commented Oct 25, 2023

matkoch commented Oct 31, 2023

borland commented Nov 10, 2023 • edited Loading

matkoch commented Nov 19, 2023 • edited Loading

borland commented Nov 20, 2023 • edited Loading

matkoch commented Nov 20, 2023

borland commented Nov 20, 2023

borland commented Nov 20, 2023 • edited Loading

matkoch commented Nov 20, 2023 • edited Loading

borland commented Nov 20, 2023 • edited Loading

borland commented Oct 24, 2023 •

edited

Loading

borland commented Nov 10, 2023 •

edited

Loading

matkoch commented Nov 19, 2023 •

edited

Loading

borland commented Nov 20, 2023 •

edited

Loading

borland commented Nov 20, 2023 •

edited

Loading

matkoch commented Nov 20, 2023 •

edited

Loading

borland commented Nov 20, 2023 •

edited

Loading