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

CI: Add support in yml for detecting darc dependency changes in eng/Version.Details.xml #73435

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

radical
Copy link
Member

@radical radical commented Aug 5, 2022

Issue

Currently, whenever a darc flow PR is opened, we cannot trigger different jobs based on which
dependency got updated. This is needed because in some cases, like for ILLinker, Emscripten
updates we want to trigger all the wasm jobs. But that might not be needed for some of the other
dependencies.

  • This causes failures to slip through to main, which gets discovered after the fact as other PR
    failures, or rolling build failures, creating extra work for developers.

Solution

  • This PR identifies the changed dependencies, and emits one azure variable per dependency,
    which can then be used in conditions in the yml .

  • The changed dependency can be checked as dependencies.evaluate_paths.outputs['DarcDependenciesChanged.System_CommandLine']

  • Included this in the evaluate_paths job itself, instead of creating another job

  • Also, use this for wasm jobs

@ghost
Copy link

ghost commented Aug 5, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: radical
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@radical radical changed the title Add support in yml for detecting darc dependency changes in eng/Version.Details.xml [WIP] Add support in yml for detecting darc dependency changes in eng/Version.Details.xml Aug 5, 2022
@radical radical added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Aug 5, 2022
@ghost
Copy link

ghost commented Aug 5, 2022

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: radical
Assignees: radical
Labels:

area-Infrastructure-mono

Milestone: -

@radical
Copy link
Member Author

radical commented Aug 5, 2022

@akoeplinger thoughts? The changes right now include an artificial change to eng/Version.Details.xml.

  1. In Evaluate Paths job, two variables get set, one per changed dependency:
eng/pipelines/evaluate-changed-darc-deps.sh --difftarget HEAD^1
========================== Starting Command Output ===========================
/usr/bin/bash --noprofile --norc /home/vsts/work/_temp/45925021-bbde-4179-bee2-f8f8b3eda8da.sh
Setting pipeline variable Microsoft_NETCore_Runtime_ICU_Transport=true
Setting pipeline variable System_CommandLine=true
Finishing: Evaluate eng/Version.Details.xml for dependency changes
  1. And I tried accessing the variable in WBT job:
  CommandLineChanged1:
    Parsing expression: <dependencies.evaluate_paths.outputs['DarcDependenciesChanged.System_CommandLine']>
    Evaluating: dependencies['evaluate_paths']['outputs']['DarcDependenciesChanged.System_CommandLine']
    Result: 'true'

@radical radical changed the title [WIP] Add support in yml for detecting darc dependency changes in eng/Version.Details.xml CI: Add support in yml for detecting darc dependency changes in eng/Version.Details.xml Aug 6, 2022
@radical radical marked this pull request as ready for review August 6, 2022 00:34
@radical radical added area-Infrastructure and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it area-Infrastructure-mono labels Aug 6, 2022
@ghost
Copy link

ghost commented Aug 6, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Issue

Currently, whenever a darc flow PR is opened, we cannot trigger different jobs based on which
dependency got updated. This is needed because in some cases, like for ILLinker, Emscripten
updates we want to trigger all the wasm jobs. But that might not be needed for some of the other
dependencies.

  • This causes failures to slip through to main, which gets discovered after the fact as other PR
    failures, or rolling build failures, creating extra work for developers.

Solution

  • This PR identifies the changed dependencies, and emits one azure variable per dependency,
    which can then be used in conditions in the yml .

  • The changed dependency can be checked as dependencies.evaluate_paths.outputs['DarcDependenciesChanged.System_CommandLine']

  • Included this in the evaluate_paths job itself, instead of creating another job

Author: radical
Assignees: radical
Labels:

area-Infrastructure

Milestone: -

@ghost
Copy link

ghost commented Aug 6, 2022

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Issue

Currently, whenever a darc flow PR is opened, we cannot trigger different jobs based on which
dependency got updated. This is needed because in some cases, like for ILLinker, Emscripten
updates we want to trigger all the wasm jobs. But that might not be needed for some of the other
dependencies.

  • This causes failures to slip through to main, which gets discovered after the fact as other PR
    failures, or rolling build failures, creating extra work for developers.

Solution

  • This PR identifies the changed dependencies, and emits one azure variable per dependency,
    which can then be used in conditions in the yml .

  • The changed dependency can be checked as dependencies.evaluate_paths.outputs['DarcDependenciesChanged.System_CommandLine']

  • Included this in the evaluate_paths job itself, instead of creating another job

Author: radical
Assignees: radical
Labels:

area-Infrastructure, area-Infrastructure-mono

Milestone: -

@radical radical requested a review from steveisok August 6, 2022 00:36
@radical radical added the arch-wasm WebAssembly architecture label Aug 6, 2022
@ghost
Copy link

ghost commented Aug 6, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Issue

Currently, whenever a darc flow PR is opened, we cannot trigger different jobs based on which
dependency got updated. This is needed because in some cases, like for ILLinker, Emscripten
updates we want to trigger all the wasm jobs. But that might not be needed for some of the other
dependencies.

  • This causes failures to slip through to main, which gets discovered after the fact as other PR
    failures, or rolling build failures, creating extra work for developers.

Solution

  • This PR identifies the changed dependencies, and emits one azure variable per dependency,
    which can then be used in conditions in the yml .

  • The changed dependency can be checked as dependencies.evaluate_paths.outputs['DarcDependenciesChanged.System_CommandLine']

  • Included this in the evaluate_paths job itself, instead of creating another job

Author: radical
Assignees: radical
Labels:

arch-wasm, area-Infrastructure, area-Infrastructure-mono

Milestone: -

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

LGTM but I'd prefer to get someone from the infrastructure team to take a look as well

@@ -34,4 +34,11 @@ variables:
${{ if eq(variables['Build.Reason'], 'PullRequest') }}:
value: Debug

- name: wasmDarcDepsChanged
value: $[ or(
eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_NET_Workload_Emscripten_Manifest-7_0_100'], true),
Copy link
Member

Choose a reason for hiding this comment

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

can we somehow do a string contains check here so we don't need to include the 7_0_100 version number? we'll almost certainly forget to update this here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I'll try

eng/pipelines/common/variables.yml Outdated Show resolved Hide resolved
@radical radical requested a review from safern August 8, 2022 19:06
@radical radical added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 8, 2022
@radical
Copy link
Member Author

radical commented Aug 9, 2022

/azp run runtime, runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@radical
Copy link
Member Author

radical commented Aug 9, 2022

Build windows arm64 Release NativeAOT is #69832 .
Libraries Build windows x64 Debug is #73646 .
Libraries Test Run checked coreclr Linux arm64 Debug is #73647
Libraries Test Run release coreclr OSX x64 Debug failed due to network issue in downloading tests
mono llvmfullaot Pri0 Runtime Tests Run Linux x64 release is #69832 .

And all of these are unrelated to this PR.

@radical radical merged commit 4211dc4 into dotnet:main Aug 9, 2022
@radical radical deleted the darc-deps branch August 9, 2022 19:16
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants