Use platform-matrix and split jobs by steps for libraries pipelines #274

safern · 2019-11-26T08:42:44Z

In this PR we now use platform-matrix and the shared logic to define pool, containers, helix-queues, xplat variables, checkout the repo, etc.

Also, as part of this PR we now split the builds as follows:

Build sources
Build tests for 3 different OSs (Linux, OSX and Windows).
Run tests for our test matrix.

At the beginning this might cause slower builds, but there will be follow up work in order to improve them as we're changing the queues to align with what coreclr uses in order to share the same native toolset and to be coherent with hour dependencies.

Of course we will keep working on improving CI times and making it better.

This is needed for coreclr to depend on libraries steps for live-live CI and next steps are to start plumbing coreclr into libraries builds.

Also, there's clean-up we can do afterwards to consolidate artifacts names and all the paths we use to publish and download artifacts.

FYI: @stephentoub

cc: @dotnet/runtime-infrastructure

trylek

Looks awesome to me. Thank you!

safern · 2019-11-26T08:57:46Z

/azp run runtime-libraries outerloop

azure-pipelines · 2019-11-26T08:57:57Z

Azure Pipelines successfully started running 1 pipeline(s).

jashook

This looks great thank you for the work.

safern · 2019-11-26T18:58:39Z

/azp run runtime-libraries outerloop

azure-pipelines · 2019-11-26T18:58:50Z

Azure Pipelines successfully started running 1 pipeline(s).

ViktorHofer · 2019-11-26T21:07:14Z

eng/referenceAssemblies.props

  <PropertyGroup>
    <IsReferenceAssembly Condition="'$(IsReferenceAssembly)' == '' and ($(MSBuildProjectFullPath.Contains('\ref\')) or $(MSBuildProjectFullPath.Contains('/ref/')))">true</IsReferenceAssembly>

    <!-- Create a common root output directory for all reference assemblies -->


the comment needs to be updated.

ViktorHofer · 2019-11-26T21:09:31Z

eng/pipelines/libraries/variables.yml

    value: $(Build.SourcesDirectory)/src/libraries
  - name: pipelinesPath
    value: /eng/pipelines/libraries
+  - name: isOfficialBuild


isn't this something that coreclr's and installer's pipelines would use as well?

It is unclear if it will be used I think

don't you have any logic specific to internal builds?

Please just be considerate with making additional fundamental changes at this point as it will reset all the testing and delay my live-live pipeline work. Frankly speaking I would be the happiest if we could merge this change in right now its current form and follow up with additional cleanups in separate PR's.

I'm reviewing the PR right now and will comment on what points out. We can address those in a subsequent PR of course.

That was my plan. So at part of this iteration I wanted to just make the minimal set of changes on how we had set it up for libraries (using the same variables and all that). In a follow-up PR I plan working with @trylek and trying to define as much shared variables as possible.

But yes, this template can be shared and we can have a variable to know if it is internal or public build. As they use that to define the helix-queues. https://github.com/dotnet/runtime/blob/master/eng/pipelines/coreclr/templates/helix-queues-setup.yml#L118

So making changes around it now seems like effort incorrectly spent.

Yes, that's why I wanted to leave pretty much as is. Don't want to spin up CI again.

Anyway I will have a follow-up PR probably to unify the test and build steps again for the time being as the machine provisioning delay is impacting the build time by a LOT.

Thanks.

I believe code which is added via a PR should be reviewed carefully. In my experience, changing or getting rid of stuff after-merge is more costly/complex, mainly because of the lack of context. I would prefer having TODO comments where we intend to change the behavior later but I understand that we want to get this in asap therefore I'm fine with not adding them now.

This applies to any kind of PR, not specific to this one.

For this one since the changes were complicated and since we don’t really know what the final shape is going to be, I didn’t even knew which TODOs to add. After this is merged I will go and investigate what can be removed, honestly I submitted it as is as it was blocking @trylek.

I think reviewing the build result and platform matrix should suffice.

ViktorHofer · 2019-11-26T21:41:53Z

eng/pipelines/libraries/base-job.yml

+
+        # Windows variables
+        - ${{ if eq(parameters.osGroup, 'Windows_NT') }}:
+          - _msbuildCommand: powershell -ExecutionPolicy ByPass -NoProfile eng\common\msbuild.ps1 -warnaserror:0 -ci


why do we need warnaserror false here?

Actually this was ported to xplat-setup and I can remove it. And I also think it was added for a reason I can’t remember anymore. So I will remove it in a follow up PR.

Thanks.

And I also think it was added for a reason I can’t remember anymore.

That's exactly what I was referring to above with "lack of context" :)

ViktorHofer · 2019-11-26T21:55:27Z

eng/pipelines/libraries/build-job.yml

+
+        - ${{ if eq(parameters.isOfficialBuild, true) }}:
+          - task: DotNetCoreCLI@2
+            displayName: Restore internal tools


I think it's worth discussing if we should extract that into the common layer in a subsequent PR.

Related: https://github.com/dotnet/runtime/blob/master/eng/install-nuget-credprovider.sh

Yeah I think this should definitely go into the common layer whenever we do the official build.

ViktorHofer · 2019-11-26T22:14:54Z

eng/pipelines/libraries/build-job.yml

+  archType: ''
+  framework: netcoreapp
+  isOfficialBuild: false
+  timeoutInMinutes: 150


can we adjust the timeout per leg? Often we needed to change it only for arm/arm64.

Yes, we can override it in platform-matrix.

safern · 2019-11-28T08:10:51Z

I've modified the PR to run the tests as part of the build step (however the templates for granular steps is already there). The reason why I had to do this is because it turns out our tests are Configuration dependent, so if you build the tests on Debug they test framework behavior for that configuration, which was causing the build to have test failures because we wanted to have the minimal set of test builds. In order to avoid expanding the build-test matrix and introduce more steps, I decided to keep it simple to unblock @trylek's work to be able to merge before the holidays so that he can make progress on his end.

The plan is to submit a PR to make tests Configuration agnostic and once that's in, I can go back to what I had initially in .azure-ci and I will also do the clean up suggested in the reviews.

safern · 2019-11-28T10:22:44Z

Libraries CI which is the only one really affected is green. Will merge to unblock @trylek's work, I will follow up with PRs once #378 is merged.

jaredpar · 2019-12-02T15:10:21Z

<!-- disable CS8597 because we throw null on reference assemblies. -->

Is there an issue tracking the reference assembly code producing throw null! instead of disabling this warning?

Refers to: eng/referenceAssemblies.props:18 in f985105. [](commit_id = f985105, deletion_comment = False)

jaredpar · 2019-12-02T15:11:24Z

<!-- disable CS8597 because we throw null on reference assemblies. -->

Or rather if there is no intent to emit null correct code in reference assemblies, reasonable, why not use nullable annotate instead of compiling with nullable checking?

In reply to: 560437235 [](ancestors = 560437235)

Refers to: eng/referenceAssemblies.props:18 in f985105. [](commit_id = f985105, deletion_comment = False)

safern added 6 commits November 21, 2019 16:36

Initial work to make corefx templates re-usable for live live

e02dfd4

Support building and running tests

6733081

First try to use platform-matrix

2e79ad4

Add a common layer in libraries build and a job/test template

bdcd457

Removed unused yaml and projects

2121faa

Flow test scope and move testScope arg to base template

253034b

safern requested review from ViktorHofer, jashook and trylek November 26, 2019 08:42

Dotnet-GitSync-Bot added the area-Infrastructure-coreclr label Nov 26, 2019

safern added area-Infrastructure-libraries and removed area-Infrastructure-coreclr labels Nov 26, 2019

trylek approved these changes Nov 26, 2019

View reviewed changes

Fix dependencies

3fa0328

safern force-pushed the LibrariesLiveLiveCi branch from 4ae62a5 to 3fa0328 Compare November 26, 2019 08:53

Remove excluded paths

d28afe2

safern marked this pull request as ready for review November 26, 2019 09:00

jashook approved these changes Nov 26, 2019

View reviewed changes

safern added 2 commits November 26, 2019 09:30

Fix download asset errors in run step

0dc03a2

Fix bug in build-job when runtests:true

592dc9d

ViktorHofer reviewed Nov 26, 2019

View reviewed changes

ViktorHofer approved these changes Nov 26, 2019

View reviewed changes

Fix issue where build configurations.props are not present

029c37f

safern force-pushed the LibrariesLiveLiveCi branch from 122db5d to 029c37f Compare November 27, 2019 01:53

safern added 2 commits November 27, 2019 09:42

Fix nano queues syntax

ab2c0cb

Don't split jobs until we make tests Configuration agnostic

4957317

Include missing parameter

f985105

safern merged commit ef33d76 into master Nov 28, 2019

safern deleted the LibrariesLiveLiveCi branch November 28, 2019 10:23

ViktorHofer mentioned this pull request Dec 2, 2019

Add root build definition which builds the entire repo #98

Closed

safern mentioned this pull request Jan 16, 2020

Address PR feedback from old PR #1837

Merged

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

Use platform-matrix and split jobs by steps for libraries pipelines #274

Use platform-matrix and split jobs by steps for libraries pipelines #274

Uh oh!

Conversation

safern commented Nov 26, 2019

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

safern commented Nov 26, 2019

Uh oh!

azure-pipelines bot commented Nov 26, 2019

Uh oh!

jashook left a comment

Choose a reason for hiding this comment

Uh oh!

safern commented Nov 26, 2019

Uh oh!

azure-pipelines bot commented Nov 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern commented Nov 28, 2019

Uh oh!

safern commented Nov 28, 2019

Uh oh!

jaredpar commented Dec 2, 2019

Uh oh!

jaredpar commented Dec 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ViktorHofer Nov 26, 2019 •

edited

Loading

ViktorHofer Nov 26, 2019 •

edited

Loading