arcade-services icon indicating copy to clipboard operation
arcade-services copied to clipboard

Move darc dependency version properties into a separate file

Open radical opened this issue 2 years ago • 8 comments

Scenario: We want to trigger specific jobs on CI based on which dependencies got updated in a flow PR.

Currently, a darc dependency update PR updates eng/Version.Details.xml, and eng/Versions.props. It is easy to determine (https://github.com/dotnet/runtime/blob/main/eng/pipelines/evaluate-changed-darc-deps.sh) which dependencies were updated in eng/Version.Details.xml. But since eng/Versions.props has other properties too, it isn't straight forward to determine the same.

For example:

  1. a flow PR has dotnet/emsdk update in dotnet/runtime
  2. we want to trigger only the wasm jobs based on this

This could be done via a script like "check Versions.props, all the changes are only property names corresponding to darc dependencies", but IMHO, it would be cleaner if the version properties were in a separate file that can then be imported by eng/Versions.props.

Another alternative would be to move all the non-dependency version properties out of eng/Versions.props, but that's not the pattern followed in the various repos.

cc @lewing @steveisok @akoeplinger

radical avatar Sep 20 '23 18:09 radical

cc @ViktorHofer

radical avatar Sep 20 '23 23:09 radical

@tkapin @premun this looks like work for ProdCon?

missymessa avatar Sep 21 '23 19:09 missymessa

I think @mmitche would have a better understanding as to what is the best way to handle this use-case

oleksandr-didyk avatar Sep 22 '23 08:09 oleksandr-didyk

@radical I'm not sure AzDO triggers offer enough flexibility here. This might require significant investment. What's the business justification for this besides running fewer / shorter CI jobs?

tkapin avatar Sep 22 '23 15:09 tkapin

@radical I'm not sure AzDO triggers offer enough flexibility here. This might require significant investment. What's the business justification for this besides running fewer / shorter CI jobs?

Special AzDO trigger features shouldn't be needed for this. Essentially, in dotnet/runtime:

  1. we have a Evaluate paths job that runs, which builds a list of files that changed in the PR (https://github.com/dotnet/runtime/blob/main/eng/pipelines/evaluate-changed-paths.sh)
  2. Then we set some properties based on that, which we can then use in conditions in the yml (https://github.com/dotnet/runtime/blob/main/eng/pipelines/common/evaluate-default-paths.yml#L94-L115 https://github.com/dotnet/runtime/blob/main/eng/pipelines/common/evaluate-paths-job.yml https://github.com/dotnet/runtime/blob/b3ef4981b273a884ae1c1057384b2305f5b51f2b/eng/pipelines/common/xplat-setup.yml#L121-L127)
        - name: shouldRunWasmBuildTestsOnDefaultPipeline
          value: $[
            or(
              eq(dependencies.evaluate_paths.outputs['SetPathVars_tools_illink.containsChange'], true),
              eq(dependencies.evaluate_paths.outputs['SetPathVars_wasmbuildtests.containsChange'], true))
            ]

The idea being to control triggering jobs based on what files had changes. For example, if the PR only has changes in src/mono/wasm, then we only trigger the wasm jobs, and skip the rest. And this works well for most cases.

One case where we want more granularity is darc version updates - to trigger jobs based on which dependencies are being updated. A darc flow affects two files:

  • eng/Version.Details.xml
    • this is a xml file with only darc dependencies, so it is easy to identify which dependency changed (https://github.com/dotnet/runtime/blob/main/eng/pipelines/evaluate-changed-darc-deps.sh), allowing conditions like https://github.com/dotnet/runtime/blob/b3ef4981b273a884ae1c1057384b2305f5b51f2b/eng/pipelines/common/xplat-setup.yml#L109-L119.
        - name: wasmDarcDependenciesChanged
          value: $[ or(
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_NET_Workload_Emscripten_Current_Manifest-8_0_100_Transport'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_DotNet_Build_Tasks_Workloads'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.System_Runtime_TimeZoneData'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_Net_Compilers_Toolset'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_CodeAnalysis'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_CodeAnalysis_CSharp'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_CodeAnalysis_Analyzers'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_CodeAnalysis_NetAnalyzers'], true),
                      eq(dependencies.evaluate_paths.outputs['DarcDependenciesChanged.Microsoft_NET_ILLink_Tasks'], true)) ]
  • eng/Versions.props
    • This one is tricky because the style followed in various repos, including dotnet/runtime, is to have other non-darc-dependency-version properties in the same file. This makes it harder to determine if there are any non-darc-dependency changes in this file.
    • If the properties were also in a separate file with only the darc dependency version properties, then it would become trivial.

So, this shouldn't need any special features, just a change in what file the tool updates.

cc @SamMonoRT @lewing

radical avatar Sep 22 '23 23:09 radical

Thanks for detailed elaboration on how to do this. My team is on full capacity now though, can you also expand on the business justification? As said, I understand this would shorten the CI times, but is there anything else that would justify prioritizing this?

tkapin avatar Sep 25 '23 17:09 tkapin

can you also expand on the business justification? As said, I understand this would shorten the CI times, but is there anything else that would justify prioritizing this?

Basically what you said - reducing CI load, and making PR runs quicker.

radical avatar Sep 25 '23 17:09 radical

Thanks. This seems like a useful thing to do, but as I said, there's no capacity to look at this at the moment. We'll keep this on radar though.

tkapin avatar Sep 25 '23 17:09 tkapin

This is now supported via the Directory.Packages.props

premun avatar Oct 30 '25 13:10 premun