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

enable cgo for nvidia code #214

Merged
merged 1 commit into from Nov 26, 2018
Merged

Conversation

sharanyad
Copy link
Contributor

Summary

address build error
go build github.com/aws/amazon-ecs-init/ecs-init/vendor/github.com/NVIDIA/gpu-monitoring-tools/bindings/go/nvml: invalid flag in #cgo LDFLAGS: -Wl,--unresolved-symbols=ignore-in-object-files

Implementation details

Testing

New tests cover the changes:

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License: yes

@sharanyad sharanyad requested review from jahkeup, petderek and a team November 15, 2018 23:18
@petderek
Copy link
Contributor

Did you check make rpm?

@@ -37,14 +37,14 @@ mkdir -p "${SRCPATH}"
ln -s "${TOPWD}/ecs-init" "${SRCPATH}"
cd "${SRCPATH}/ecs-init"
if [[ "$1" == "dev" ]]; then
go build -tags 'development' -ldflags "${VERSION_FLAG} ${GIT_HASH_FLAG} ${GIT_DIRTY_FLAG}" \
CGO_ENABLED=1 CGO_LDFLAGS_ALLOW='-Wl,--unresolved-symbols=ignore-in-object-files' go build -tags 'development' -ldflags "${VERSION_FLAG} ${GIT_HASH_FLAG} ${GIT_DIRTY_FLAG}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe put the flags into another variable since we repeat later

@@ -37,14 +37,14 @@ mkdir -p "${SRCPATH}"
ln -s "${TOPWD}/ecs-init" "${SRCPATH}"
cd "${SRCPATH}/ecs-init"
if [[ "$1" == "dev" ]]; then
go build -tags 'development' -ldflags "${VERSION_FLAG} ${GIT_HASH_FLAG} ${GIT_DIRTY_FLAG}" \
CGO_ENABLED=1 CGO_LDFLAGS_ALLOW='-Wl,--unresolved-symbols=ignore-in-object-files' go build -tags 'development' -ldflags "${VERSION_FLAG} ${GIT_HASH_FLAG} ${GIT_DIRTY_FLAG}" \
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find anything to clarify the --unresolved-symbols=ignore-in-object-files portion of the flag, can you give some background on why these linker options are required for building with CGO specifically for enabling nvidia code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vendor package for nvml APIs uses the cgo ldflags here -

// #cgo LDFLAGS: -ldl -Wl,--unresolved-symbols=ignore-in-object-files

Looks like only certain ldflags are whitelisted by default (golang/go#23749) and hence the build error as mentioned in the summary of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that explains it 👍

Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

LGTM, let's work on getting a test build and confirm that this doesn't affect anything we're not thinking about right now.

@@ -37,14 +37,14 @@ mkdir -p "${SRCPATH}"
ln -s "${TOPWD}/ecs-init" "${SRCPATH}"
cd "${SRCPATH}/ecs-init"
if [[ "$1" == "dev" ]]; then
go build -tags 'development' -ldflags "${VERSION_FLAG} ${GIT_HASH_FLAG} ${GIT_DIRTY_FLAG}" \
CGO_ENABLED=1 CGO_LDFLAGS_ALLOW='-Wl,--unresolved-symbols=ignore-in-object-files' go build -tags 'development' -ldflags "${VERSION_FLAG} ${GIT_HASH_FLAG} ${GIT_DIRTY_FLAG}" \
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that explains it 👍

@sharanyad sharanyad merged commit 8fa2327 into aws:gpu-support Nov 26, 2018
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.

None yet

3 participants