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

bats-core 1.0.1 (new formula) #28977

Merged
merged 2 commits into from
Jun 14, 2018
Merged

Conversation

kenden
Copy link
Contributor

@kenden kenden commented Jun 13, 2018

This is an updated version from closed PR #20073
(keeping the commit from @btamayo)

Now that bats-core has a stable release, it is time to create a formula for it.

I propose creating an alias bats and removing the existing formula bats
once bats-core has had a few more stable releases.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@SMillerDev
Copy link
Member

Maybe alias bats -> bats-core and then rename the original bats to [email protected]? @ilovezfs probably has an opinion on this.

@kenden
Copy link
Contributor Author

kenden commented Jun 13, 2018

@SMillerDev Thanks. Could you check my renaming of the original bats? I'm unsure if I need to change the file content also.

@ilovezfs ilovezfs requested a review from woodruffw June 13, 2018 11:04
@ilovezfs ilovezfs added the formula rename PR or issue relates to changing a formula's name label Jun 13, 2018
@SMillerDev
Copy link
Member

@kenden Jenkins says you do 😉. Also, by convention the versioned formula would be called [email protected] with the class BatsAt04

@kenden kenden force-pushed the new-bats-core-formula branch 6 times, most recently from e87db7e to bb50b91 Compare June 13, 2018 13:22
@kenden
Copy link
Contributor Author

kenden commented Jun 13, 2018

[email protected] cannot have:

  • keg_only :versioned_formula
    (else it fails to install with:
    "This formula is keg-only, which means it was not symlinked into /usr/local,
    because this is an alternate version of another formula.")
  • conflicts_with "bats-core"
    (Jenkins does not allow it for versioned formulae. Although, in this case, I think it would be correct and Jenkins is wrong)
  • none of those (neither conflicts_with or keg_only)
    (else it has a problem if bats-core is installed already)

So, I suggest renaming bats to [email protected],
and also naming bats-core to [email protected]
It this case, keg_only :versioned_formula should work.
@SMillerDev @ilovezfs, @woodruffw what do you think?

bottle :unneeded

conflicts_with "[email protected]",
:because => "[email protected] and bats-core install a `bats` executable"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "both install a bats executable" is slightly shorter, and is what we use elsewhere for this type of conflict 🙂

Copy link
Contributor Author

@kenden kenden Jun 13, 2018

Choose a reason for hiding this comment

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

Fixed, there is no bats formula to conflict with any more, so I removed the block

@ilovezfs
Copy link
Contributor

We probably don't need the @ formula at all.

@woodruffw
Copy link
Member

I agree with @ilovezfs -- unmaintained project + new, maintained fork that's compatible suggests removal and aliasing to me.

@kenden kenden changed the title New bats-core formula New bats-core formula && remove bats formula Jun 13, 2018
@kenden
Copy link
Contributor Author

kenden commented Jun 13, 2018

@woodruffw I removed the old bats formula, added bats-core and aliased it to bats. All checks pass.

@ilovezfs
Copy link
Contributor

formula_renames.json too

@kenden
Copy link
Contributor Author

kenden commented Jun 13, 2018

@woodruffw, how does it get merged?

@ilovezfs
Copy link
Contributor

ilovezfs commented Jun 13, 2018

After some investigation, I'm actually a bit concerned with just handling this as a rename. Based on sstephenson/bats#150, bats-core has not yet received an official nod from the original upstream, and despite sstephenson/bats#150 (comment), there is some anecdotal evidence that it is not: sobolevn/git-secret#203

For now I'd be more comfortable treating this just as a new formula, and leave the original bats in place. We can always revisit that later.

CC @mislav @sstephenson for any thoughts!

@ilovezfs ilovezfs changed the title New bats-core formula && remove bats formula bats-core 1.0.1 (new formula) Jun 13, 2018
@ilovezfs ilovezfs added new formula PR adds a new formula to Homebrew/homebrew-core and removed formula rename PR or issue relates to changing a formula's name labels Jun 13, 2018
@kenden
Copy link
Contributor Author

kenden commented Jun 13, 2018

@ilovezfs I agree, that's what I wrote in the PR description originally.

@kenden
Copy link
Contributor Author

kenden commented Jun 13, 2018

@ilovezfs @SMillerDev @woodruffw I reverted, keeping the original bats and just adding bats-core.

@woodruffw
Copy link
Member

woodruffw commented Jun 13, 2018 via email

Formula/bats.rb Outdated
@@ -3,10 +3,11 @@ class Bats < Formula
homepage "https://github.com/sstephenson/bats"
url "https://github.com/sstephenson/bats/archive/v0.4.0.tar.gz"
sha256 "480d8d64f1681eee78d1002527f3f06e1ac01e173b761bc73d0cf33f4dc1d8d7"
head "https://github.com/sstephenson/bats.git"
Copy link
Contributor

Choose a reason for hiding this comment

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

Reason to remove ability to install --HEAD ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-added it

desc "Bash Automated Testing System"
homepage "https://github.com/bats-core/bats-core"
url "https://github.com/bats-core/bats-core/archive/v1.0.1.tar.gz"
sha256 "821626f1e5058a4f25a95722399b460942f27535186a815a279e518b503f8de7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add head to support --HEAD installs?

Copy link
Contributor Author

@kenden kenden Jun 13, 2018

Choose a reason for hiding this comment

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

@jasonkarns I can, but CONTRIBUTING.md
says to run
$ brew audit --new-formula bats-core
which then breaks with:

bats-core:
  * Formulae should not have a HEAD spec
Error: 1 problem in 1 formula

Trying anyway, maybe the jenkins config differs.

Copy link
Contributor Author

@kenden kenden Jun 13, 2018

Choose a reason for hiding this comment

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

No, Jenkins does not like it for new Formulae. Removing head from bats-core.

@ilovezfs ilovezfs merged commit 30c6ae4 into Homebrew:master Jun 14, 2018
@ilovezfs
Copy link
Contributor

@btamayo @kenden Thanks for your first contributions to Homebrew! You're awesome. 🦇

@ilovezfs ilovezfs mentioned this pull request Jun 14, 2018
4 tasks
@kenden
Copy link
Contributor Author

kenden commented Jun 14, 2018

@ilovezfs I'm glad it's merged.
One note: I didn't see that "Allow edits from maintainers" checked-by-default checkbox on the PR. Thanks for adding changes, but please don't use git rebase on other people's PRs.
The PR's commits should look like that:

2bf55e7 - author ilovezfs - Update test in bats-core.rb
f27f255c - author kenden - Replace caveats block by conflicts_with
7d36a405 - author - kenden - Update bats-core formula to version 1.0.1
0863a884 - author btamayo - bats-core 0.4.0 (new formula)

Thanks!

@ilovezfs
Copy link
Contributor

@kenden we squash PRs down to one commit per formula per PR in virtually all cases, and certainly always in new formula PRs.

@kenden
Copy link
Contributor Author

kenden commented Jun 14, 2018

@ilovezfs It makes sense, but it didn't happen this time:
Homebrew:3d822c7944ebcf654fdf4fcd65d8d0b0353807d6...master
It's not one commit, but 2, and changed.

@kenden kenden deleted the new-bats-core-formula branch June 14, 2018 08:39
@ilovezfs
Copy link
Contributor

@kenden I said one commit per formula ;)

@mislav
Copy link
Sponsor Contributor

mislav commented Jun 14, 2018

For now I'd be more comfortable treating this just as a new formula, and leave the original bats in place. We can always revisit that later.

Good call. bats-core is an unofficial successor to bats. I'm 👍 for having a new formula that is marked as conflicts with bats.

@lock lock bot added the outdated PR was locked due to age label Jul 14, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants