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

Replace external MSBuild processes with TaskHostFactory #14700

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

TomEdwardsEnscape
Copy link
Contributor

@TomEdwardsEnscape TomEdwardsEnscape commented Feb 22, 2024

This PR provides a 70% improvement to build times within the Avalonia repository.

What does the pull request do?

Avalonia.Build.Tasks is an MSBuild task assembly that is compiled at the start of every build. Its output assembly is then loaded by MSBuild for use by other solution projects.

Because MSBuild processes are kept alive in anticipation of future builds, the tasks assembly remains loaded after a build completes. This means that its DLL file cannot be modified (on Windows, at least). The locked assembly file causes build errors if something about the Avalonia.Build.Tasks project changes and the file needs to be rebuilt.

The current solution to this is to spawn an entirely new MSBuild process which spins up the entire project tree again, executes a specified target, then exits. This works, but is very slow, because MSBuild and the Avalonia project tree both take a considerable amount of time to load.

This PR replaces this system with TaskHostFactory. Like starting a new MSBuild process, this isolates the use of the Avalonia.Build.Tasks assembly to a short-lived process. Unlike starting a new MSBuild process, only the task and its immediate inputs are loaded. This is much, much faster.

The build time improvements also apply to design-time builds within Visual Studio. The IDE is now more responsive and completes loading the solution sooner.

Errors after switching to this branch

Please note that due to Visual Studio's project caching behaviour, you may encounter file locking errors after switching to this PR's branch with the Avalonia solution already loaded. Restarting the IDE will fix this. It's not a problem with the PR itself, and will stop being an issue once the changes are on master and spread to other branches.

The PR's changes can be easily validated under normal development circumstances by making a change to Avalonia.Build.Tasks and building the solution again.

What is the updated/expected behavior with this PR?

When building Avalonia.Desktop.slnf:

Scenario master PR Delta
Full rebuild 02:22 0:43 -70%
Build after invalidating projects 01:31 0:25 -73%

Breaking changes

None.

This entire subject only affects builds of the Avalonia repository, not builds of projects which consume Avalonia's Nuget packages. Package builds have always loaded the build tasks assembly normally, and continue to do so.

Obsoletions / Deprecations

None

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0045245-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Copy link
Member

@maxkatz6 maxkatz6 left a comment

Choose a reason for hiding this comment

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

Tested in Rider. Works without any issues.
Thank you!

@maxkatz6 maxkatz6 added this pull request to the merge queue Feb 22, 2024
Merged via the queue into AvaloniaUI:master with commit 3194e32 Feb 23, 2024
6 checks passed
@TomEdwardsEnscape TomEdwardsEnscape deleted the fixes/no-external-msbuild branch February 23, 2024 07:33
@grokys
Copy link
Member

grokys commented Jul 23, 2024

This PR seems to prevent the references file being output, which since AvaloniaUI/AvaloniaVS#413 is required by the VS extension. The effect of this is that intellisense in the VS extension is broken with 11.1.0.

There's even a comment to this end in the .targets file: https://github.com/AvaloniaUI/Avalonia/blob/master/packages/Avalonia/AvaloniaBuildTasks.targets#L249-L253

However the references file isn't created.

@TomEdwardsEnscape
Copy link
Contributor Author

The target you linked to was added three months ago in #14397. That's after this PR, which was completed five months ago.

@grokys
Copy link
Member

grokys commented Jul 24, 2024

Yeah, that PR didn't fix it in fact. Should be fixed by #16427.

EDIT: To clarify for anyone spelunking this later - this PR did remove references generation, and #14397 attempted to fix that. However it looks like it didn't actually fix it. The (hopefully) final fix is in #16427.

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.

4 participants