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

Add PackageDependenciesDesignTime to cache assets file #28405

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

ocallesp
Copy link
Contributor

@ocallesp ocallesp commented Oct 7, 2022

Fixes #27738

Implements reading the PackageDefinitions and PackageDependencies items in ResolvePackageAssets, so that ResolvePackageDependencies doesn't need to be called at all unless EmitLegacyAssetsFileItems is true. This improve incremental build perf and also significantly improve the design time build perf (23%) for this large solution scenario by saving to the cache file the same items generated from PreprocessPackageDependenciesDesignTime.


Note:
This implementation was taken from PreprocessPackageDependenciesDesignTime.ExecuteCore

@ocallesp ocallesp requested review from ericstj, a team and joperezr as code owners October 7, 2022 20:39
@ocallesp ocallesp changed the base branch from main to release/7.0.2xx October 7, 2022 20:40
@ocallesp ocallesp changed the title GitHub 27738 Add PackageDependenciesDesignTime to cache assets file Oct 7, 2022
@dotnet dotnet deleted a comment from dotnet-issue-labeler bot Oct 7, 2022
@ocallesp ocallesp force-pushed the github-27738 branch 2 times, most recently from 0f4c1e7 to d383ef0 Compare October 7, 2022 21:00
@ocallesp ocallesp marked this pull request as draft October 7, 2022 21:55
@ocallesp ocallesp force-pushed the github-27738 branch 4 times, most recently from 8a2df1e to 2bb5447 Compare October 8, 2022 00:31
@ocallesp ocallesp marked this pull request as ready for review October 8, 2022 01:01
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

See comments for ways we should be able to eliminate a bunch of the code here and process less data.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks.

Have you made any progress with @drewnoakes on running the project system tests with these changes?

Also do you have any perf testing results? It would be great to be able to present those (or a demo) at the next team showcase.

Comment on lines 21 to 22
string projectAssetsJsonPath = Path.GetTempFileName();
string projectCacheAssetsJsonPath = Path.GetTempFileName();
Copy link
Member

@drewnoakes drewnoakes Nov 21, 2022

Choose a reason for hiding this comment

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

The test should delete both of these files when it completes, to avoid polluting the temp dir.

The same is true for other tests below.

@dsplaisted dsplaisted merged commit 866c1ac into release/7.0.2xx Nov 30, 2022
@dsplaisted dsplaisted deleted the github-27738 branch November 30, 2022 22:46
@ocallesp
Copy link
Contributor Author

@marcin-krystianc I see that the repository https://github.com/marcin-krystianc/TestSolutions/tree/master/LargeAppWithPrivatePackagesCentralisedNGBVRemoved does not contain any readme file nor a license.

We would like to know if we are allowed to share this with folks.

@marcin-krystianc
Copy link
Contributor

@marcin-krystianc I see that the repository https://github.com/marcin-krystianc/TestSolutions/tree/master/LargeAppWithPrivatePackagesCentralisedNGBVRemoved does not contain any readme file nor a license.

We would like to know if we are allowed to share this with folks.

Hi @ocallesp, I'll add some readme and license information, but of course it is ok to share it with others.

@ocallesp
Copy link
Contributor Author

@marcin-krystianc I appreciate your willingness to share your repository with others.

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.

5 participants