Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workflows/doctor: use ephemeral runners #14172

Merged
merged 2 commits into from Nov 24, 2022

Conversation

carlocab
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • 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.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This workflow hasn't been set up to run on our ephemeral runners. Let's
fix that.

This workflow hasn't been set up to run on our ephemeral runners. Let's
fix that.
@carlocab carlocab requested a review from Bo98 November 24, 2022 06:17
@BrewTestBot
Copy link
Member

Review period will end on 2022-11-25 at 06:17:22 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 24, 2022
@Bo98 Bo98 added the critical Critical change which should be shipped as soon as possible. label Nov 24, 2022
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Nov 24, 2022
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@Bo98
Copy link
Member

Bo98 commented Nov 24, 2022

I've enabled this repo in the GitHub App settings, so should work now.

.github/workflows/doctor.yml Outdated Show resolved Hide resolved
@carlocab
Copy link
Member Author

carlocab commented Nov 24, 2022

Running on ephemeral is really slow. Our bare metal runners can complete the brew doctor workflow in 12s. The ephemeral runners take between three and seven minutes. 13-arm64 takes three and a half minutes. See https://github.com/Homebrew/brew/actions/runs/3538204547.

I wonder why they're so much slower.

Co-authored-by: Bo Anderson <mail@boanderson.me>
@Bo98
Copy link
Member

Bo98 commented Nov 24, 2022

Some of it will be updating homebrew-core being slow (ephemeral will have an older copy while non-ephemeral will be near up-to-date from the last run) and needing to run brew install-bundler-gems on an older install (which we could cache).

Cleanup slowness on Monterey still needs further investigation.

@MikeMcQuaid
Copy link
Member

Some of it will be updating homebrew-core being slow (ephemeral will have an older copy while non-ephemeral will be near up-to-date from the last run)

Perhaps we could avoid doing this at all for Homebrew/brew usage?

needing to run brew install-bundler-gems on an older install (which we could cache).

actions/cache feels like a good call. also: why do we need these gems for brew doctor?

Cleanup slowness on Monterey still needs further investigation.

Is this pre-cleanup?

@Bo98
Copy link
Member

Bo98 commented Nov 24, 2022

why do we need these gems for brew doctor?

We don't really but it's a fixed part of --only-setup.

Is this pre-cleanup?

Yes (--only-cleanup-before). We could skip it, but I'm a bit concerned something that should be doing little to nothing is taking 3 minutes so this feels like something's wrong that's indicating something to fix in the base image.

@MikeMcQuaid
Copy link
Member

We don't really but it's a fixed part of --only-setup.

Gotcha. Could definitely remove or move to another --only where these gems are actually needed.

We could skip it, but I'm a bit concerned something that should be doing little to nothing is taking 3 minutes so this feels like something's wrong that's indicating something to fix in the base image.

Agreed 👍🏻.

IMO we should do the bare minimum to only cleanup the stuff brew doctor complains about that. Could experiment with faster ways of doing this e.g. renaming /usr/local/include to /usr/local/include.old instead of moving to /tmp.

@carlocab
Copy link
Member Author

We don't really but it's a fixed part of --only-setup.

Gotcha. Could definitely remove or move to another --only where these gems are actually needed.

How much of the setup do we need? (Or the cleanup steps, for that matter?) I'm wondering if we can cut those all out and run setup-homebrew and then call brew doctor.

@Bo98
Copy link
Member

Bo98 commented Nov 24, 2022

IMO we should do the bare minimum to only cleanup the stuff brew doctor complains about that. Could experiment with faster ways of doing this e.g. renaming /usr/local/include to /usr/local/include.old instead of moving to /tmp.

The self-hosted images should already be set up that the cleanup required is zero.

How much of the setup do we need?

Should be zero I think.

@carlocab
Copy link
Member Author

So, maybe something like this then?

diff --git a/.github/workflows/doctor.yml b/.github/workflows/doctor.yml
index 50409557e..0335cda25 100644
--- a/.github/workflows/doctor.yml
+++ b/.github/workflows/doctor.yml
@@ -32,8 +32,9 @@ jobs:
         uses: Homebrew/actions/setup-homebrew@master
 
       - run: brew test-bot --only-cleanup-before
+        if: ! contains(matrix.runner, github.run_id)
 
-      - run: brew test-bot --only-setup
+      - run: brew doctor
 
       - run: brew test-bot --only-cleanup-after
-        if: always()
+        if: always() && ! contains(matrix.runner, github.run_id)

Not sure if some of those should be wrapped in ${{ }}. I assume not.

@carlocab carlocab merged commit 9c88c39 into Homebrew:master Nov 24, 2022
@carlocab carlocab deleted the doctor-runners branch November 24, 2022 14:41
carlocab added a commit to carlocab/brew that referenced this pull request Nov 24, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 25, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants