Add support for building S.P.CoreLib in a separate job #34166

mangod9 · 2020-03-26T22:48:34Z

WIP to add support to build System.Private.CoreLib separately so coreclr and libraries builds can run in parallel.

dnfclas · 2020-03-26T22:48:46Z

All CLA requirements met.

eng/pipelines/coreclr/templates/build-corelib-job.yml

eng/pipelines/runtime.yml

jkoritzinsky · 2020-03-26T22:58:30Z

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

This also needs to be done in runtime-official.yml

eng/pipelines/coreclr/templates/build-corelib-job.yml

eng/pipelines/runtime.yml

Removing compilerName parameter and renaming display names

eng/pipelines/coreclr/templates/build-corelib-job.yml

Also some cleanup

trylek · 2020-03-27T18:30:41Z

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.

trylek · 2020-03-27T18:39:29Z

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.

eng/pipelines/libraries/base-job.yml

akoeplinger · 2020-04-01T21:22:03Z

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.

eng/pipelines/libraries/outerloop.yml

eng/pipelines/libraries/run-test-job.yml

src/libraries/tests.proj

safern · 2020-04-01T21:50:17Z

@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

Looks great to me modulo Santi's remarks, thanks so much Manish for pulling this off!

eng/pipelines/coreclr/templates/build-corelib-job.yml

eng/pipelines/coreclr/templates/xplat-pipeline-job.yml

eng/pipelines/libraries/base-job.yml

mangod9 · 2020-04-01T23:08:37Z

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.

the main reasoning for this is to improve build times, it appears Mono builds are fairly fast -- would this help ?

safern · 2020-04-01T23:18:38Z

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.

trylek · 2020-04-01T23:20:19Z

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.

safern · 2020-04-01T23:23:04Z

eng/pipelines/libraries/build-job.yml

+          - ${{ if and(eq(parameters.runtimeFlavor, 'coreclr'), eq(parameters.testScope, '')) }}:
+            - ${{ format('coreclr_corelib_build_{0}{1}_{2}_{3}', parameters.osGroup, parameters.osSubgroup, parameters.archType, parameters.liveRuntimeBuildConfig) }}
+          - ${{ 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) }}


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.

yup will do as a subsequent PR.

these are now included in this PR: #34460

eng/pipelines/libraries/run-test-job.yml

eng/pipelines/coreclr/ci.yml

eng/pipelines/runtime.yml

safern

My last two comments are important to be addressed as part of this PR. After that, feel free to merge.

safern

Thanks for working on this 😄

ViktorHofer · 2020-04-02T07:51:21Z

src/libraries/pretest.proj

@@ -78,7 +78,8 @@
             Projects="@(Project)"
             Properties="TargetFramework=$(BuildTargetFramework);%(Project.AdditionalProperties)"
             BuildInParallel="true"
-             ContinueOnError="ErrorAndStop" />
+             ContinueOnError="ErrorAndStop" 
+             Condition="'$(SkipBuildProjects)' != 'true'"/>


What about instead directly invoking pretest.proj from CI and passing in the target that you care about?

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?

Yeah I'm fine with that.

ok will do on next PR.

ViktorHofer · 2020-04-02T07:52:03Z

src/libraries/tests.proj

@@ -41,8 +41,19 @@
  <Import Project="$(RepositoryEngineeringDir)coverage.targets" Condition="'$(EnableCoverageSupport)' == 'true'" />
  <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.targets" />

+  <ItemGroup>
+    <SetupProjects Include="$(MSBuildThisFileDirectory)restore\runtime\runtime.depproj" />


This is CI only, why not directly invoking the runtime.depproj before tests.proj?

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.

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.

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.

No problem at all, I can make the changes afterwards.

mangod9 · 2020-04-02T14:27:01Z

This is now merged, I will open another PR for couple of outstanding comments. Thanks everyone for your review and help on this.

ViktorHofer · 2020-04-02T16:52:06Z

@mangod9 how did you merge this? Squash or merge? cc @jkotas

mangod9 · 2020-04-02T16:57:07Z

regular merge, should it have been Squash?

jkotas · 2020-04-02T16:58:57Z

Yep: https://github.com/dotnet/runtime/blob/master/docs/pr-guide.md#merging-pull-requests

Next time...

mangod9 · 2020-04-02T17:03:51Z

oops sorry, should have checked on that.

Add support for building S.P.CoreLib in a separate job

74cb414

mangod9 added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Infrastructure-coreclr labels Mar 26, 2020

mangod9 requested review from jkoritzinsky and trylek March 26, 2020 22:48

mangod9 commented Mar 26, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Outdated Show resolved Hide resolved

mangod9 commented Mar 26, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Show resolved Hide resolved

mangod9 commented Mar 26, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Outdated Show resolved Hide resolved

mangod9 commented Mar 26, 2020

View reviewed changes

eng/pipelines/runtime.yml Show resolved Hide resolved

safern reviewed Mar 26, 2020

View reviewed changes

mangod9 added 2 commits March 26, 2020 20:31

PR Comments.

def5d70

Removing compilerName parameter and renaming display names

fix deletion typo

fc99faa

mangod9 changed the title ~~Add support for building S.P.CoreLib in a separate job~~ [WIP] Add support for building S.P.CoreLib in a separate job Mar 27, 2020

safern reviewed Mar 27, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Show resolved Hide resolved

safern reviewed Mar 27, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Outdated Show resolved Hide resolved

safern reviewed Mar 27, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Outdated Show resolved Hide resolved

safern reviewed Mar 27, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Show resolved Hide resolved

Adding step to upload output directory.

de8f012

Also some cleanup

mangod9 added 2 commits March 27, 2020 11:43

update libraries job to depend on corelib instead of product

34a19e8

reverting libraries dependency change

eb1ee34

Keeping the download of corelib artifacts to check if just corelib is sufficient to build libraires.

safern reviewed Mar 27, 2020

View reviewed changes

eng/pipelines/libraries/base-job.yml Outdated Show resolved Hide resolved

Add OSX and musl_arm64 corelib builds + use full product for mono

478b77f

safern reviewed Mar 28, 2020

View reviewed changes

eng/pipelines/libraries/base-job.yml Show resolved Hide resolved

mangod9 added 3 commits March 27, 2020 21:50

Another attempt to fix mono iOS case

a6a2a3b

propagating runtimeFlavor to libraries base-job.yml

fbfb35e

updating DependsOn for libraries job to use corelib

46efc4d

safern reviewed Apr 1, 2020

View reviewed changes

eng/pipelines/libraries/outerloop.yml Show resolved Hide resolved

safern reviewed Apr 1, 2020

View reviewed changes

eng/pipelines/libraries/run-test-job.yml Outdated Show resolved Hide resolved

safern reviewed Apr 1, 2020

View reviewed changes

eng/pipelines/libraries/run-test-job.yml Outdated Show resolved Hide resolved

safern reviewed Apr 1, 2020

View reviewed changes

eng/pipelines/libraries/run-test-job.yml Outdated Show resolved Hide resolved

safern reviewed Apr 1, 2020

View reviewed changes

src/libraries/tests.proj Outdated Show resolved Hide resolved

trylek approved these changes Apr 1, 2020

View reviewed changes

eng/pipelines/coreclr/templates/build-corelib-job.yml Outdated Show resolved Hide resolved

eng/pipelines/coreclr/templates/xplat-pipeline-job.yml Show resolved Hide resolved

eng/pipelines/libraries/base-job.yml Show resolved Hide resolved

mangod9 added 2 commits April 1, 2020 15:58

PR Comments

56bf4c0

typo fix :(

18e5190

safern reviewed Apr 1, 2020

View reviewed changes

eng/pipelines/libraries/run-test-job.yml Outdated Show resolved Hide resolved

Fix run-test-job pretest build

e76db3a

safern reviewed Apr 2, 2020

View reviewed changes

eng/pipelines/coreclr/ci.yml Outdated Show resolved Hide resolved

safern reviewed Apr 2, 2020

View reviewed changes

eng/pipelines/runtime.yml Outdated Show resolved Hide resolved

safern approved these changes Apr 2, 2020

View reviewed changes

removed some unused corelib configs.

e3c15b0

safern approved these changes Apr 2, 2020

View reviewed changes

ViktorHofer reviewed Apr 2, 2020

View reviewed changes

mangod9 merged commit b927233 into dotnet:master Apr 2, 2020

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for building S.P.CoreLib in a separate job #34166

Add support for building S.P.CoreLib in a separate job #34166

mangod9 commented Mar 26, 2020

dnfclas commented Mar 26, 2020 •

edited

Loading

jkoritzinsky commented Mar 26, 2020

safern left a comment

trylek commented Mar 27, 2020

trylek commented Mar 27, 2020

akoeplinger commented Apr 1, 2020

safern commented Apr 1, 2020

trylek left a comment

mangod9 commented Apr 1, 2020

safern commented Apr 1, 2020

trylek commented Apr 1, 2020

safern Apr 1, 2020

mangod9 Apr 1, 2020

mangod9 Apr 2, 2020

safern left a comment

safern left a comment

ViktorHofer Apr 2, 2020

safern Apr 2, 2020

ViktorHofer Apr 2, 2020

mangod9 Apr 2, 2020

ViktorHofer Apr 2, 2020

mangod9 Apr 2, 2020

ViktorHofer Apr 2, 2020 •

edited

Loading

mangod9 Apr 2, 2020

ViktorHofer Apr 2, 2020 •

edited

Loading

mangod9 commented Apr 2, 2020

ViktorHofer commented Apr 2, 2020

mangod9 commented Apr 2, 2020

jkotas commented Apr 2, 2020

mangod9 commented Apr 2, 2020

Add support for building S.P.CoreLib in a separate job #34166

Add support for building S.P.CoreLib in a separate job #34166

Conversation

mangod9 commented Mar 26, 2020

dnfclas commented Mar 26, 2020 • edited Loading

jkoritzinsky commented Mar 26, 2020

safern left a comment

Choose a reason for hiding this comment

trylek commented Mar 27, 2020

trylek commented Mar 27, 2020

akoeplinger commented Apr 1, 2020

safern commented Apr 1, 2020

trylek left a comment

Choose a reason for hiding this comment

mangod9 commented Apr 1, 2020

safern commented Apr 1, 2020

trylek commented Apr 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

mangod9 commented Apr 2, 2020

ViktorHofer commented Apr 2, 2020

mangod9 commented Apr 2, 2020

jkotas commented Apr 2, 2020

mangod9 commented Apr 2, 2020

dnfclas commented Mar 26, 2020 •

edited

Loading

ViktorHofer Apr 2, 2020 •

edited

Loading

ViktorHofer Apr 2, 2020 •

edited

Loading