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

Bootstrap improvements #10282

Merged

Conversation

YuliiaKovalova
Copy link
Member

@YuliiaKovalova YuliiaKovalova commented Jun 24, 2024

Fixes #10300

Context

The PR introduces a simplified and transparent way for getting sdk bits and patching the changes to them using install-scripts.
Details can be found in the doc: https://github.com/dotnet/msbuild/blob/5852b3d778362105203ac4e550fd9456028627c5/documentation/wiki/Bootstrap.md

Notes

Might resolve #6566

@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/bootstrap_improvements branch 5 times, most recently from 8185e62 to 0fee63d Compare June 26, 2024 20:18
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/bootstrap_improvements branch from 0fee63d to 0907c8c Compare June 26, 2024 20:20
@YuliiaKovalova YuliiaKovalova changed the title Dev/ykovalova/bootstrap improvements Bootstrap improvements Jun 26, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Jun 26, 2024
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/bootstrap_improvements branch from 41a3efa to 6196063 Compare June 27, 2024 10:20
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review June 27, 2024 10:24
@YuliiaKovalova YuliiaKovalova requested a review from a team as a code owner June 27, 2024 10:24
@JanKrivanek
Copy link
Member

This feels as it deserves some very small doc on what is the current bootstrap approach on grabbing bits (does it use '.dotnet'?) and what are other alternatives and their pros and cons.
This change looks OK - I'm just lost if that's the only option and if we're OK with possible introduced full build slowdown.

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

This looks great!

I'm putting 'requesting changes' just so that the following two things are reviewed (and I'm fine if consciously dismissed):

  • The new task might be better to inherit from some of our ready-made base
  • The versioning info might benefit from centralization - if easy to do so. Or/and documenting where it needs to be updated and how is the new version determined

eng/BootStrapMsBuild.props Outdated Show resolved Hide resolved
eng/cibuild_bootstrapped_msbuild.sh Outdated Show resolved Hide resolved
src/MSBuild.Bootstrap.Utils/Tasks/InstallDotNetCoreTask.cs Outdated Show resolved Hide resolved
src/MSBuild.Bootstrap/MSBuild.Bootstrap.csproj Outdated Show resolved Hide resolved
documentation/wiki/Bootstrap.md Show resolved Hide resolved
eng/BootStrapMsBuild.props Outdated Show resolved Hide resolved
documentation/wiki/Bootstrap.md Show resolved Hide resolved
documentation/wiki/Bootstrap.md Outdated Show resolved Hide resolved
documentation/wiki/Bootstrap.md Outdated Show resolved Hide resolved
eng/cibuild_bootstrapped_msbuild.sh Outdated Show resolved Hide resolved
eng/cibuild_bootstrapped_msbuild.ps1 Outdated Show resolved Hide resolved
src/MSBuild.Bootstrap.Utils/Tasks/InstallDotNetCoreTask.cs Outdated Show resolved Hide resolved
eng/BootStrapMsBuild.targets Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/bootstrap_improvements branch from b107f90 to 0428ff2 Compare July 11, 2024 16:18
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/bootstrap_improvements branch from 54fc686 to e9ce4ef Compare July 11, 2024 17:03
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/bootstrap_improvements branch from aa7a5bf to cb5dd94 Compare July 11, 2024 19:29
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks great!

I have couple comments for consideration

documentation/release-checklist.md Outdated Show resolved Hide resolved
src/MSBuild.Bootstrap.Utils/Tasks/InstallDotNetCoreTask.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/bootstrap_improvements branch from c0e58c7 to 5852b3d Compare July 12, 2024 14:54
This avoids ever having to block on building the task.

I'm leaving the project in place for easier editing, but not including
it in the `.sln` so it won't be part of any build--you'd have to open
it individually to get a great inner-loop experience with the task.
Pushing this lengthy process before `ResolveProjectReferences` means
it's more likely to be able to be scheduled and finish without being on
the critical path through builds.
@rainersigwald
Copy link
Member

Ok so I am going to be a hyper-nitpicker in the hopes that this shares some of my "how to think about build process changes" opinions.

On my machine, this took 17 seconds (rm -r artifacts && build.cmd), and increased overall build time by like 40%.

image

I think we can do better!

In general you reduce overall build time by getting work off the critical path. As currently written this is well on the critical path:

  1. There's a new task that must be built before the Bootstrap project can proceed.
  2. Execution of that task takes a long time.
  3. Test projects depend on the bootstrap (because UnitTests.Shared depends on it).

But that's not set in stone.

We can use a RoslynCodeTaskFactory to avoid the project. Then executing the task need have no dependencies, it could be literally the first thing the build does: 9be76bc.

Then we can rearrange the call to the task to be earlier, instead of just-in-time for bootstrap creation (which implicitly delays it until most of the repo has built): d0c2057.

On my machine that knocks a good 15 seconds off the end-to-end build time.

documentation/release-checklist.md Outdated Show resolved Hide resolved
eng/BootStrapMsBuild.props Outdated Show resolved Hide resolved
src/UnitTests.Shared/BootstrapLocationAttribute.cs Outdated Show resolved Hide resolved
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

I still don't like that we seem to need to specify an MSBuild version in the post-bootstrap build, but we can attack that later.

documentation/release-checklist.md Outdated Show resolved Hide resolved
src/MSBuild.Bootstrap/MSBuild.Bootstrap.csproj Outdated Show resolved Hide resolved
eng/BootStrapMsBuild.targets Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova merged commit ba2ea6f into dotnet:main Jul 30, 2024
10 checks passed
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.

Rework the approach to bootstrap Enable workload resolver in bootstrap MSBuild
3 participants