Improve CI path-changes-based-triggers to work better #79127
Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue Details-- WIP --
|
ghost
assigned 
|
reviving an old branch |
radical
force-pushed
the
ci-wasm-triggers-anew
branch
5 times, most recently
from
eb9a802 to
3a64ba2
Compare
radical
force-pushed
the
ci-wasm-triggers-anew
branch
4 times, most recently
from
0f9105f to
32826ce
Compare
radical
force-pushed
the
ci-wasm-triggers-anew
branch
from
32826ce to
5172d90
Compare
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.
…ries, and non_wasm. Add wasm_specific_except_wbt_dbg
|
Reviewing individual commits should be easier. |
radical
changed the title
|
Any objections to this? |
|
I haven't reviewed the actual code yet, but I wonder why there needs to be a new |
True. I was expecting the existing behavior to be like In the past with similar changes, we had situations where triggers for some non-wasm builds got disabled as an unintended side-effect, and I'm not familiar with all the builds to be 100% sure here.
Maybe we can take the extra parameter for now. And the various area owners can gradually move their builds to this, and then eventually we make this the default:) The |
|
A tool could possibly be written that would read the subsets, and check the conditions for all the jobs against that. Check for things like the expected list of jobs being triggered by path changes, and dependent jobs also being triggered etc. |
|
ping @akoeplinger @ViktorHofer @trylek @agocke for review |
|
This shouldn't impact official builds but just to be sure, can we wait with merging this in until next week when we branched-off for P1? Dependency flow is already complicated enough and I believe this change falls under the "general infrastructure improvement" bucket that isn't necessary to merge in for Preview 1. Does that make sense? |
|
Yes, this can wait for the branch. Thank you for taking a look 👍 |
|
ping @akoeplinger @ViktorHofer @trylek @agocke for review |
|
The |
Background:
eng/pipelines/common/evaluate-default-paths.ymlis used to specify named subsets of paths, andevaluate-changed-paths.shdecides 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 combinedProblem
This script currently supports
--include, and--excludeparameters to determine if we have any changed paths.
As described above, these two are evaluated exclusively.
For example:
This would return true if there are changes:
[ '/a/b/c.txt' ]- caught byinclude[ '/r/s.txt' ]- caught byexcludebut it would also return true for
[ '/a/b/d.txt' ]becauseincludedoes not consider the
excludelist.Solution:
This commit adds a new
--combinedparameter which essentially supports$include - $exclude. Thus allowing exact more constrained pathspecification.
It is passed in addition to
include, andexclude.Given:
[ '/a/b/c.txt' ]-true[ '/r/s.txt' ]-falsebecause this does not fall in$include - $exclude[ '/a/b/d.txt' ]-false- excluded in$include - $excludeThe implementation is trivially implemented by passing both the lists to
git diffwhich supports this already.Track subset name changes in the ymls
Update
wasmjobs to have tighter trigger conditionsThis 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.