From 3b48453d48bd7c8f9f761ce6d480196fb0a32f1e Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Tue, 7 Feb 2023 15:49:22 -0500 Subject: [PATCH] Improve CI path-changes-based-triggers to work better (#79127) ## Background: `eng/pipelines/common/evaluate-default-paths.yml` is used to specify named subsets of paths, and `evaluate-changed-paths.sh` decides which subsets have "changed files". And these subsets are used in conditions for the various jobs to determine when they should be run. --- `evaluate-changed-paths.sh: Add support for include+exclude combined` ## Problem This script currently supports `--include`, and `--exclude` parameters to determine if we have any changed paths. ``` Scenarios: 1. exclude paths are specified Will include all paths except the ones in the exclude list. 2. include paths are specified Will only include paths specified in the list. 1st we evaluate changes for all paths except ones in excluded list. If we can not find any applicable changes like that, then we evaluate changes for included paths if any of these two finds changes, then a variable will be set to true. ``` As described above, these two are evaluated exclusively. For example: ``` include: [ '/a/b/*' ], exclude: [ '/a/b/d.txt' ] ``` - This would return true if there are changes: - `[ '/a/b/c.txt' ]` - caught by `include` - or `[ '/r/s.txt' ]` - caught by `exclude` - but it would also return true for `[ '/a/b/d.txt' ]` because `include` does not consider the `exclude` list. ## Solution: - This commit adds a new `--combined` parameter which essentially supports `$include - $exclude`. Thus allowing exact more constrained path specification. - It is passed in addition to `include`, and `exclude`. Given: ``` include: [ '/a/b/*' ], exclude: [ '/a/b/d.txt' ] ``` - For the following path changes: - `[ '/a/b/c.txt' ]` - `true` - `[ '/r/s.txt' ]` - `false` because this does not fall in `$include - $exclude` - `[ '/a/b/d.txt' ]` - `false` - excluded in `$include - $exclude` The implementation is trivially implemented by passing both the lists to `git diff` which supports this already. --- - Track subset name changes in the ymls - Update `wasm` jobs to have tighter trigger conditions - This results in wasm specific changes only triggering relevant wasm jobs. For example, if there are wasm debugger changes then only the wasm debugger jobs will be triggered. Or if there are only workload manifest[1] changes then Wasm.Build.Tests will be triggered only. --- eng/pipelines/common/evaluate-default-paths.yml | 117 ++++++++++----------- eng/pipelines/common/evaluate-paths-job.yml | 2 + eng/pipelines/common/templates/wasm-build-only.yml | 6 +- .../common/templates/wasm-build-tests.yml | 5 - .../common/templates/wasm-debugger-tests.yml | 1 + .../common/templates/wasm-library-tests.yml | 21 ++-- .../common/templates/wasm-runtime-tests.yml | 9 +- eng/pipelines/evaluate-changed-paths.sh | 48 +++++++-- eng/pipelines/global-build.yml | 2 + eng/pipelines/runtime-linker-tests.yml | 4 +- 10 files changed, 108 insertions(+), 107 deletions(-) diff --git a/eng/pipelines/common/evaluate-default-paths.yml b/eng/pipelines/common/evaluate-default-paths.yml index dc01caf..fd0697b 100644 --- a/eng/pipelines/common/evaluate-default-paths.yml +++ b/eng/pipelines/common/evaluate-default-paths.yml @@ -10,8 +10,11 @@ parameters: eng/testing/scenarios/BuildWasmAppsJobsList.txt eng/testing/tests.wasm.targets src/libraries/sendtohelix-wasm.targets + src/mono/mono/**/*wasm* src/mono/nuget/Microsoft.NET.Runtime.WebAssembly.Sdk/* src/mono/nuget/Microsoft.NET.Runtime.wasm.Sample.Mono/* + src/mono/nuget/Microsoft.NETCore.BrowserDebugHost.Transport/* + src/mono/nuget/Microsoft.NET.Workload* src/mono/sample/wasm/* src/mono/wasi/* src/mono/wasm/* @@ -23,10 +26,25 @@ parameters: _wasm_pipelines: [ eng/pipelines/**/*wasm* ] + _wasm_src_native: [ + src/native/minipal/* + src/native/libs/CMakeLists.txt + src/native/libs/configure.cmake + src/native/libs/build* + src/native/libs/Common/* + src/native/libs/System.Globalization.Native/* + src/native/libs/System.IO.Compression.Native/* + src/native/libs/System.Native/* + ] + # src/workloads is only used in runtime-official builds # where evaluate-paths is not used _always_exclude: [ eng/pipelines/common/evaluate-default-paths.yml + '*.md' + LICENSE.TXT + PATENTS.TXT + THIRD-PARTY-NOTICES.TXT src/workloads/* ] @@ -42,10 +60,6 @@ jobs: - src/native/libs/System.IO.Compression.Native/* exclude: - eng/Version.Details.xml - - '*.md' - - LICENSE.TXT - - PATENTS.TXT - - THIRD-PARTY-NOTICES.TXT - docs/* - src/installer/* - src/mono/* @@ -71,10 +85,6 @@ jobs: - ${{ parameters._const_paths._always_exclude }} - eng/Version.Details.xml - - '*.md' - - LICENSE.TXT - - PATENTS.TXT - - THIRD-PARTY-NOTICES.TXT - docs/* - src/installer/* - src/coreclr/* @@ -89,10 +99,6 @@ jobs: - subset: libraries exclude: - eng/Version.Details.xml - - '*.md' - - LICENSE.TXT - - PATENTS.TXT - - THIRD-PARTY-NOTICES.TXT - docs/* - src/installer/* - src/mono/* @@ -114,21 +120,11 @@ jobs: include: - src/tools/illink/* - - subset: non_runtimetests - exclude: - - src/tests/* - - ${{ parameters._const_paths._wasm_pipelines }} - - ${{ parameters._const_paths._always_exclude }} - - subset: installer include: - docs/manpages/* exclude: - eng/Version.Details.xml - - '*.md' - - LICENSE.TXT - - PATENTS.TXT - - THIRD-PARTY-NOTICES.TXT - docs/* - src/coreclr/* - src/mono/* @@ -139,6 +135,7 @@ jobs: - eng/pipelines/coreclr/* - eng/pipelines/mono/* - eng/pipelines/libraries/* + - ${{ parameters._const_paths._wasm_specific_only }} - ${{ parameters._const_paths._wasm_pipelines }} - ${{ parameters._const_paths._always_exclude }} @@ -161,10 +158,11 @@ jobs: # # ** WASM ** - # Changes in *only* Wasm.Build.Tests, or debugger, are very self-contained, - # so we try to avoid triggering only those relevants tests + # Changes in *only* Wasm.Build.Tests, debugger, or runtime-tests are very + # self-contained, so we try to trigger only those relevants tests # - subset: wasmbuildtests + combined: true include: - eng/Version.Details.xml - eng/Versions.props @@ -181,9 +179,7 @@ jobs: - src/mono/nuget/Microsoft.NET.Runtime.MonoTargets.Sdk/* - src/mono/nuget/Microsoft.NET.Runtime.WebAssembly.Sdk/* - src/mono/nuget/Microsoft.NET.Runtime.wasm.Sample.Mono/* - - src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.Current.Manifest/* - - src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.net6.Manifest/* - - src/mono/nuget/Microsoft.NET.Workload.Mono.Toolchain.net7.Manifest/* + - src/mono/nuget/Microsoft.NET.Workload* - src/mono/nuget/Microsoft.NETCore.BrowserDebugHost.Transport/* - src/mono/wasm/build/* - src/mono/wasm/emscripten-version.txt @@ -191,10 +187,14 @@ jobs: - src/mono/wasm/runtime/* - src/mono/wasm/templates/* - src/mono/wasm/Wasm.Build.Tests/* + - ${{ parameters._const_paths._wasm_src_native }} - src/tasks/* - ${{ parameters._const_paths._wasm_pipelines }} + exclude: + - ${{ parameters._const_paths._always_exclude }} - subset: wasmdebuggertests + combined: true include: - eng/testing/ProvisioningVersions.props - src/libraries/System.Runtime.InteropServices/* @@ -202,53 +202,47 @@ jobs: - src/mono/mono/* - src/mono/wasm/debugger/* - src/mono/wasm/runtime/* + - ${{ parameters._const_paths._wasm_src_native }} - ${{ parameters._const_paths._wasm_pipelines }} - - - subset: any_other_than_wasm_wbt_dbg exclude: - - src/mono/wasm/Wasm.Build.Tests/* - - src/mono/wasm/debugger/* - - ${{ parameters._const_paths._wasm_pipelines }} + - src/mono/nuget/* - ${{ parameters._const_paths._always_exclude }} - - subset: wasm_wbt_or_dbg + # wasm/runtimetests need to be run + - subset: wasm_runtimetests + combined: true include: - - src/mono/wasm/Wasm.Build.Tests/* + - src/tests/* + - src/mono/* + - ${{ parameters._const_paths._wasm_src_native }} + exclude: + - src/mono/nuget/* + - src/mono/sample/* + - src/mono/tests/* + - src/mono/tools/* + - src/mono/wasi/* - src/mono/wasm/debugger/* + - src/mono/wasm/Wasm.Build.Tests/* + - ${{ parameters._const_paths._wasm_pipelines }} + - ${{ parameters._const_paths._always_exclude }} - - subset: wasm + # Wasm except Wasm.build.Tests, and debugger + - subset: wasm_specific_except_wbt_dbg + combined: true include: - ${{ parameters._const_paths._wasm_specific_only }} - ${{ parameters._const_paths._wasm_pipelines }} # other paths that should also trigger wasm jobs - src/mono/* - - # libraries with some wasm specific code - - subset: wasm_libraries - include: - - src/libraries/Common/* - - src/libraries/System.Console/* - - src/libraries/System.Diagnostics.FileVersionInfo/tests/* - - src/libraries/System.IO.Compression/* - - src/libraries/System.IO.MemoryMappedFiles/* - - src/libraries/System.Net.Http/* - - src/libraries/System.Net.Mail/* - - src/libraries/System.Net.Primitives/* - - src/libraries/System.Net.WebClient/* - - src/libraries/System.Net.WebProxy/* - - src/libraries/System.Net.WebSockets.Client/* - - src/libraries/System.Net.WebSockets/* - - src/libraries/System.Runtime.InteropServices.JavaScript/* - - src/libraries/System.Runtime.InteropServices/* - - src/libraries/System.Runtime.Serialization.Formatters/* - - src/libraries/System.Security.Cryptography/* - - src/libraries/System.Text.Encodings.Web/* - - # anything other than wasm-specific paths - - subset: non_wasm exclude: - - ${{ parameters._const_paths._wasm_specific_only }} - - ${{ parameters._const_paths._wasm_pipelines }} + - eng/testing/scenarios/BuildWasmAppsJobsList.txt + - eng/testing/workloads-testing.targets + - src/mono/mono/component/mini-wasm-debugger.c + - src/mono/wasm/debugger/* + - src/mono/wasm/Wasm.Build.Tests/* + - src/mono/nuget/Microsoft.NET.Runtime* + - src/mono/nuget/Microsoft.NET.Workload* + - src/mono/nuget/Microsoft.NETCore.BrowserDebugHost.Transport/* - ${{ parameters._const_paths._always_exclude }} # anything other than mono, or wasm specific paths @@ -258,7 +252,6 @@ jobs: - ${{ parameters._const_paths._wasm_specific_only }} - ${{ parameters._const_paths._wasm_pipelines }} - ${{ parameters._const_paths._always_exclude }} - - eng/testing/tests.mobile.targets - src/mono/* - src/tasks/AndroidAppBuilder/* diff --git a/eng/pipelines/common/evaluate-paths-job.yml b/eng/pipelines/common/evaluate-paths-job.yml index 670fba6..64c3a12 100644 --- a/eng/pipelines/common/evaluate-paths-job.yml +++ b/eng/pipelines/common/evaluate-paths-job.yml @@ -41,6 +41,8 @@ jobs: parameters: subsetName: ${{ path.subset }} arguments: + - ${{ if eq(path.combined, true) }}: + - --combined # The commit that we're building is always a merge commit that is merging into the target branch. # So the first parent of the commit is on the target branch and the second parent is on the source branch. - --difftarget HEAD^1 diff --git a/eng/pipelines/common/templates/wasm-build-only.yml b/eng/pipelines/common/templates/wasm-build-only.yml index b25cf85..f97e0ef 100644 --- a/eng/pipelines/common/templates/wasm-build-only.yml +++ b/eng/pipelines/common/templates/wasm-build-only.yml @@ -26,11 +26,7 @@ jobs: or( eq(variables['wasmDarcDependenciesChanged'], true), eq(dependencies.evaluate_paths.outputs['SetPathVars_libraries.containsChange'], true), - and( - eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm.containsChange'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_non_runtimetests.containsChange'], true)), - eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm_libraries.containsChange'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_mono_excluding_wasm.containsChange'], true)) + eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm_specific_except_wbt_dbg.containsChange'], true)) ] jobParameters: isExtraPlatforms: ${{ parameters.isExtraPlatformsBuild }} diff --git a/eng/pipelines/common/templates/wasm-build-tests.yml b/eng/pipelines/common/templates/wasm-build-tests.yml index 3ade2d7..ff35e11 100644 --- a/eng/pipelines/common/templates/wasm-build-tests.yml +++ b/eng/pipelines/common/templates/wasm-build-tests.yml @@ -34,11 +34,6 @@ jobs: nameSuffix: WasmBuildTests buildArgs: -s mono+libs+host+packs+libs.tests -c $(_BuildConfig) /p:ArchiveTests=true /p:TestWasmBuildTests=true /p:TestAssemblies=false /p:BrowserHost=$(_hostedOs) timeoutInMinutes: 180 - # if !alwaysRun, then: - # if this is runtime-wasm (isWasmOnlyBuild): - # - then run only if it would not have run on default pipelines (based - # on path changes) - # - else run based on path changes condition: >- or( eq(variables['alwaysRunVar'], true), diff --git a/eng/pipelines/common/templates/wasm-debugger-tests.yml b/eng/pipelines/common/templates/wasm-debugger-tests.yml index ff3b33f..994b169 100644 --- a/eng/pipelines/common/templates/wasm-debugger-tests.yml +++ b/eng/pipelines/common/templates/wasm-debugger-tests.yml @@ -27,6 +27,7 @@ jobs: value: $[ or( eq(variables['wasmDarcDependenciesChanged'], true), + eq(dependencies.evaluate_paths_outputs['DarcDependenciesChanged.Microsoft_DotNet_HotReload_Utils_Generator_BuildTool'], true), eq(dependencies.evaluate_paths.outputs['SetPathVars_wasmdebuggertests.containsChange'], true)) ] jobParameters: diff --git a/eng/pipelines/common/templates/wasm-library-tests.yml b/eng/pipelines/common/templates/wasm-library-tests.yml index 380a217..adfc43e 100644 --- a/eng/pipelines/common/templates/wasm-library-tests.yml +++ b/eng/pipelines/common/templates/wasm-library-tests.yml @@ -27,25 +27,16 @@ jobs: # map dependencies variables to local variables - name: alwaysRunVar value: ${{ parameters.alwaysRun }} + # - wasm darc deps changed + # - any libs that can have wasm specific changes + # - any other wasm specific changes that are not wbt, or dbg - name: shouldRunOnDefaultPipelines value: $[ or( eq(variables['wasmDarcDependenciesChanged'], true), - and( - ne(variables['onlyWBTOrDbgTestHaveChanges'], true), - or( - eq(dependencies.evaluate_paths.outputs['SetPathVars_libraries.containsChange'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_mono_excluding_wasm.containsChange'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm_libraries.containsChange'], true), - and( - eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm.containsChange'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_non_runtimetests.containsChange'], true))))) - ] - - name: onlyWBTOrDbgTestHaveChanges - value: - and( - eq(dependencies.evaluate_paths.outputs.SetPathVars_wasm_wbt_or_dbg.containsChange, true), - ne(dependencies.evaluate_paths.outputs.SetPathVars_any_other_than_wasm_wbt_dbg.containsChange, true)) + eq(dependencies.evaluate_paths.outputs['SetPathVars_libraries.containsChange'], true), + eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm_specific_except_wbt_dbg.containsChange'], true)) + ] - name: _wasmRunSmokeTestsOnlyArg value: /p:RunSmokeTestsOnly=${{ eq(parameters.shouldRunSmokeOnly, true) }} - name: chromeInstallArg diff --git a/eng/pipelines/common/templates/wasm-runtime-tests.yml b/eng/pipelines/common/templates/wasm-runtime-tests.yml index 7a9aa31..2808afd 100644 --- a/eng/pipelines/common/templates/wasm-runtime-tests.yml +++ b/eng/pipelines/common/templates/wasm-runtime-tests.yml @@ -27,8 +27,7 @@ jobs: value: $[ or( eq(variables['wasmDarcDependenciesChanged'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_runtimetests.containsChange'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm.containsChange'], true)) + eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm_runtimetests.containsChange'], true)) ] jobParameters: testGroup: innerloop @@ -37,12 +36,6 @@ jobs: runtimeVariant: monointerpreter buildArgs: -s mono+libs -c $(_BuildConfig) timeoutInMinutes: 180 - # FIXME: will get triggered by only wbt/dbg changes - # if !alwaysRun, then: - # if this is runtime-wasm (isWasmOnlyBuild): - # - then run only if it would not have run on default pipelines (based - # on path changes) - # - else run based on path changes condition: >- or( eq(variables['alwaysRunVar'], true), diff --git a/eng/pipelines/evaluate-changed-paths.sh b/eng/pipelines/evaluate-changed-paths.sh index b145f39..37b9e5a 100755 --- a/eng/pipelines/evaluate-changed-paths.sh +++ b/eng/pipelines/evaluate-changed-paths.sh @@ -6,6 +6,10 @@ Scenarios: 2. include paths are specified Will only include paths specified in the list. 3. exclude + include: + a. `--combined` is passed: + Will use the include paths, then use exclude paths on that set, and if that has any changes + then a variable will be set to true. + b. `--combined` is not passed (default): 1st we evaluate changes for all paths except ones in excluded list. If we can not find any applicable changes like that, then we evaluate changes for included paths if any of these two finds changes, then a variable will be set to true. @@ -33,6 +37,7 @@ usage() echo " --difftarget SHA or branch to diff against. (i.e: HEAD^1, origin/main, 0f4hd36, etc.)" echo " --excludepaths Escaped list of paths to exclude from diff separated by '+'. (i.e: 'src/libraries/*+'src/installer/*')" echo " --includepaths Escaped list of paths to include on diff separated by '+'. (i.e: 'src/libraries/System.Private.CoreLib/*')" + echo " --combined Uses both exclude, and include paths" echo " --subset Subset name for which we're evaluating in order to include it in logs" echo " --azurevariable Name of azure devops variable to create if change meets filter criteria" echo "" @@ -59,6 +64,7 @@ include_paths=() subset_name='' azure_variable='' diff_target='' +combined=false while [[ $# > 0 ]]; do opt="$(echo "${1/#--/-}" | tr "[:upper:]" "[:lower:]")" @@ -81,6 +87,9 @@ while [[ $# > 0 ]]; do include_paths+=(${tmp[@]}) shift ;; + -combined) + combined=true + ;; -subset) subset_name=$2 shift @@ -138,7 +147,7 @@ probePaths() { if [[ ${#exclude_paths[@]} -gt 0 ]]; then echo "" - echo "******* Probing $_subset exclude paths *******"; + echo "******* Collecting $_subset exclude paths *******"; for _path in "${exclude_paths[@]}"; do echo "$_path" if [[ -z "$exclude_path_string" ]]; then @@ -147,16 +156,11 @@ probePaths() { exclude_path_string="$exclude_path_string :!$_path" fi done - - if ! probePathsWithExitCode $exclude_path_string; then - found_applying_changes=true - printMatchedPaths $exclude_path_string - fi fi - if [[ $found_applying_changes != true && ${#include_paths[@]} -gt 0 ]]; then + if [[ ${#include_paths[@]} -gt 0 ]]; then echo "" - echo "******* Probing $_subset include paths *******"; + echo "******* Collecting $_subset include paths *******"; for _path in "${include_paths[@]}"; do echo "$_path" if [[ -z "$include_path_string" ]]; then @@ -165,10 +169,34 @@ probePaths() { include_path_string="$include_path_string :$_path" fi done + fi - if ! probePathsWithExitCode $include_path_string; then + if [[ "$combined" == "true" ]]; then + # Try both include, and exclude + # finds all the changes in include files, then excludes the exclude files + local combined_path_string="$include_path_string $exclude_path_string" + echo "******* Probing $_subset combined paths *******"; + if ! probePathsWithExitCode $combined_path_string; then found_applying_changes=true - printMatchedPaths $include_path_string + printMatchedPaths $combined_path_string + fi + else + # First try exclude + if [[ ${#exclude_paths[@]} -gt 0 ]]; then + echo "******* Probing $_subset exclude paths *******"; + if ! probePathsWithExitCode $exclude_path_string; then + found_applying_changes=true + printMatchedPaths $exclude_path_string + fi + fi + + # if no changes found, then try include + if [[ $found_applying_changes != true && ${#include_paths[@]} -gt 0 ]]; then + echo "******* Probing $_subset include paths *******"; + if ! probePathsWithExitCode $include_path_string; then + found_applying_changes=true + printMatchedPaths $include_path_string + fi fi fi diff --git a/eng/pipelines/global-build.yml b/eng/pipelines/global-build.yml index 1e47945..a13286c 100644 --- a/eng/pipelines/global-build.yml +++ b/eng/pipelines/global-build.yml @@ -176,3 +176,5 @@ extends: extraStepsParameters: name: SourceBuildPackages timeoutInMinutes: 95 + condition: + eq(dependencies.evaluate_paths.outputs['SetPathVars_non_mono_and_wasm.containsChange'], true) diff --git a/eng/pipelines/runtime-linker-tests.yml b/eng/pipelines/runtime-linker-tests.yml index 698d0f8..3b965ac 100644 --- a/eng/pipelines/runtime-linker-tests.yml +++ b/eng/pipelines/runtime-linker-tests.yml @@ -121,8 +121,8 @@ extends: condition: or( eq(variables['isRollingBuild'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm_libraries.containsChange'], true), - eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm.containsChange'], true), + eq(dependencies.evaluate_paths.outputs['SetPathVars_libraries.containsChange'], true), + eq(dependencies.evaluate_paths.outputs['SetPathVars_wasm_specific_except_wbt_dbg.containsChange'], true), eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_NET_ILLink_Tasks'], true)) extraStepsTemplate: /eng/pipelines/libraries/execute-trimming-tests-steps.yml extraStepsParameters: -- 2.7.4