Fix ENV["CI"] usage

[MikeMcQuaid]

We shouldn't be using ENV["CI"] in formula, it's way too broad and breaks e.g. installing MySQL in GitHub Actions. Instead, use HOMEBREW_GITHUB_ACTIONS which we set in brew test-bot when on GitHub Actions.

This involves deleting a bunch of tests that did nothing before. I'm not sure if the style/audit checks will let this fly but: these tests need removed and they are worse than useless as-is. If they won't go through CI like this: I'll likely add some shitty --version or similar tests.

Fixes #70493
Closes #70494

[jonchang]

This should check GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED rather than HOMEBREW_GITHUB_ACTIONS. The former are specifically for our persistent runners, while the latter is set whenever test bot detects we are on GitHub Actions, which could be ephemeral runners.

I suggest keeping the tests in even if they have an early return on persistent runners, as we could potentially run brew test foo or brew postinstall foo on ephemeral GitHub-hosted macOS runners in the future to avoid polluting the persistent runners. Also suggest adding a rubocop for this otherwise it'll re-appear in homebrew/core.

[MikeMcQuaid]

This should check GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED rather than HOMEBREW_GITHUB_ACTIONS

That variable is filtered out so it cannot be checked for.

I suggest keeping the tests in even if they have an early return on persistent runners, as we could potentially run brew test foo or brew postinstall foo on ephemeral GitHub-hosted macOS runners in the future to avoid polluting the persistent runners.

If we do that in future: we can readd the tests from the git log.

Also suggest adding a rubocop for this otherwise it'll re-appear in homebrew/core.

I agree, this would be a good thing to have.

[MikeMcQuaid]

Also suggest adding a rubocop for this otherwise it'll re-appear in homebrew/core.

I agree, this would be a good thing to have.

Homebrew/brew#10567

[jonchang]

This should check GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED rather than HOMEBREW_GITHUB_ACTIONS

That variable is filtered out so it cannot be checked for.

It is explicitly excluded from filtering as otherwise test bot wouldn't be able to detect it.

[MikeMcQuaid]

It is explicitly excluded from filtering as otherwise test bot wouldn't be able to detect it.

This is not correct. brew test-bot is excluded from filtering, not this variable.

[jonchang]

https://github.com/Homebrew/brew/blob/3eb47725f91f13c7f86d90701aabc84d23e6d5b3/bin/brew#L99

These should be changed to GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED as otherwise this behavior will be incorrect for github-hosted runners.

[MikeMcQuaid]

https://github.com/Homebrew/brew/blob/3eb47725f91f13c7f86d90701aabc84d23e6d5b3/bin/brew#L99

Thanks for the correction, ripgrep's filtering failed me 😭

These should be changed to GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED as otherwise this behavior will be incorrect for github-hosted runners.

I don't agree. There's a subset that could be changed but many of these are applying to Homebrew's CI more generally and not just our self-hosted runners. Only PostgreSQL shared memory issues seem specific to our self-hosted configuration.

[jonchang]

I don't agree. There's a subset that could be changed but many of these are applying to Homebrew's CI more generally and not just our self-hosted runners. Only PostgreSQL shared memory issues seem specific to our self-hosted configuration.

A huge number of outside repositories, rightly or wrongly, rely on setting this variable to influence the behavior of their continuous integration:

https://github.com/search?q=HOMEBREW_GITHUB_ACTIONS&type=code

The same is not true of GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED, as these are mostly disconnected forks of homebrew/core or homebrew/brew or our own repositories:

https://github.com/search?q=GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED&type=code

The original impetus of this was to correct the behavior on CI for non-Homebrew organization repositories. It will still be broken for these hundreds of repos if they rely on these formulae. The behavior is still not correct and I think we could have taken a few more minutes to discuss these changes prior to this pull request being merged.

[MikeMcQuaid]

A huge number of outside repositories, rightly or wrongly, rely on setting this variable to influence the behavior of their continuous integration:

https://github.com/search?q=HOMEBREW_GITHUB_ACTIONS&type=code

Wrongly. We shouldn't be changing our behaviour because others misuse internal, undocumented variables.

The same is not true of GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED, as these are mostly disconnected forks of homebrew/core or homebrew/brew or our own repositories:

As said above: I agree this is the variable we should use for things that specifically fail on our self-hosted workers e.g. PostgreSQL.

For things like MySQL: the issue is with them being tested in parallel by brew test-bot. GITHUB_ACTIONS_HOMEBREW_SELF_HOSTED does not make sense here as it's nothing to do with our workers being self-hosted. Something like HOMEBREW_BREW_TEST_BOT or whatever may make more sense.

The original impetus of this was to correct the behavior on CI for non-Homebrew organization repositories. It will still be broken for these hundreds of repos if they rely on these formulae. The behavior is still not correct and I think we could have taken a few more minutes to discuss these changes prior to this pull request being merged.

The conversation is ongoing more than 24h later and it's no longer broken for users of ENV["CI"]. You're right that it's not broken for users of HOMEBREW_GITHUB_ACTIONS but: none of them have complained. There was no comment suggesting HOMEBREW_GITHUB_ACTIONS was the wrong approach on #70493.

I have no issue if you wish to open PRs to propose changing these variables. I'll happily review them.