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

buildifier 4.0.0 + simplified build process #70921

Closed
wants to merge 2 commits into from
Closed

buildifier 4.0.0 + simplified build process #70921

wants to merge 2 commits into from

Conversation

vvvvv
Copy link
Contributor

@vvvvv vvvvv commented Feb 11, 2021

Currently buildifier fails to build and blocks #70446 and #70143 from moving forward.
The reason buildifier fails has nothing to do with go but with its build system bazel and bazelisk as explained here.
This update increases the version to 4.0.0 but also simplifies the build process.
The build no longer depends on bazelisk which should simplify things in the future.

This is a duplicate of #70328 but because it's more then just a version update I created a new PR.

  • 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>)?

@BrewTestBot BrewTestBot added go Go use is a significant feature of the PR or issue no ARM bottle Formula has no ARM bottle labels Feb 11, 2021
This was referenced Feb 11, 2021
Comment on lines +25 to +35
(dir/"go.mod").write <<~EOS
module github.com/bazelbuild/buildifier
go 1.15
require (
github.com/bazelbuild/buildtools v0.0.0-20210208125002-752270cf2f52
github.com/golang/protobuf v1.4.3 // indirect
google.golang.org/protobuf v1.25.0 // indirect
)
EOS

(dir/"go.sum").write <<~EOS
Copy link
Member

Choose a reason for hiding this comment

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

Does upstream not have these? We do not want to maintain them in the formula.

Copy link
Contributor Author

@vvvvv vvvvv Feb 11, 2021

Choose a reason for hiding this comment

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

Upstream doesn't have those as they build it with bazel - which is total overkill and failes to build right now because of reasons that have to do with bazel not buildifier or go.

I know this isn't ideal it's also not ideal to block a perfectly working language update (go 1.15.8) because you try to rebuild every package using go, but also packages written in go that fail because of totally different reasons.

Go 1.15.8 is being blocked because bazel 4.0.0 (nothing to do with go) misbehaves right now and buildifier used to be build with bazel.

Copy link
Member

Choose a reason for hiding this comment

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

If we go from "unmaintainable because of bazel" to "unmaintainable because of hardcoded files" it doesn't really fix anything for brew. In that case we're better off deprecating the formula because it doesn't build.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened a PR bazelbuild/buildtools#959 on upstream, but the test failed. And I don't know how to fix it because I don't use Bazel at all. Any help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we go from "unmaintainable because of bazel" to "unmaintainable because of hardcoded files" it doesn't really fix anything for brew. In that case we're better off deprecating the formula because it doesn't build.

It's not pretty but it's far from "unmaintainable because of hardcoded files".
It's version pinned and might not be an issue with the next update when upstream switches to go modules.
If they don't switch then it's super easy to update.

Copy link
Member

Choose a reason for hiding this comment

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

It's not pretty but it's far from "unmaintainable because of hardcoded files".

I disagree

be maintained (i.e. the last release wasn’t ages ago, it works without patching on all supported macOS releases and has no outstanding, unpatched security vulnerabilities)

https://docs.brew.sh/Acceptable-Formulae#niche-or-self-submitted-stuff

We're essentially opening this rule up and stretching it to it's very limits because

  1. It requires heavy patching (adding a new build system)
  2. This patching might introduce security vulnerabilities because homebrew isn't going to keep track of all used dependencies.

I don't know if @Homebrew/core disagrees, but I think we need to drop this formula because we can't make it work anymore.


def install
system "bazelisk", "build", "--config=release", "buildifier:buildifier"
bin.install "bazel-bin/buildifier/darwin_amd64_stripped/buildifier"
ENV["GOPATH"] = buildpath
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed for modern projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in dce2d5b

@SMillerDev
Copy link
Member

I made #71330 to disable these, they're not worth the maintenance effort of rolling our own go.mod files etc

@SMillerDev SMillerDev closed this Feb 18, 2021
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 21, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 21, 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 no ARM bottle Formula has no ARM bottle outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants