Refactor remaining Project and Build files

[Nirmal4G]

Info

Extracted from CommunityToolkit/WindowsCommunityToolkit#4234 as the Common Code projects were separated in this new repository.

Changes

Simplify Roslyn multi-targeting

• Refactor Roslyn multi-targeting to use multiple projects in the same project directory without using Shared projects.
• This refactoring is made in preparation for the solution to use the NuGet's CPVM and TargetFramework as alias features.
• This also slightly improves IDE load time and Build performance.

Move MSBuild logic into new files

• Move the T4 MSBuild logic to a new file.
• Move Public Key to a new file toolkit.spk.
• Move MSBuild logic to near-by Directory.Build.{props|targets} files.

Simplify MSBuild logic in project files

• Add necessary guard to check for pack.
• Remove redundant properties and values.
• Remove and adjust quotes in property functions.
• Use wildcards to generalize and reduce items declared.

Add, Update and Format build Comments

• Remove redundant comments.
• Add missing comments on certain code blocks.
• Format code in comments with proper quote style.

Misc Changes

• Fix the text flow warping in the MSBuild Console logging.
• Use checked version properties instead of hard-coded checks.
• Update the structure of the projects list in the Solution file.

PR Checklist

• [x] Created a feature branch in your fork
• [x] Based off latest main branch of toolkit
• [x] PR doesn't include merge commits (always rebase on top of our main)
• [x] Header has been added to all new source files (ran build/Update-Headers.ps1)
• [x] Tested code with current supported SDKs
• [x] Contains NO breaking changes
• [x] Code follows all style conventions

Notes

Please suggest alternate names for dotnet Community Toolkit.sln. We have already established that we can't use .NET. I tried using NET, DotNET, dotNET and Core as prefix. Except for Core, others neither look good nor stand-out in the file list.

• Rebase merge please.
• When merging, please update the commit title to PR title instead of the default Merge pull request #xxxx from repo/branch, and commit message to either PR message or messages of individual commits. The auto-merge option does this by default.

[Nirmal4G]

Project File structuring

• SOF (Start Of File) Imports,
• Core properties, Package properties, build properties,
• Build items, Resource items, Content items, Misc. items,
• In-place Imports, Choose (elements within follows outer sort order)
• ProjectReference items, PackageReference items,
• Targets and Properties and Items close to said Targets,
• EOF (End Of File) Imports.

Where there's a condition by target properties, we should group them together under Choose. For multiple target values, we should sort them by the order in which they are defined. And the order in which they should be defined is either ascending or descending in terms of compatibility layering.

[Nirmal4G]

@Sergio0694 PR closed by 237 by mistake. Can you reopen it?

[Sergio0694]

Huh, weird, yeah no problem! 😄

[Nirmal4G]

Included a missed commit during rebase. see this comment on why. It should also explain why (To keep the diff simple between each commit) some lines have been rearranged.

[Sergio0694]

@Nirmal4G This is just way too much for a single PR. Also, you just cannot keep adding new stuff after opening a PR. How is anyone supposed to leave a review if you keep changing more and more stuff after people have already started looking at the changes. This just forces people to review everything from scratch all over again, and it's really time consuming. Not to mention, the PR title says "refactoring", but clearly it's not doing just that. This PR cannot be reviewed or merged this way.

If you want to introduce some functional/build change, please open an issue first so we can discuss things, and then open a PR just for that change. You can't just keep adding more and more stuff to this PR, it causes too much churn and ovehead for us.

[Nirmal4G]

@Sergio0694

I'm so sorry if I edited this PR too much. I only did it to ease your reviewing. It's just that the PR has been open for so long that I had to optimize my private fork (not the one in my profile) a little to keep up with the changes. I mostly do the work in PR itself as I don't have a CI setup to test the changes. I'm in the process of doing that. Some of the changes that I pushed earlier were too soon and I only did it to check if it was being built successfully. I should've marked it as a draft.

For changes that actually matter, I do open issues along with a draft PR. Many of the changes I made here are very old. I have several versions of similar changes; I push one by one to check for build and squash some, edit some and finalize those changes to my private fork.

In the future, I'll make sure to contribute very narrow set of changes.

[Sergio0694]

This PR still doing way more than the title says. It's not just refactoring stuff, it's also making a whole lot of functional changes.

I should also repeat: I consider build changes to be very high-risk. I'm not seeing a benefit of changing so much stuff here. If there is a specific issue you think should be solved, then you should first open an issue with a proper explanation so we can discuss things, and only then open a PR once we all agree.

[Nirmal4G]

Is there any particular change you want to discuss? The only build change is the last commit that introduces a new Item type for packing common files regardless of pack content option! That's the only change but it's non-breaking.

[Sergio0694]

This PR includes a whole bunch of MSBuild changes and new targets. You can't just slip those kind of changes into a PR just saying it's "refactoring" stuff.

Again, if the PR says it's doing refactoring, there should be no functional changes.

[Nirmal4G]

Except last commit, there are no functional changes. Look carefully, I just simplified, re-arranged and extracted common MSBuild logic to a new file. That's all. Even the last commit is not functionally different but I do get that it's new. The last might be related to the Dynamic Pack PR I have; I can remove it from here if you're okay with that.

[Sergio0694]

• Moving the T4 stuff into a new .target should be its own PR.
• The whole last commit should be its own PR.
• Changing the lang version and the compiler strictness mode are functional changes.

This whole PR is like 4 different PRs all combined. And as I said, especially for the whole last commit, you should first open an issue to illustrate the issue, and then a PR if that's approved. It's not even clear to me what actual problem is the last commit trying to solve in the first place. This PR, as I said multiple times already, is just doing way more than it states.

I should also repeat, doing this literally just makes it more difficult to also review things, meaning it makes us waste time, and it makes you waste time as well for all the back and forth. It's just not the best way to contribute to the project. I appreciate your enthusiasm for wanting to help here, but this is really not the best way to go about it.

[Nirmal4G]

Moving the T4 stuff into a new .target should be its own PR.

That is called refactoring in MSBuild world! 😎. But I do get that you want it as a separate commit. I'll do that! But moving into a new PR is just too much!

The whole last commit should be its own PR.

I agree. It's related to the Dynamic Pack PR I already have. Can I leave it there? since that PR requires changes from this commit. So, the two must be reviewed and accepted together.

Changing the lang version and the compiler strictness mode are functional changes.

The binaries are the same. That's why I included this change in this PR. I can move those into the next PR or a new one.

Compiler strictness just enforces at language-level. But this Behavior changes only if we're using experimental and non-standard features. Lang version just sets it to the latest, since we're always upgrading and not downgrading our .NET SDK versions, we can use the latest language features if and when it becomes available. Use of strictness also ensures that we use watered down, restricted feature set so if and when Roslyn breaks something (I hope never) we won't get burned because of that!

[Nirmal4G]

I should also repeat, doing this literally just makes it more difficult to also review things, meaning it makes us waste time, and it makes you waste time as well for all the back and forth. It's just not the best way to contribute to the project. I appreciate your enthusiasm for wanting to help here, but this is really not the best way to go about it.

I'm so sorry about this. I'm new to the whole contributing thing. You can infer most of how I do source control from my commits. Creating a whole new PR is like applying for a passport or a visa (😢), it's too much for every single change. That's why I make related changes in a single PR and sometimes they become big. I'm still learning what constitutes a lean and mean PR for a particular project. Earlier you put different style refactoring all in one PR, even though it breaks binary compatibility.

The kinds of changes I make here and to other projects out there, no one really does them because it's boring, repetitive and as you've said causes too much churn but we have to do it at least once in a while to make the project good for first-time learners such as myself!

[Nirmal4G]

@Sergio0694 I have sent you a DM on Discord in regards to the updated patch tree here.

For only changes since the last review: https://github.com/CommunityToolkit/dotnet/compare/a3b2649..322bc61

[Nirmal4G]

@Sergio0694 Sorry for the notification but is there anything blocking this PR from getting merged?

[Nirmal4G]

@Sergio0694 Updated in response to PR #428 which broke this!

[Sergio0694]

Just a heads up, there's major changes I'm doing in #436, so this PR will need updates after that as well.

[michael-hawker]

Still going to suggest that we close this PR, as it should be broken out into smaller PRs as mentioned above.

@Nirmal4G in general doing things like renaming a directory from build to eng has little purpose or value to the project and just creates noise in these PRs. It's something that should be discussed in an issue first, folks can agree on a timing and value of the proposal, and then it can be coordinated with others working on the repo for a major folder restructure between releases.

Similarly, unless we can automate structuring of the project file XML with a tool like XAML Styler does for XAML files in the WCT repo, then manually moving Item/PropertyGroups around is just a temporary thing that also just creates more noise that's hard to review. Especially when mixed in with other changes in part of a larger PR.

Diving into making changes can be fun and exciting, but if you want to ensure your contributions are having a meaningful impact and will be merged; it's best to start a conversation with an issue or discussion first to lay out exactly what you plan to do, how it benefits the project, and what's involved with the change. This should be scoped to something that's a single 'block' and not a laundry list of lots of items. Each idea should be its own issue.

Or ask to pick up an already open bug or issue on the repo that someone's not working on or reach out on Discord to Sergio or myself and ask if there's something on the top-of-our-minds build related that we're struggling with that you could assist with. Really happy to have your passion directed at our projects, but we just want to ensure we're on the same page and can move forward together! Thanks!

[Nirmal4G]

@michael-hawker This is still in draft! Also this is on top of existing PRs. Changes that are mutually exclusive are separated but when they are cumulative, how they can be separated?

Code changes are one thing but refactorings are if not always cumulative and to produce clean diff and blame, they need to be arranged properly by theme of change or purpose of change.

I'm still a novice but if may ask, how would you separate this patch without causing conflicts that break build!?

[Nirmal4G]

@michael-hawker There's a discussion open for these PRs. It's part of a larger refactoring with the goal of introducing a common build infra for Community Toolkit repos (or atleast for my forks if we're not moving forward with the idea!).

1. CommunityToolkit/WindowsCommunityToolkit#4691
2. CommunityToolkit/dotnet#463

[wzxprince]

I am Chinese！！！I like it！