brew icon indicating copy to clipboard operation
brew copied to clipboard

cmd/uses: add `--skip-recursive-build-dependents` flag

Open carlocab opened this issue 2 years ago • 10 comments

  • [x] Have you followed the guidelines in our Contributing document?
  • [x] Have you checked to ensure there aren't other open Pull Requests for the same change?
  • [x] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [ ] Have you written new tests for your changes? Here's an example.
  • [x] Have you successfully run brew style with your changes locally?
  • [x] Have you successfully run brew typecheck with your changes locally?
  • [x] Have you successfully run brew tests with your changes locally?

We use brew uses to determine the list of dependent formulae to test in test-bot. By default, we generate this list with the invocation

brew uses --formula --include-build --include-test --recursive <formula>

This results in our testing formulae that do not need to be tested. As an example, suppose foo is a build dependency of bar, and baz depends on bar. Since we don't rebuild bar when testing baz, then no changes in foo are relevant to baz, and any test result from testing baz as a dependent of foo is not meaningful.

One instance where this is particularly wasteful is in go PRs. envoy and [email protected] both declare a build dependency on bazelisk, which in turn declares a build dependency on go. The two formulae also take about four hours each to build.

Let's cut the CI time we spend testing formulae that don't need to be tested by adding a --skip-recursive-dependents flag to brew uses. Adding this flag will omit all dependents of a build dependent from the output.

For reference, here is the list of formulae that are present without the --skip-recursive-build-depenents flag but are no longer there with it:

arduino-cli bat-extras crun envoy [email protected] kube-ps1 kubectx kubernetes-cli kubie modd newrelic-infra-agent [email protected]

carlocab avatar Nov 19 '21 13:11 carlocab

Review period will end on 2021-11-22 at 13:53:28 UTC.

BrewTestBot avatar Nov 19 '21 13:11 BrewTestBot

The implementation in DependenciesHelpers is a bit hacky. Suggestions appreciated.

carlocab avatar Nov 19 '21 13:11 carlocab

One instance where this is particularly wasteful is in go PRs. envoy and [email protected] both declare a build dependency on bazelisk, which in turn declares a build dependency on go. The two formulae also take about four hours each to build.

I'm wondering if we should be running brew uses multiple times and do different things with runtime/build/test dependencies. Thoughts?

MikeMcQuaid avatar Nov 19 '21 14:11 MikeMcQuaid

I'm wondering if we should be running brew uses multiple times and do different things with runtime/build/test dependencies.

Possibly, yes. We don't need to build test dependents from source, for example. I think we might still be doing that.

carlocab avatar Nov 19 '21 14:11 carlocab

This isn't quite correct yet.

❯ brew uses --include-build --include-test --recursive --skip-recursive-build-dependents --formula [email protected]
arduino-cli              kops                     kubectx                  kubie                    modd                     tanka
devspace                 krew                     kubeless                 landscaper               newrelic-infra-agent     [email protected]
helmsman                 kube-ps1                 kubernetes-cli           minikube                 riff

devspace depends on kubernetes-cli (runtime) and go (build). kubernetes-cli depends on [email protected] (build). My intention is for us to not see devspace here, since it is a dependent of a build dependent.

carlocab avatar Nov 19 '21 23:11 carlocab

Better:

❯ brew uses --include-build --include-test --recursive --formula --skip-recursive-build-dependents [email protected]
arduino-cli                   kubernetes-cli                modd                          newrelic-infra-agent          [email protected]

❯ comm -23 <(brew uses --include-build --include-test --recursive --formula go) <(brew uses --include-build --include-test --recursive --formula --skip-recursive-build-dependents go)
arduino-cli
bat-extras
cdktf
charm-tools
conjure-up
crun
envoy
[email protected]
git-annex-remote-rclone
juju-wait
kube-ps1
kubectx
kubernetes-cli
kubie
localstack
modd
navi
newrelic-infra-agent
snapcraft
[email protected]
wireguard-tools

carlocab avatar Nov 20 '21 12:11 carlocab

Bonus: this will cut 510 (!) formulae from a [email protected] CI run.

https://gist.github.com/carlocab/5aa6e3f05e4bad513f1cd11fb619e69e

Will check a few of these to make sure this is the behaviour we want.

carlocab avatar Nov 20 '21 16:11 carlocab

Review period ended.

BrewTestBot avatar Nov 22 '21 15:11 BrewTestBot

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Dec 17 '21 00:12 github-actions[bot]

Reopening after discussion at Homebrew/homebrew-test-bot#837. Conflicts need resolving. Also some discussion here about correctness/desirability.

carlocab avatar Sep 13 '22 08:09 carlocab

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Oct 05 '22 00:10 github-actions[bot]