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

*: Stop using head? conditionals in test blocks #180218

Merged
merged 7 commits into from
Aug 7, 2024

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Aug 6, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

  • I thought it didn't make much sense to have head? conditionals in test blocks since we don't test HEAD builds on CI, and the formulae test blocks are mostly for CI confidence.

issyl0 added 7 commits August 6, 2024 13:32
- We don't test `HEAD` builds on CI, and the formulae test blocks are mostly for CI, so let's clean up.
- We don't test `HEAD` builds on CI, and the formulae test blocks are mostly for CI, so let's clean up.
- We don't test `HEAD` builds on CI, and the formulae test blocks are mostly for CI, so let's clean up.
- We don't test `HEAD` builds on CI, and the formulae test blocks are mostly for CI, so let's clean up.
- We don't test `HEAD` builds on CI, and the formulae test blocks are mostly for CI, so let's clean up.
- We don't test `HEAD` builds on CI, and the formulae test blocks are mostly for CI, so let's clean up.
- We don't test `HEAD` builds on CI, and the formulae test blocks are mostly for CI, so let's clean up.
@issyl0 issyl0 added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Aug 6, 2024
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.

Makes sense to me but interested if other @homebrew/core folks disagree.

@BrewTestBot BrewTestBot added this pull request to the merge queue Aug 6, 2024
Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

make sense 👍

@MikeMcQuaid MikeMcQuaid removed this pull request from the merge queue due to a manual request Aug 6, 2024
@MikeMcQuaid
Copy link
Member

Let's hold off merging until some other folks have a chance to give input (e.g. until tomorrow UK time)

@BrewTestBot BrewTestBot added this pull request to the merge queue Aug 6, 2024
@MikeMcQuaid MikeMcQuaid removed this pull request from the merge queue due to a manual request Aug 6, 2024
@MikeMcQuaid MikeMcQuaid marked this pull request as draft August 6, 2024 12:46
@MikeMcQuaid
Copy link
Member

Making a draft so @BrewTestBot leaves it alone until tomorrow. Naughty 🤖 !

@carlocab
Copy link
Member

carlocab commented Aug 6, 2024

build.head? can be removed, it's definitely nonsense.

I'm less certain about head?. In theory, --HEAD installs are ideally for upstream to use to test their builds so they can do something like brew install --HEAD foo && brew test foo in their CI. In practice, almost no upstream maintainers do this (I can think of maybe 2, but not necessarily in CI).

This means that the impact of removing it is likely small, so it maybe doesn't hurt too much. OTOH, we probably want to be nice to upstream maintainers that do rely on it, if any.

@MikeMcQuaid
Copy link
Member

build.head? can be removed, it's definitely nonsense.

That is just brew test --HEAD or not, right?

OTOH, we probably want to be nice to upstream maintainers that do rely on it, if any.

Agreed. A middle ground here could be: we remove them all, if upstream specifically requests that we re-add them: we re-add with a comment linking to them asking for it. How's that sound @carlocab?

@carlocab
Copy link
Member

carlocab commented Aug 6, 2024

build.head? can be removed, it's definitely nonsense.

That is just brew test --HEAD or not, right?

Huh, maybe 😅. Can't remember if I tested that or not when I was looking at build.head? usage in test blocks. Though not sure why it would be needed if you just used head? instead.

OTOH, we probably want to be nice to upstream maintainers that do rely on it, if any.

Agreed. A middle ground here could be: we remove them all, if upstream specifically requests that we re-add them: we re-add with a comment linking to them asking for it. How's that sound @carlocab?

Yes, sounds fine to me.

@branchvincent
Copy link
Member

Works for me, we could also remove the inverse: build.stable?

@MikeMcQuaid MikeMcQuaid marked this pull request as ready for review August 7, 2024 08:06
@MikeMcQuaid
Copy link
Member

Thanks again @issyl0!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Aug 7, 2024
Merged via the queue into master with commit 91fe5bc Aug 7, 2024
20 checks passed
@MikeMcQuaid MikeMcQuaid deleted the no-head-conditionals-in-test-blocks branch August 7, 2024 08:10
@issyl0
Copy link
Member Author

issyl0 commented Aug 7, 2024

Works for me, we could also remove the inverse: build.stable?

I'll get on it!

@chenrui333
Copy link
Member

yeah, I almost never find myself using build.head for tests, nor have I seen anyone else use it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants