Add support for building S.P.CoreLib in a separate job#34166
Conversation
|
|
|
It looks like we're missing a step to upload the intermediate artifacts so they can get pulled down by later jobs, so we should make sure we add that step. |
safern
left a comment
There was a problem hiding this comment.
This also needs to be done in runtime-official.yml
Removing compilerName parameter and renaming display names
Also some cleanup
|
Thanks Manish, you're making great progress here. As I see it based on our offline chat, as next step I would create another template, build-coreclr-job.yml, to basically contain (build-job.yml minus build-corelib-job.yml starting with corelib download from a previous corelib job) and change build-job.yml to just call these two templates. After that we can modify the template build-coreclr-and-libraries-job and adjust runtime and runtime-official as you've already started doing. As usual, we'll see what happens with the perf job that is somewhat special but I believe this should fix 90% of the existing pipelines. |
|
Hmm, you may be right that it's an overshoot to create yet another template and we can just "subtract" build-corelib-job from build-job. The only downside is that we need to audit all places in our pipelines calling build-job and update them accordingly. Keeping build-job as a wrapper for build-corelib-job followed by build-coreclr-job was supposed to maintain "backwards compatibility" during the transition. |
Keeping the download of corelib artifacts to check if just corelib is sufficient to build libraires.
|
How hard would it be to extract Mono CoreLib too as part of this? It already builds completely independent from the Mono runtime so should just need .yml/.proj changes. |
|
@ViktorHofer might have insights as well as he has been working on re-shuffling pieces to be able to restore ref and src projects as part of the libraries build. |
trylek
left a comment
There was a problem hiding this comment.
Looks great to me modulo Santi's remarks, thanks so much Manish for pulling this off!
the main reasoning for this is to improve build times, it appears Mono builds are fairly fast -- would this help ? |
Yeah, mono builds are fairly fast, I think the downsides of uploading/downloading assets and such, might not give us much gain, plus the only legs that build against mono are the iOS at the moment. |
|
I guess that, even with Mono build being fast, there would be some value in having both runtime builds behave the same including separate CoreLib build. Having said that, I believe that by all means this is worth a separate PR and perhaps even a separate design discussion. |
| - ${{ if or(ne(parameters.runtimeFlavor, 'coreclr'), ne(parameters.testScope, '')) }}: | ||
| - ${{ format('{0}_product_build_{1}{2}_{3}_{4}', parameters.runtimeFlavor, parameters.osGroup, parameters.osSubgroup, parameters.archType, parameters.liveRuntimeBuildConfig) }} | ||
| - ${{ if eq(parameters.osGroup, 'WebAssembly') }}: | ||
| - ${{ format('{0}_product_build_{1}{2}_{3}_{4}', parameters.runtimeFlavor, 'linux', parameters.osSubgroup, 'x64', parameters.liveRuntimeBuildConfig) }} |
There was a problem hiding this comment.
WebAssembly is still depending on the full coreclr build. We should be able to also make it build against corelib by doing something similare as above.
There was a problem hiding this comment.
yup will do as a subsequent PR.
safern
left a comment
There was a problem hiding this comment.
My last two comments are important to be addressed as part of this PR. After that, feel free to merge.
| BuildInParallel="true" | ||
| ContinueOnError="ErrorAndStop" /> | ||
| ContinueOnError="ErrorAndStop" | ||
| Condition="'$(SkipBuildProjects)' != 'true'"/> |
There was a problem hiding this comment.
What about instead directly invoking pretest.proj from CI and passing in the target that you care about?
There was a problem hiding this comment.
Yeah that makes sense. We could just call change the current call to /t: GenerateTestSharedFrameworkDepsFile.
Maybe on a follow-up PR so that he can merge?
| <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.targets" /> | ||
|
|
||
| <ItemGroup> | ||
| <SetupProjects Include="$(MSBuildThisFileDirectory)restore\runtime\runtime.depproj" /> |
There was a problem hiding this comment.
This is CI only, why not directly invoking the runtime.depproj before tests.proj?
There was a problem hiding this comment.
one issue is that build parallelization might cause a race, pretest.proj has to complete before tests start to build, since pretest generates the dep file to ensure testhost is setup correctly.
There was a problem hiding this comment.
Not following. You can do two steps before the test build so the sequence would be:
- runtime.proj
- pretest.proj and
- tests.proj
Noteworthy, if you would wait until Monday, we could just invoke the lib-pretest lib-tests subsets which would handle all that for you. I plan to merge this on Monday (if ready) which introduces the libraries subsets #33553.
There was a problem hiding this comment.
I ideally want to merge this tomorrow, since this PR touches a bunch of infra pieces. I can commit to reworking this once your PR is merged.
There was a problem hiding this comment.
No problem at all, I can make the changes afterwards.
|
This is now merged, I will open another PR for couple of outstanding comments. Thanks everyone for your review and help on this. |
|
regular merge, should it have been Squash? |
|
oops sorry, should have checked on that. |
WIP to add support to build System.Private.CoreLib separately so coreclr and libraries builds can run in parallel.