Remove Roslyn 4.3 workaround, bump to 4.3.1

[Sergio0694]

This PR removes the Roslyn 4.4 workaround and bumps the 4.3 dependency to 4.3.1.

Warning: this is a breaking change for consumers using VS 17.3.x before the 17.3.5 patch. This is by design, as VS 17.3.x versions before the latest patch are unsupported (EOL), so build errors there are fine. The changes here allow all consumers on VS >= 17.3.5 to leverage all the new performance improvements that ForAttributeWithMetadataName provides.

cc. @michael-hawker Any concerns with the release strategy here? I guess we just need to call this out in the blog post?

[Sergio0694]

@Nirmal4G Can you take a look at the build failure? I'd be fine just removing the caching, but I'd imagine you'd want to find a way to fix that and leave the caching in place instead, so let me know if you can help with this. Thank you! 🙂

[michael-hawker]

@Sergio0694 if all the patch VS versions are out of their support date, then this seems fine. Will they have a message outputted that tells them to update VS? (Or is that something we can have the source generator emit on our end to aid in understanding beyond general failure?)

[Sergio0694]

@michael-hawker I did confirm with @333fred that all patch versions before the latest are effectively EOL, so yes that should be fine. Currently, people using a 17.3 version before 17.3.5 will get some error regarding analyzers failing to load due to some assembly reference with an incorrect version, which yeah isn't the best. Not sure we can emit a message, as the Roslyn assembly version in VS is still the same, so I don't think we can detect that version from our .targets (or even if we could, whether the extra complexity there would be worth it). Not entirely sure what the best approach is here. In theory since 17.4.3 and below (up to 17.3.0) are EOL we could pretty much just shrug? I can agree it's not the best UX for devs on those versions though...

[Nirmal4G]

@Sergio0694 As I suspected, the renamed file was not committed.

But having multiple project folders and files just to target multiple versions of the library without separate source files in them is a disaster waiting to happen. I knew this could happen and that's why I simplified Roslyn targeting with some MSBuild magic (at least for my private fork).

[Sergio0694]

@Nirmal4G Yeah I had missed a file, it's fixed now. I disagree that it's a "disaster waiting to happen", I simply missed a file, the CI failed, we fixed it, now it's all good. Nothing bad happened, it's really not a big deal, so I'd like to keep this setup and avoid complicating things even further. This was a one-off anyway and I really don't expect to change those projects at all 🙂

[Nirmal4G]

@Sergio0694 Sure, no problem.

so, I'd like to keep this setup and avoid complicating things even further. This was a one-off anyway and I really don't expect to change those projects at all.

I personally don't like the current setup. It uses shared projects which are not VSCode friendly. It feels more like a hack and is very brittle. The whole point of separating the projects is to keep it dotnet cli friendly. But adding shared projects means throwing VS IDE back into the mix. And it stops people using VSCode to contribute back. Case in point.

But my patch (still a work in progress) removes the need for shared projects and is VSCode friendly. So, what makes it complicated? If we need the logic more focused, we can do so in a review. This patch also paves a simple path to implement Central package versioning and output paths. Is there any way we could still discuss this?

More Information regarding my Build patches

There are so many build improvements like dynamic packing of Roslyn source generators, central output paths and central package versions which greatly benefit this and other CommunityToolkit repos. We can go further by eliminating the additional project file per Roslyn version when NuGet/Home#5154 is implemented. Everyone, even Runtime build-infra team is so interested in this.

I'm also building towards a CommunityToolkit.Build.Sdk which would eliminate all those pesky props/targets files altogether similarly to Arcade SDK. But this one is very far in the future.

[Sergio0694]

"Is there any way we could still discuss this?"

I'd recommend creating an issue/discussion pasting your last comment too, and we can talk about it there 🙂

[Nirmal4G]

Thanks, I have started a discussion. See #463.

[Nirmal4G]

I can agree it's not the best UX for Devs on those versions though...

There's a ReadMe for NuGet Packages, right? We could do an update to ReadMe, call this in the blog post and we can update the release notes section in the package to point to breaking changes/migration docs.

I'm sure in one way or another people will see one of these.

[Nirmal4G]

I have made updates to this branch. For those who have not installed the .NET SDK 7 locally, until the patch for .NET SDK v6 and VS v17.3 comes around, they are stuck building with the wrong compiler. I have added a patch to build the repo locally with the existing setup.

[Sergio0694]

I'm not worried about local builds, especially because I'll be adding the .NET 7 target shortly anyway, so people will have to use that SDK locally too, which will include the fix for this issue already. For now it builds fine locally once you have VS 17.3.5 or greater, and it passes fine in the CI as well, that's all we need for the time being 🙂