Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update package titles and consolidate build logic #3894

Merged
merged 12 commits into from
Jul 20, 2021

Conversation

Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Mar 26, 2021

Fixes #3898

  1. Consolidate common MSBuild logic
  2. Organize/Consolidate Package tags
  3. Update Package titles and description
  4. Update solution to properly pack design tools
  5. Move in-package icon to root and rename to Icon.png (matching "official" dotnet packages).

Also Fixes #3897

PR Type

What kind of change does this PR introduce?

  • Code style update (formatting)
  • Refactoring (no functional changes, no API changes)
  • Build related changes

What is the current behavior?

It's hard to read and understand the repo (because of the redundant files and logic). Package titles are messy. Packages doesn't contain DesignTools plugins.

What is the new behavior?

It's now little bit better to read and understand the repo. Update Package titles to follow common (machine readable too) format. Packages now contain DesignTools plugins.

PR Checklist

Please check if your PR fulfils the following requirements:

  • Tested code with current supported SDKs
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

  • If you're editing this patch tree, please rebase on latest HEAD and then commit, without updating from the latest HEAD.
  • 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 bot does this by default.

@ghost
Copy link

ghost commented Mar 26, 2021

Thanks Nirmal4G for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi, Kyaa-dost and Rosuavio March 26, 2021 19:15
@Nirmal4G Nirmal4G changed the title Refactor project files Refactor project files and fixup markdown files Mar 26, 2021
@Nirmal4G Nirmal4G changed the title Refactor project files and fixup markdown files Update package titles and markdown files Mar 27, 2021
@Nirmal4G Nirmal4G changed the title Update package titles and markdown files Update package titles and refactor markdown files Mar 27, 2021
@Nirmal4G Nirmal4G changed the title Update package titles and refactor markdown files Update package titles, format markdown and consolidate build logic Mar 27, 2021
@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 3 times, most recently from 021c9dd to b60b7de Compare March 27, 2021 14:25
@Nirmal4G Nirmal4G changed the title Update package titles, format markdown and consolidate build logic Update package titles and consolidate build logic Mar 27, 2021
@ghost ghost added the improvements ✨ label Apr 3, 2021
@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 2 times, most recently from 34b3c01 to 4a74699 Compare May 5, 2021 09:44
@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 2 times, most recently from 5a28070 to f06b480 Compare May 17, 2021 20:20
@michael-hawker
Copy link
Member

This looks to be built on #3893, so waiting for that to be finished before we look at this one.

@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 2 times, most recently from 418f402 to a38b58c Compare June 17, 2021 14:11
@Nirmal4G Nirmal4G marked this pull request as ready for review July 16, 2021 18:42
Nirmal4G added 12 commits July 20, 2021 21:45
- Update comments to be more to the point.
- Rearrange some lines in ascending order.
- Add more file types and their preferences.
- Rename missed files from previous renames.
Organize Package tags
Update Package descriptions
Remove redundant MSBuild logic
Consolidate common MSBuild logic
Remove redundant MSBuild logic
Consolidate common MSBuild logic
Reorder code-blocks for better readability
Fix-up comments and new-lines across project files

Improve support for Visual Studio 2022:
Since VS IDE 2022 (17.0) is 64-bit only, replace all 32-bit specific MSBuild properties with generic ones.
Remove redundant 'Exclude' as bin/obj doesn't exist in the Shared project.

We use None to include items to be visible under the Shared Project
in VS IDE. But having both None and Compile/Page includes for the
same files will impact Build and IDE perf.

So, Only include None when Compile/Page is already active.
Makes sure that global NoWarn(s) can be added/removed
without editing each and every project file.
When the Controls project was exploded into several sub-projects,
they were not marked for build in the solution file, leading to the
missing of design tools assemblies in their respective control's packages.

This patch updates the solution file with build options similar to what
was already working, before the splitting of the Controls project.
Use same versions of dependencies across projects
to ensure a consistent and predictable builds.

This also fixes a Warning (NU1603) in the UI Test projects.
The package title doesn't follow a particular format and is not machine readable.
Thus, I propose the following format: "'Product' - 'Area' - 'Specifics' ('Source/Target')"
Remove targets that requires "DesignTools" check to let build succeed.
Revert this patch when adding the actual "DesignTools" project for Media Controls.
VS IDE still adds this but its no longer needed!
Every "official" .NET packages use 'Icon' as its file name.
Since, the icon file itself is public and older packages refer them by URL,
we only change the file name during packaging which would be in the package.
It was decided that the term 'Basic' is new and
has no established meaning in the software community.

So, Replacing the term 'Basic' with 'Common'. To disambiguate
multiple instances of 'Common', we use an already existing term
'Primitive' and we also append the Platform to which the package targets.
@michael-hawker
Copy link
Member

Apparently we have some merge conflicts now:

Directory.Build.props
Microsoft.Toolkit.Uwp.Notifications/Properties/Microsoft.Toolkit.Uwp.Notifications.rd.xml
Microsoft.Toolkit.Win32/README.md
UITests/UITests.Tests.MSTest/UITests.Tests.MSTest.csproj
UITests/UITests.Tests.Shared/UITests.Tests.Shared.projitems
UITests/UITests.Tests.TAEF/UITests.Tests.TAEF.csproj

It won't let me resolve them online here.

@ghost
Copy link

ghost commented Jul 20, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@ghost ghost removed the auto merge ⚡ label Jul 20, 2021
@Nirmal4G Nirmal4G force-pushed the feature/refactor branch 2 times, most recently from c3fcfb9 to e984265 Compare July 20, 2021 17:09
@Nirmal4G
Copy link
Contributor Author

Ahh… The PRs #4090 and #4117 broke this. I've fixed the conflicts.

@michael-hawker michael-hawker merged commit 747497a into CommunityToolkit:main Jul 20, 2021
@Nirmal4G Nirmal4G deleted the feature/refactor branch July 20, 2021 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate common build logic [Proposal] New Assembly/Package Title format
4 participants