Skip to content

Conversation

anonymous-akorn
Copy link
Contributor

@anonymous-akorn anonymous-akorn commented Dec 2, 2020

We're experiencing queuing on GHA from trying to use too many machine resources. A disproportionate chunk of this is coming from Windows and its excessive build permutations.

Android on Windows has been broken for a while - temporarily removed until fixed.

For desktop, remove 6 of the permutations, leaving the two required ones: Release-x64-static and Debug-x86-dynamic.

We're experiencing queuing on GHA by trying to use too many machine resources. A disproportionate chunk of this is coming from Windows and its excessive build permutations.

Android on Windows has been broken for a while - temporarily remove until fixed.

For desktop, remove 7 of the permutations, leaving just one.
These are the two required checks on pull requests. Build these two instead of just the one specified in the prior change.
@vimanyu
Copy link
Contributor

vimanyu commented Dec 2, 2020

This makes sense but I was wondering if there is any merit in also preserving the exhaustive list for "non-PR" cases? With some tricks, we could maintain the same list of configurations in desktop.yml and modify the matrix dependent on if its a PR event or not. This way, we trim the configurations for PR (or any other events) and also have an exhaustive list of configurations just incase someone wants to run them all.

Pros:

  • no need to maintain a separate workflow for exhaustive list which could be useful to catch certain edge cases (for example, unit tests erroring out in Debug and not Release because of ASSERT macros).
  • no need to modify the configurations in feature development branch (honestly not that big a deal because I do modify the workflow anyway to add a "push" trigger for my branch testing but this means adding more things).

Cons:

  • makes the workflow more complicated and harder to follow
  • no clean way of doing dynamic matrix in github actions

It probably makes more sense to keep it simple for now maintaining a trimmed list and let developers add things if they need to.

@anonymous-akorn
Copy link
Contributor Author

This makes sense but I was wondering if there is any merit in also preserving the exhaustive list for "non-PR" cases? With some tricks, we could maintain the same list of configurations in desktop.yml and modify the matrix dependent on if its a PR event or not. This way, we trim the configurations for PR (or any other events) and also have an exhaustive list of configurations just incase someone wants to run them all.

Pros:

  • no need to maintain a separate workflow for exhaustive list which could be useful to catch certain edge cases (for example, unit tests erroring out in Debug and not Release because of ASSERT macros).
  • no need to modify the configurations in feature development branch (honestly not that big a deal because I do modify the workflow anyway to add a "push" trigger for my branch testing but this means adding more things).

Cons:

  • makes the workflow more complicated and harder to follow
  • no clean way of doing dynamic matrix in github actions

It probably makes more sense to keep it simple for now maintaining a trimmed list and let developers add things if they need to.

I do think we should avoid duplicating the workflow just to have different build parameters. The matrix can be populated dynamically from arbitrary JSON, which means one way to switch between matrices with complex differences is to store distinct matrices as JSON and have a simple switch in a job that runs before the rest. I did something not too dissimilar, albeit in a smaller way, in the integration test workflow to modify input parameters before storing them as matrix parameters.

On the flip side, I wonder how much value the other permutations really give us here. The two cases in this PR hit every build parameter, just not in every permutation. Do we know of concrete cases where a change could work in these two cases, but not in one of the other 6 permutations?

Regardless, my opinion is that we should submit this change as-is to alleviate the queueing. If we want to restore the ability to hit the other permutations in a non-PR capacity, that should be addressed in a separate PR as the change will be non-trivial.

@vimanyu
Copy link
Contributor

vimanyu commented Dec 4, 2020

I agree. Lets push this out for now and we can add the dropped permutations later if we need to.

vcpkg_triplet -> vcpkg_triplet_suffix

Also add comments to include/exclude statements to easier understand their effects on the matrix.
@anonymous-akorn anonymous-akorn merged commit 60d5aab into dev Dec 4, 2020
@anonymous-akorn anonymous-akorn deleted the feature/aks-prune-tests branch December 4, 2020 18:17
@firebase firebase locked and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants