Skip to content

vitess 8.0.0 (new formula)#66364

Closed
ankane wants to merge 1 commit intoHomebrew:masterfrom
ankane:vitess
Closed

vitess 8.0.0 (new formula)#66364
ankane wants to merge 1 commit intoHomebrew:masterfrom
ankane:vitess

Conversation

@ankane
Copy link
Copy Markdown
Contributor

@ankane ankane commented Dec 6, 2020

  • 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?
  • 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 brew install <formula>)?

Notes

  • make install doesn't install mysqlctl and vtctl, which are needed to run the local examples (called in ./101_initial_cluster.sh). make install-local on master installs mysqlctl but not vtctl (PR to add vtctl: Add vtctl to make install-local vitessio/vitess#7125), so the formula hopefully won't need the bin.install lines in the future.

@BrewTestBot BrewTestBot added go Go use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core labels Dec 6, 2020
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you sure automake is needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, was part of the Mac instructions, but looks like it's not needed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If go is used, how are the dependencies/modules handled? Where are their versions specified, are they pinned, etc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not super familiar with Go packaging, but looks like go.sum: https://github.com/vitessio/vitess/blob/master/go.sum

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

vitess uses go modules, the dependencies are in go.mod

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are mysqlctl and vtctl used outside of our tests? If not, they should go into pkgshare as well

Copy link
Copy Markdown
Contributor Author

@ankane ankane Dec 6, 2020

Choose a reason for hiding this comment

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

These aren't used in Homebrew tests, but are needed in bin if users want to follow the Getting Started tutorial on the Vitess website.

sha256 "c47320b9bcb874b1a6dfca78ec677be7c4bb4c7b2a6470df80bd1bc0ad125e92"
license "Apache-2.0"

depends_on "go" => :build
Copy link
Copy Markdown

@systay systay Dec 8, 2020

Choose a reason for hiding this comment

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

Vitess only works using go@1.13

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like it builds fine with the latest version and we don't allow new formula to depend on legacy software.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@SMillerDev just fyi, go 1.13 isn't legacy, it still receives security updates: https://golang.org/doc/devel/release.html#go1.13.minor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Until February, meanwhile there have been 2 new releases. It's not deprecated but it is legacy IMHO.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Building might work, but tests do not pass for newer go versions. It's shameful, but still not done. :(

Vitess is only supported on go@1.13 at the moment (I'm one of the maintainers of Vitess)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tend to agree: we can accept this formula when it builds with latest go.

@SMillerDev SMillerDev requested a review from fxcoudert December 8, 2020 09:07
@ankane
Copy link
Copy Markdown
Contributor Author

ankane commented Dec 10, 2020

Will go ahead and close for now. @systay @deepthi feel free to let me know when Go 1.15 is supported (looks like there's been some work done here: vitessio/vitess#6792).

@ankane ankane closed this Dec 10, 2020
@systay
Copy link
Copy Markdown

systay commented Jan 8, 2021

@ankane the upcoming Vitess 9.0 release will run on golang 1.15. We released 9.0-rc1 yesterday. .https://github.com/vitessio/vitess/releases/tag/v9.0.0-rc1

@ankane
Copy link
Copy Markdown
Contributor Author

ankane commented Jan 8, 2021

@systay great to hear, will be on the lookout for the final release.

@ankane ankane mentioned this pull request Jan 26, 2021
5 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 8, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

go Go use is a significant feature of the PR or issue 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.

7 participants