Stub out library when not built with cgo enabled #11
Conversation
|
Hi @mindprince , any chance of accepting this PR. It would help-out building cadvisor (that depends on this lib) without CGO. |
| szUUID = 0 | ||
| ) | ||
|
|
||
| var errLibraryNotLoaded = errors.New("this binary is built without CGO, NVML is disabled") |
There was a problem hiding this comment.
Let's rename this to errNoCgo?
There was a problem hiding this comment.
That would be fine. I think maybe the file name could be renamed to bindings_nocgo.go
| return "", errLibraryNotLoaded | ||
| } | ||
|
|
||
| // DeviceCount returns the number of nvidia devices on the system. |
There was a problem hiding this comment.
What's the best practice for built-constraint files? Do we repeat the method comments in all such files? Or do we keep it only at one place?
There was a problem hiding this comment.
Usually the libs implement this to make clear the function use. Even in stubs.
There was a problem hiding this comment.
For different build-constraint you should have comments on each method. They don't need to be the same (e.g. this could say "returns an error to indicate NVML is disabled"), but they should be there.
| "time" | ||
| ) | ||
|
|
||
| const ( |
| return "", errLibraryNotLoaded | ||
| } | ||
|
|
||
| // DeviceCount returns the number of nvidia devices on the system. |
There was a problem hiding this comment.
For different build-constraint you should have comments on each method. They don't need to be the same (e.g. this could say "returns an error to indicate NVML is disabled"), but they should be there.
|
@dashpole @mindprince I've been testing this PR and it works pretty fine. Any news on it getting merged? |
|
I am happy to merge this once the comments are addressed.
…--
Rohit
On Wed, Aug 28, 2019 at 2:37 PM Carlos Eduardo ***@***.***> wrote:
@dashpole <https://github.com/dashpole> @mindprince
<https://github.com/mindprince> I've been testing this PR and it works
pretty fine. Any news on it getting merged?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11?email_source=notifications&email_token=AAFJX6UUVN3PW47QSOTME4LQG3V2TA5CNFSM4HMLFQNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5MRSFQ#issuecomment-525932822>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFJX6W5W6IVXZTODJUZR23QG3V2TANCNFSM4HMLFQNA>
.
|
|
@mindprince I've added most recommendations, would you mind if I submit a new PR with reference to this? |
|
Sure, go ahead.
Consider adding the original author of the PR as the co-author of the
commit:
https://help.github.com/en/articles/creating-a-commit-with-multiple-authors.
…On Wed, Aug 28, 2019 at 2:46 PM Carlos Eduardo ***@***.***> wrote:
@mindprince <https://github.com/mindprince> I've added most
recommendations, would you mind if I submit a new PR with reference to this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11?email_source=notifications&email_token=AAFJX6VXHH6JIGARZ46RWWLQG3W3HA5CNFSM4HMLFQNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5MSG6Y#issuecomment-525935483>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFJX6VD3USQ3IQSPKYR4PLQG3W3HANCNFSM4HMLFQNA>
.
|
|
I've had this in my K8s tree for some time (as part of cadvisor/kubelet), it works fine. @carlosedp Thanks for integrating the comments, I've seen them but had no time to deal with this. |
|
Oh, no probs @lorenz ... I didn't knew how involved with it you were so I forked the PR. Hope you don't mind but I added you too as the author. :) |
GoNVML depends on C code so it (and everything that depends on it) is currently not buildable with cgo disabled. Since its API design assumes that everything can fail anyways if the NVML is not available this makes it possible to just always fail if cgo is disabled. That also makes sense since systems without C standard library will not be able to run NVML anyways.
I've modified the
errLibraryNotLoadedmessage in thebindings_unsupported.gofile, that way it is clear to the user when he runs into an initialization error that the error is caused by not using cgo to build his binary.