go-delve / homebrew-delve

Homebrew formula for the Delve Go debugger
Apache License 2.0
48 stars 13 forks source link

fix: permit weak imports #2

Closed mcphailtom closed 8 years ago

mcphailtom commented 8 years ago

Adds the new directive added by homebrew team to get around the compile errors in 10.12 since weak imports are now disallowed by default.

MikeMcQuaid commented 8 years ago

This will not be needed when https://github.com/Homebrew/brew/pull/769 is merged (which should be in the next <24h).

aarzilli commented 8 years ago

I think that instead of doing this we should either add the relevant LDFLAGS to delve (but I don't have 10.12 so I can't do it) or wait for the upstream issue to be fixed, assuming it is fixed in a timely manner.

Thanks for looking into this.

PS. Re-reading @achivetta explanation in that issue I'm thinking that maybe our problem is that we are not specifying the minimum version of OS X that we need, in the makefile.

mcphailtom commented 8 years ago

great, thanks @MikeMcQuaid ... problem solved 👍 and a better solution than per formula exceptions

mcphailtom commented 8 years ago

Actually, let me roll that back, something still awry here as the @MikeMcQuaid merge does not appear to be working .....

@aarzilli I'm unclear on what the implicit min os value is, I'm hoping @achivetta can clarify this but I'm making the assumption that there is some association with the value that Golang itself is built with being passed on to packages that are subsequently build.

i.e. If the go formula specifies 10.6 as the minimum base OS that this becomes implicit for delve when built with go?

That being said I am no expert on the intricacies of compilation so this is a best guess.

MikeMcQuaid commented 8 years ago

Actually, let me roll that back, something still awry here as the @MikeMcQuaid merge does not appear to be working .....

@mcphailtom Do you have a CI failure log or something I can see? Ran brew update? Can I see your brew config?

mcphailtom commented 8 years ago

thanks @MikeMcQuaid .... I'm still trying to wrap my head around how brew hangs together

HOMEBREW_VERSION: 0.9.9
ORIGIN: https://github.com/Homebrew/brew
HEAD: 5504e2c1320d52f8a92a33c90c313a47668979c0
Last commit: 6 minutes ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 1ba3616d554237099c95ee6f39c1d9c0511d6772
Core tap last commit: 5 hours ago
HOMEBREW_PREFIX: /usr/local
HOMEBREW_REPOSITORY: /usr/local
HOMEBREW_CELLAR: /usr/local/Cellar
HOMEBREW_BOTTLE_DOMAIN: https://homebrew.bintray.com
CPU: octa-core 64-bit haswell
Homebrew Ruby: 2.0.0-p648
Clang: 8.0 build 800
Git: 2.9.3 => /usr/local/bin/git
Perl: /usr/bin/perl
Python: /usr/local/bin/python => /usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/bin/python2.7
Ruby: /Users/tommcphail/.rvm/rubies/ruby-2.3.1/bin/ruby
Java: 1.8.0_92, 1.8.0_51
OS X: 10.12-x86_64
Xcode: 8.0
CLT: N/A
X11: N/A

Delve build error :=

go build -ldflags="-s -X main.Build=v0.11.0" github.com/derekparker/delve/cmd/dlv
# github.com/derekparker/delve/proc
ld: weak import of symbol '_voucher_mach_msg_set' not supported because of option: -no_weak_imports for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [build] Error 2
MikeMcQuaid commented 8 years ago

Hmm, weird, can you link me to the formula too?

mcphailtom commented 8 years ago

@MikeMcQuaid https://github.com/go-delve/homebrew-delve/blob/master/Formula/delve.rb

mcphailtom commented 8 years ago

@MikeMcQuaid This is what you mean right?

It's specifying:

depends_on "go" => :build

which I assume is matched by the block you had in your PR

MikeMcQuaid commented 8 years ago

It is, weird. I'll dig in more on this (but feel free to beat me to it)

mcphailtom commented 8 years ago

Cool, it's time for bed here but I will definitely take a look first thing tomorrow.

aarzilli commented 8 years ago

@aarzilli I'm unclear on what the implicit min os value is, I'm hoping @achivetta can clarify this but I'm making the assumption that there is some association with the value that Golang itself is built with being passed on to packages that are subsequently build.

Regardless of where the implicit value is coming from we should probably set our own, since we are using cgo.

mcphailtom commented 8 years ago

Regardless of where the implicit value is coming from we should probably set our own, since we are using cgo.

I don't disagree with doing so at all, it's probably pragmatic to be explicit. As I understand it though that won't solve the issue. Specifically Homebrew links with LDFLAGS that disable weak links, the issue therefore is removing that flag rather than adding it. The directive from @MikeMcQuaid does this I believe on a per invocation basis

mcphailtom commented 8 years ago

This should be fixed by https://github.com/Homebrew/brew/pull/769 with one small change

mcphailtom commented 8 years ago

Looks like this won't be needed https://github.com/Homebrew/brew/pull/782