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

audit: formally ban go get usage #2562

Merged
merged 2 commits into from Apr 30, 2017

Conversation

Projects
None yet
4 participants
@DomT4
Copy link
Contributor

DomT4 commented Apr 28, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

This has finally bothered me one too many times.

DomT4 added some commits Apr 28, 2017

audit: formally ban go get usage
There's been an informal ban for a while but let's
be punchier because this crops up still.
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 28, 2017

It's a pity that make files using go get will escape this check.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Apr 28, 2017

It may be fairly expensive in terms of speed, but could possibly run a File.read on Makefile if one exists and the project is using go as a dependency to check that, if there's the desire to do so.

Not sure how worth the expense/complication it is, but I can't think of any impossible-to-hurdle implementation issue.

Edit - Just to be more explicit, would probably have to be done in formula_installer rather than audit.

@bfontaine

This comment has been minimized.

Copy link
Member

bfontaine commented Apr 28, 2017

@DomT4 It would be overly complicated, I think. What if the Makefile has a target that runs go get but the formula doesn’t use it?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 28, 2017

I think if anything it would need to be a superenv shim for go that blows up if called with get, but I was mostly musing.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Apr 28, 2017

Yeah, I was mostly throwing an idea out there 😺. I think this audit tweak will catch the most likely problem which is people newish to Homebrew trying to fix go problems in formulae. I'm happy with this as-is at least for now, unless y'all desire changes.

What if the Makefile has a target that runs go get but the formula doesn’t use it?

This is actually pretty common. In some cases IIRC we even inreplace out those lines from the darn Makefile so we can run make without it fetching things we've already fetched 😓.

As always the theme comes back to upstreams needing to move to the vendor system, which is a beautiful thing that almost always "just works" ™️, but adoption on that hasn't been as strong as I hoped it would be.

@bfontaine

This comment has been minimized.

Copy link
Member

bfontaine commented Apr 28, 2017

That’s a great branch name, by the way 😄

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 30, 2017

Great work here @DomT4!

@MikeMcQuaid MikeMcQuaid merged commit 0c9047a into Homebrew:master Apr 30, 2017

3 checks passed

codecov/patch 100% of diff hit (target 64.44%)
Details
codecov/project 64.44% (+<.01%) compared to 539881f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@DomT4 DomT4 deleted the DomT4:you_shall_not_pass_go_get branch Apr 30, 2017

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Apr 30, 2017

Thanks folks ❤️

@jmhobbs jmhobbs referenced this pull request May 3, 2017

Open

1.1.0 Deprecated go get #1

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.