search

codebude / QRCoder

A pure C# Open Source QR Code implementation
MIT License
4.66k stars 1.11k forks source link

Remove framework overrides #560

Closed Shane32 closed 5 months ago

Shane32 commented 5 months ago

Summary

The QRCoder solution won't build on my Windows 11 laptop with a fresh installation of Visual Studio 2022, even with the .NET Framework 3.5 development tools installed. It seems to be related to the framework override csproj properties. It builds fine when I remove them.

Can we remove these properties?

Test plan

Ensure it builds without these excess properties, both locally and in CI.

codebude commented 5 months ago

The framework overrides were added a good 4 years ago by @csturm83 via https://github.com/codebude/QRCoder/commit/0d084052bc8957fa12766deddc465903afe8c4f3. Maybe he can still remember why.

Unfortunately, I no longer know. I can only assume that it was due to the build pipeline at the time via MyGet, where the .NET framework was not in the default path. But I'm not 100 percent sure. If csturm83 doesn't veto it, we can merge it.

csturm83 commented 5 months ago

@codebude @Shane32 It was a workaround documented in https://github.com/codebude/QRCoder/pull/189#discussion_r300832269. If the workaround is no longer needed to publish a 3.5 target in the current build pipeline, feel free to remove.

Shane32 commented 5 months ago

How do we test if it is needed anymore? Dotnet pack from within CI?

csturm83 commented 5 months ago

@Shane32 Not sure. There was an update in the comment that was referenced in the workaround (https://github.com/dotnet/msbuild/issues/1333#issuecomment-296346352) to reference ref assemblies via a NuGet package. You could try that if removing the overrides doesn't work.

codebude commented 5 months ago

You could try that if removing the overrides doesn't work.

I would say it works. At least the pipeline logs say that the net35 binaries were successfully build: https://github.com/codebude/QRCoder/actions/runs/9334333660/job/25692441349?pr=560#step:5:13

Shane32 commented 5 months ago

I had problems a while back where I was required to add the NuGet package reference to the .NET Reference Assemblies. However, I don't need to anymore, and in fact it breaks builds within the latest .NET SDK. See link below. The .NET team recommended that I remove the reference, even though there still exists MS documentation stating to include the reference for certain scenarios. Supposedly the reference is automatically included by the .NET SDK for .NET Framework builds or something. I would assume that the issue requiring the patch has since been resolved.

See: https://github.com/dotnet/aspnetcore/issues/54544

csturm83 commented 5 months ago

the net35 binaries were successfully build:

The original issue was surrounding dotnet pack. I suspect the issue in the SDK has since been resolved as @Shane32 suggests.