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/tests: use the container and macos-14 for some jobs #18395

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

ZhongRuoyu
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?

The containers have homebrew/core tapped so we don't need to wait forever for the core repo to be cloned.

The tap syntax job requires all official taps. Unlike the Ubuntu images 1, macos-14 has both core and cask tapped 2 so setting up Homebrew should be faster there. We don't use the containers because the cask tap is not available by default.

Footnotes

  1. https://github.com/actions/runner-images/blob/ubuntu24/20240922.1/images/ubuntu/scripts/build/install-homebrew.sh

  2. https://github.com/actions/runner-images/blob/macos-14/20240923.101/images/macos/scripts/build/install-homebrew.sh#L23-L24

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @ZhongRuoyu! Makes sense to me. Have suggested tweaks to defaults for a few containers where we don't explicitly mention the version to avoid needing updates in future and potentially catch issues quicker on the default container.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@ZhongRuoyu
Copy link
Member Author

We don't have homebrew/brew:master though, only homebrew/ubuntu22.04 for master:

elif [[ "${GITHUB_EVENT_NAME}" == "push" &&
"${GITHUB_REF}" == "refs/heads/master" &&
"${{ matrix.version }}" == "22.04" ]]; then
tags+=(
"ghcr.io/homebrew/ubuntu${{ matrix.version }}:master"
"homebrew/ubuntu${{matrix.version}}:master"
)
fi

Perhaps we can publish homebrew/brew:master too?

@MikeMcQuaid
Copy link
Member

We don't have homebrew/brew:master though, only homebrew/ubuntu22.04 for master:

Ooo, my bad.

Perhaps we can publish homebrew/brew:master too?

Yeh, I think it'd be ideal to have a master Docker build of some sort that can be tracked without caring about the underlying Ubuntu version?

That doesn't need to block this PR, though!

ZhongRuoyu added a commit that referenced this pull request Sep 24, 2024
When we don't care about which specific Ubuntu version to use,
`homebrew/brew:master` is better than `homebrew/ubuntu22.04:master` as
it allows us to implicitly rely on the default Ubuntu version.

See discussion at #18395.
@ZhongRuoyu
Copy link
Member Author

Yeh, I think it'd be ideal to have a master Docker build of some sort that can be tracked without caring about the underlying Ubuntu version?

Agreed. Here we go: #18396

@ZhongRuoyu
Copy link
Member Author

ZhongRuoyu commented Sep 24, 2024

The codecov action failed because it's trying to download a binary to the action's directory:

Error: Codecov: Failed to write uploader binary: EACCES: permission denied, open '/__w/_actions/codecov/codecov-action/e28ff129e5465c2c0dcc6f003fc735cb6ae0c673/dist/codecov'

Is it possible/desirable to set up ACL for the actions directory in setup-homebrew, @Bo98? Though there doesn't seem to be an environment variable for that.

@Bo98
Copy link
Member

Bo98 commented Sep 24, 2024

Maybe - I'll need to investigate to see.

Quick initial question though: do we get any speedup from using the containers for RSpec tests? Homebrew/core shouldn't be required there.

@ZhongRuoyu
Copy link
Member Author

ZhongRuoyu commented Sep 24, 2024

do we get any speedup from using the containers for RSpec tests?

The speedup is only in not having to tap homebrew/core. That aside, the speed is roughly the same (see before and after).

Homebrew/core shouldn't be required there.

In that case perhaps we could just set core: false for these tests.

@ZhongRuoyu
Copy link
Member Author

Homebrew/core shouldn't be required there.

Is #15634 (comment) no longer the case?

@Bo98
Copy link
Member

Bo98 commented Sep 24, 2024

Ah there's two remaining tests that still use Homebrew/core: https://github.com/search?q=repo%3AHomebrew%2Fbrew%20needs_homebrew_core&type=code.

Though maybe the macOS coverage is good enough for them. With proper stubbing to remove the need of Homebrew/core for brew --prefix: the answer is almost certainly yes.

ZhongRuoyu and others added 3 commits September 25, 2024 00:09
The containers have `homebrew/core` tapped so we don't need to wait
forever for the tap repo to be cloned.

Co-authored-by: Mike McQuaid <[email protected]>
It's expensive to tap `homebrew/core` on GitHub-hosted Ubuntu runners,
and using the container requires tricks to make Codecov work. So, we
rely on macOS to provide coverage for the needs_homebrew_core RSpec
tests.
@MikeMcQuaid MikeMcQuaid merged commit d767c96 into master Sep 30, 2024
28 checks passed
@MikeMcQuaid MikeMcQuaid deleted the tests-speedup branch September 30, 2024 12:40
@MikeMcQuaid
Copy link
Member

Thanks @ZhongRuoyu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants