Using glide for go package vendor #2627
Conversation
| @@ -48,6 +48,7 @@ option(COVERALLS_UPLOAD "Package code coverage data to coveralls" OFF) | |||
| option(ON_TRAVIS "Exclude special unit test on Travis CI" OFF) | |||
| option(WITH_C_API "Compile PaddlePaddle with C-API(Prediction)" OFF) | |||
| option(WITH_GOLANG "Compile PaddlePaddle with GOLANG" OFF) | |||
| option(GLIDE_INSTALL "Download and install go dependencies " ON) | |||
There was a problem hiding this comment.
Thanks for this PR!
I think we need a design doc for this work. In particular,
- Why we need package manager for Go, or say, why the default toolchain is not enough.
- Why we choose glide, comparing with other possible choices.
The design doc, together with discussions with it, would be a deposit of the reasons/thoughts we have in mind, and would be valuable for future contributors to understand the situation so could they move on towards further improvements.
There was a problem hiding this comment.
Sure, I'll add a design doc before going on working with this PR.
I thought previous discussions in #2583 is the reason why I'm working on this.
There was a problem hiding this comment.
Posted here: #2643
go/CMakeLists.txt
Outdated
| if(EXISTS $ENV{GOPATH}/bin/glide) | ||
| set(GLIDE "$ENV{GOPATH}/bin/glide") | ||
| else() | ||
| message(FATAL_ERROR "no glide executeble found: $ENV{GOPATH}/bin/glide") |
There was a problem hiding this comment.
Maybe instead of fail, we can do something like execute_process(env GOPATH ${PADDLE_GO_PATH} ${CMAKE_Go_COMPILER} get github.com/Masterminds/glide (I forgot the exact cmake command to temporarily set environment variable).
cmake/generic.cmake
Outdated
| @@ -280,7 +282,7 @@ function(go_library TARGET_NAME) | |||
| add_library(${TARGET_NAME} STATIC ${dummyfile}) | |||
| endif() | |||
| if(go_library_DEPS) | |||
| add_dependencies(${TARGET_NAME} ${go_library_DEPS}) | |||
| add_dependencies(${TARGET_NAME} ${go_library_DEPS} paddle_go_path_link) | |||
There was a problem hiding this comment.
Where is paddle_go_path_link declared?
cmake/generic.cmake
Outdated
| WORKING_DIRECTORY ${PADDLE_GO_SRC}) | ||
| add_custom_command(TARGET ${TARGET_NAME} POST_BUILD | ||
| # Automatically get all dependencies specified in the source code | ||
| #COMMAND env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} get -d ./... |
… cmake_go_vendor
typhoonzero
changed the title
| ${GO_SOURCE} | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}) | ||
| add_dependencies(${TARGET_NAME} go_path) | ||
| "./${CMAKE_CURRENT_SOURCE_REL_DIR}/${GO_SOURCE}" |
There was a problem hiding this comment.
I am curious the code works before, why now need to be under GOPATH?
There was a problem hiding this comment.
When compiling with vendor package, go build must run under GOPATH or it will fail. And this also removes warnings when glide install.
cmake/generic.cmake
Outdated
| add_custom_target(${TARGET_NAME} ALL DEPENDS ${TARGET_NAME}_timestamp ${go_binary_DEPS}) | ||
| "./${CMAKE_CURRENT_SOURCE_REL_DIR}/${go_binary_SRCS}" | ||
| WORKING_DIRECTORY "${PADDLE_IN_GOPATH}/go") | ||
| # add_custom_target(${TARGET_NAME} ALL DEPENDS go_vendor ${TARGET_NAME}_link ${TARGET_NAME}_timestamp ${go_binary_DEPS}) |
There was a problem hiding this comment.
No commended out code please.
| if(EXISTS $ENV{GOPATH}/bin/glide) | ||
| set(GLIDE "$ENV{GOPATH}/bin/glide") | ||
| else() | ||
| message(FATAL_ERROR "no glide executeble found: $ENV{GOPATH}/bin/glide") |
There was a problem hiding this comment.
This requires user to install glide, maybe we can install for them using go get github.com/Masterminds/glide?
There was a problem hiding this comment.
go getwill always install the latest version from default branch, which does not guarantee the behavior.- Thought
glideis likecmakeorgcc, put it intoDockerfileis better?
There was a problem hiding this comment.
I see, good point. Thanks.
typhoonzero
dismissed
wangkuiyi’s stale review
The design doc already merged. Dismiss this change request to merge.
Fixes: #2605
Related descussions: #2583
Using glide for go vendoring.