Skip to content

Conversation

discordianfish
Copy link
Member

This makes it possible to explicitly disable the CGO dependency.

Right now I need this to cross-compile prometheus for raspberry pi. Go disables CGO by default if doing cross compiles. With this change you can cross compile prometheus as easy as:

GOARCH=arm go build

/cc @grobie

@grobie
Copy link
Member

grobie commented Feb 9, 2015

I'm worried that this will make it very hard to understand why process_* metrics are sometimes not available even though the architecture supports it. There is currently no feedback about this fact.

For the prometheus server itself we should probably solve this issue by building releases directly on the target platform without the need for cross compilation. For other users of the client, do you have an idea how to make the side effects of "nocgo" more visible?

@discordianfish
Copy link
Member Author

@grobie Good point.. We could add the buildflags to the "Build Information" field, that would be a good start. If we need to more explicit, we can add another field 'features' saying something more explicit like: "process metrics: disabled". In general, maybe that's a better flag than 'nocgo' as well.

Regarding native compilation, we should definitely do that but that's imo a different story.

/cc @juliusv

@beorn7
Copy link
Member

beorn7 commented Feb 9, 2015

AFAIK Go already provides the 'cgo' build tag (see https://github.com/golang/go/blob/master/src/net/cgo_linux.go, https://github.com/golang/go/blob/master/src/net/cgo_stub.go ) which can be used to exclude cgo dependent parts if cgo is inactive.

@discordianfish
Copy link
Member Author

@beorn7 Maybe we should only use the cgo flag instead of the OS flags @grobie introduced?
This works for me: https://gist.github.com/discordianfish/db811052bca573758cfa

I would suggest instead of using OS dependent flags, we just enable/disable CGO depending on the OS.

@beorn7
Copy link
Member

beorn7 commented Feb 9, 2015

@discordianfish No, that wouldn't work. Parts of the standard library depend on cgo, even if it's not linux (or they put much slower work-around implementations in place).

@beorn7
Copy link
Member

beorn7 commented Feb 9, 2015

Example: On MacOS, you definitely want to leave cgo enabled, but you don't want to compile in procfs support.

@discordianfish
Copy link
Member Author

@beorn7 Okay, got it. What about this? I have to admit I get easily confused by this boolean algebra stuff but this is suppose to build the procfs stuff if cgo is enabled and it's on a compatible platform.

But this still doesn't solve @grobie objection. I think it's fine to merge this as it is, since people disabling cgo explicitly should know what they do. But if you feel like we need to make this more explicit, we could print an warning or something.

Either way, I think we definitely should have a way to build stuff depending on client_golang without cgo.

@beorn7
Copy link
Member

beorn7 commented Feb 9, 2015

👍 I think this should work as intended. But let's have @grobie have a look at it.

If cgo is disabled (for whatever reason) on linux, plan9, or solaris, the current state will create a compile error (right?). We don't want that. It has to be fixed.

Ways of reporting the reasons why process metrics are not available is orthogonal to that.

@grobie
Copy link
Member

grobie commented Feb 10, 2015

If cgo is disabled (for whatever reason) on linux, plan9, or solaris, the current state will create a compile error (right?). We don't want that. It has to be fixed.

No, if cgo is disabled, the process_collector_rest.go files is included, as !linux,!plan9,!solaris !cgo expands to (not linux AND not plan0 AND not solaris) OR not cgo. The build will gracefully succeed with process_* metrics being disabled. You can confirm this by building with

CGO_ENABLED=0 go build

👍 Hopefully the need for disabling cgo will be smaller as soon as we offer server binaries built for the major platforms and architectures.

grobie added a commit that referenced this pull request Feb 10, 2015
Add 'nocgo' tag to remove cgo dependency on build
@grobie grobie merged commit c70db11 into master Feb 10, 2015
@beorn7
Copy link
Member

beorn7 commented Feb 10, 2015

@grobie We are on the same page. By current state, I mean the code in master. I just wanted to express that this PR fixes things and doesn't make anything more confusing than it is.

@discordianfish discordianfish deleted the add-nocgo-flag branch February 10, 2015 10:30
@grobie
Copy link
Member

grobie commented Feb 10, 2015

Ah, sorry. I misunderstood you obviously. Makes sense.

On Tue, Feb 10, 2015 at 5:06 AM, Björn Rabenstein notifications@github.com
wrote:

@grobie https://github.com/grobie We are on the same page. By current
state
, I mean the code in master. I just wanted to express that this PR
fixes things and doesn't make anything more confusing than it is.

Reply to this email directly or view it on GitHub
#68 (comment)
.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants