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

Build Performance #283

Merged
merged 5 commits into from
Feb 6, 2023
Merged

Build Performance #283

merged 5 commits into from
Feb 6, 2023

Conversation

bpulliam
Copy link
Collaborator

Description

Please include a summary of the change and/or which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Target Release

Please specify which release this PR should align with. e.g., 1.0, 1.1, 1.1 Preview 1.

Checklist

  • Samples build and run using the Visual Studio versions listed in the Windows development docs.
  • Samples build and run on all supported platforms (x64, x86, ARM64) and configurations (Debug, Release).
  • Samples set the minimum supported OS version to Windows 10 version 1809.
  • Samples build clean with no warnings or errors.
  • [For new samples]: Samples have completed the sample guidelines checklist and follow standardization/naming guidelines.
  • If I am onboarding a new feature, then I must have correctly setup a new CI pipeline for my feature with the correct triggers and path filters laid out in the "Onboarding Samples CI Pipeline for new feature" section in samples-guidelines.md.
  • I have commented on my PR /azp run SamplesCI-<FeatureName> to have the CI build run on my branch for each of my FeatureName my PR is modifying. This must be done on the latest commit on the PR before merging to ensure the build is up to date and accurate. Warning: the PR will not block automatically if this is not run due to '/azp run' limitation on triggering more than 10 pipelines.

@bpulliam
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@bpulliam
Copy link
Collaborator Author

/azp run SamplesCI-AppLifeCycle, SamplesCI-Composition, SamplesCI-Content, SamplesCI-CustomControls, SamplesCI-DeploymentManager, SamplesCI-Input, SamplesCI-Insights, SamplesCI-Installer

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

</PropertyGroup>
<Import Project="$(NugetPackageDirectory)\Microsoft.WindowsAppSDK.1.2.221109.1\build\native\Microsoft.WindowsAppSDK.props" Condition="Exists('$(NugetPackageDirectory)\Microsoft.WindowsAppSDK.1.2.221109.1\build\native\Microsoft.WindowsAppSDK.props')" />
<Import Project="$(NugetPackageDirectory)\Microsoft.Windows.SDK.BuildTools.10.0.22621.755\build\Microsoft.Windows.SDK.BuildTools.props" Condition="Exists('$(NugetPackageDirectory)\Microsoft.Windows.SDK.BuildTools.10.0.22621.755\build\Microsoft.Windows.SDK.BuildTools.props')" />
<Import Project="$(NugetPackageDirectory)\Microsoft.Windows.CppWinRT.2.0.221104.6\build\native\Microsoft.Windows.CppWinRT.props" Condition="Exists('$(NugetPackageDirectory)\Microsoft.Windows.CppWinRT.2.0.221104.6\build\native\Microsoft.Windows.CppWinRT.props')" />
<Import Project="..\..\..\..\Build.Common.Cpp.props" Condition="Exists('..\..\..\..\Build.Common.Cpp.props')"/>
<PropertyGroup Label="Globals">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're trying to keep these samples independently buildable (as they should be), then we should remove all common build logic, source code, etc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lolz

  • "we should remove all common build logic"
  • "we should consolidate common settings" in the next comment below

Agreed with both points, though on the latter the current code is redundant but I'd go a step further. Actually do common repo-wide definitions while still being individually buildable

One way is to make a common Directory.Build.props in the root and every project dir, identical except the project-dir copy starts with IF Exists(..\Directory.Build.props) THEN Import(..\Directory.Build.props) ELSE ...all the content setting properties etc in the root copy... Then future changes can be made to the root copy and a simply Powershell script can walk the tree regenerating all the local copies with that content plus the IF ELSE wrapper.

Other ways to achieve similar net result. Goal is to manually put common things (like PlatformToolset) in a single place and have it affect all, but still work if a developer just grabs 1 project and builds it

@@ -36,7 +40,7 @@
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="Configuration">
<ConfigurationType>Application</ConfigurationType>
<PlatformToolset>v142</PlatformToolset>
<PlatformToolset>v143</PlatformToolset>
<CharacterSet>Unicode</CharacterSet>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we're in here, we should consolidate common settings like this to make future updates simpler. and we should make this a default to allow overriding (upgrading)

@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Microsoft.UI.Xaml" version="2.8.0-prerelease.210927001" targetFramework="native" />
<package id="Microsoft.Web.WebView2" version="1.0.1018-prerelease" targetFramework="native" />
<package id="Microsoft.Web.WebView2" version="1.0.1083-prerelease" targetFramework="native" />
</packages>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prerelease?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this repo benefit from the DevCheck -CheckDependencies / MasterSourceOfTruth/master-dependency-definition/verification like we added recently in WinAppSDK?

Will copied DevCheck.* to eng\* to propogate everywhere via Maestro, of urgent need to leverage some functionality and more longer-term. That's currently an older copy as there were changes pending. I've got a change going in RSN to finish that (moving Foundation's (latest) copy to eng\) so it should be available for use in this repo if we want.

Do we want?

Copy link
Member

@Scottj1s Scottj1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some room for improvement, but no reason to block on that

@bpulliam bpulliam merged commit 7ab543b into main Feb 6, 2023
@bpulliam bpulliam deleted the user/bpulliam/build branch February 6, 2023 13:07
@bpulliam
Copy link
Collaborator Author

bpulliam commented Feb 6, 2023

I completely agree with all comments. I'm going ahead with the merge so we bank this level of improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants