FIX: Polish go library #2583

gangliao · 2017-06-23T09:28:29Z

Update go_library and go_extern
Build go/pserver

TEST:

cmake .. -DWITH_GOLANG
make test_cclient

OUTPUT:

gangl@GangLiao ~/b/P/build> make test_cclient
[  0%] Built target go_logrus
[  0%] Built target go_pserver
Scanning dependencies of target paddle_pserver_cclient
[  0%] Building C object go/pserver/cclient/CMakeFiles/paddle_pserver_cclient.dir/paddle_pserver_cclient_dummy.c.o
[ 33%] Linking C static library libpaddle_pserver_cclient.a
[ 33%] Built target paddle_pserver_cclient
[100%] Built target extern_gtest
Scanning dependencies of target test_cclient
[100%] Building C object go/pserver/cclient/test/CMakeFiles/test_cclient.dir/test_cclient.c.o
[100%] Linking C executable test_cclient
[100%] Built target test_cclient

jacquesqiao · 2017-06-23T15:33:27Z

cmake ..
make test_cclient

这个是在哪个目录下执行的？

我在paddle/build下执行，报

No rule to make target `test_cclient'.  Stop

helinwang · 2017-06-23T21:28:39Z

Thank you very much for making the change! I was making trying to make the change and found out you already did it. Awesome!

Can you make go_extern(go_pserver ${rel}/...) Happen automatically inside go_library? User should not need to write it every time. Also, go_extern(go_logrus github.com/sirupsen/logrus) should not be necessary, since go get -d ./... already do it, if the program depends on "github.com/sirupsen/logrus". So I think we can remove go_extern by inlining it into go_library.

Also, we need to symlink Paddle root directory to $GOPATH/src/github.com/PaddlePaddle/Paddle, because otherwise if the *.go file we are building imports github.com/PaddlePaddle/Paddle/go/xxx go get -d ./... will download another fresh Paddle from github and put into there (or if we don't use go get -d ./... it will not compile). But the fresh Paddle don't contain the go code change that we made to the Paddle repo.

Here is a reference implementation for doing the symlink:

function(go_library TARGET_NAME)
  set(options OPTIONAL)
  set(oneValueArgs "")
  set(multiValueArgs DEPS)
  cmake_parse_arguments(go_library "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})

  if (${go_library_OPTIONAL} STREQUAL "SHARED")
    set(BUILD_MODE "-buildmode=c-shared")
    if(APPLE)
      set(LIB_NAME "lib${TARGET_NAME}.dylib")
    else()
      set(LIB_NAME "lib${TARGET_NAME}.so")
    endif()
  else()
    set(BUILD_MODE "-buildmode=c-archive")
    set(LIB_NAME "lib${TARGET_NAME}.a")
  endif()
  file(GLOB GO_SOURCE RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}" "*.go")

  set(GOPATH "${CMAKE_CURRENT_BINARY_DIR}/go")
  file(MAKE_DIRECTORY ${GOPATH})
  set(PADDLE_IN_GOPATH "${GOPATH}/src/github.com/PaddlePaddle")
  # automatically get all dependencies specified in the source code                                                                                                                                                        
  # for given target.                                                                                                                                                                                                      

  # Find PADDLE_DIR, we need to symlink Paddle directory into                                                                                                                                                              
  # GOPATH. If we don't do it and we have code that depends on Paddle,                                                                                                                                                     
  # go get ./... will download a new Paddle repo from Github, without                                                                                                                                                      
  # the changes in our current Paddle repo that we want to build.                                                                                                                                                          
  file(RELATIVE_PATH rel ${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR})
  get_filename_component(PARENT_DIR ${CMAKE_CURRENT_SOURCE_DIR} DIRECTORY)
  get_filename_component(PARENT_DIR ${PARENT_DIR} DIRECTORY)
  get_filename_component(PADDLE_DIR ${PARENT_DIR} DIRECTORY)

  add_custom_command(OUTPUT ${TARGET_NAME}_timestamp
    # Symlink Paddle directory into GOPATH                                                                                                                                                                                 
    COMMAND mkdir -p ${PADDLE_IN_GOPATH}/Paddle
    COMMAND rm -rf ${PADDLE_IN_GOPATH}/Paddle
    COMMAND ln -sf ${PADDLE_DIR} ${PADDLE_IN_GOPATH}/Paddle
    # Automatically get all dependencies specified in the source code                                                                                                                                                      
    COMMAND env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} get -d ${rel}/...
    # Build                                                                                                                                                                                                                
    COMMAND env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} build ${BUILD_MODE}
    -o "${CMAKE_CURRENT_BINARY_DIR}/${LIB_NAME}"
    ${GO_SOURCE}
    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})

  add_custom_target(${TARGET_NAME}_lib ALL DEPENDS ${TARGET_NAME}_timestamp)
  add_library(${TARGET_NAME} STATIC IMPORTED)
  set_property(TARGET ${TARGET_NAME} PROPERTY
    IMPORTED_LOCATION "${CMAKE_CURRENT_BINARY_DIR}/${LIB_NAME}")
  add_dependencies(${TARGET_NAME} ${TARGET_NAME}_lib)
endfunction(go_library)

helinwang · 2017-06-23T21:35:14Z

Sorry about the feature request, can you make it support go library linking a C static library as dependency? @dzhwinter need to use this feature.
I think doing go build -ldflags ... is sufficient:

-ldflags 'flag list'
arguments to pass on each go tool link invocation.
https://golang.org/cmd/go/#hdr-Compile_packages_and_dependencies

dzhwinter · 2017-06-24T06:09:06Z

Thanks for this great job!
Agree with @helinwang 's comment, clone a fresh paddle from PaddlePaddle github doen't contain the changes in my workspace, and it is a long time to download the repo all when I change a piece of code.

[100%] Built target go_logrus
# cd .; git clone https://github.com/PaddlePaddle/Paddle /Users/dzh/.go/src/github.com/PaddlePaddle/Paddle/build/go/src/github.com/PaddlePaddle/Paddle
Cloning into '/Users/dzh/.go/src/github.com/PaddlePaddle/Paddle/build/go/src/github.com/PaddlePaddle/Paddle'...

dzhwinter · 2017-06-24T11:49:15Z

should we set GOPATH include the local path? for instance,

  add_custom_target(${NAME}_goGet env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} get -d ${rel}/...)

we can set GOPATH in local path.

typhoonzero · 2017-06-26T02:07:52Z

cmake/generic.cmake

@@ -316,5 +316,5 @@ endfunction(go_test)
 # go_extern(target_name extern_source)
 # go_extern(go_redis github.com/hoisie/redis)
 function(go_extern TARGET_NAME)
-  add_custom_target(${TARGET_NAME} env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} get ${ARGN})
+  add_custom_target(${TARGET_NAME} env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} get -d ${ARGN})


Should we add -u also when doing go get so we can use the latest version of external dependencies?

@helinwang Also should we use Godeps to setup static dependencies, so we can avoid when the external lib updates and deprecates some API we are using.

@typhoonzero I think when using -u the way we use symlink to link github.com/PaddlePaddle/Paddle in $GOPATH may break, since when using -u flag, it will try to update the local git repo. -u have the benefit of always having the latest code, but the build may break if dependency introduces breaking change. If we want to be explicit on the dependency version management, as you suggested, a vendor tool would be the best fit.
Maybe let's use without -u for now, and add a vendor tool?

I see. Thanks for these details. I'll look into vendor tools.

gangliao · 2017-06-26T02:14:32Z

@helinwang @typhoonzero @dzhwinter Thank you guys for all your comments. I will follow and do it today.

typhoonzero · 2017-06-26T11:26:13Z

@dzhwinter @gangliao I've tried use godep with godep save ./... it will save all the dependencies to ./Godeps directory. So for users who can not access google.golang.org or golang.org can do the below:

add an option like GODEP_PKG=[URL] to download a saved godep package
if GODEP_PKG is set, download the package and unpack it to go/
run go restore to install package from Godeps to $GOPATH
run go build or go test

A godep package must be uploaded to somewhere to let users to download from.

gangliao · 2017-06-26T12:26:19Z

@typhoonzero Cool! Thanks. We can do this in the next PR.

For this pull request, I solved @helinwang 's comment 'automatically download the dependencies'.

-ldflags can not solve the link problem, it only works using LDFLAGS: -Lxxx in *.go @dzhwinter

dzhwinter · 2017-06-26T16:39:37Z

Great job! confirm Done.

helinwang · 2017-06-26T18:20:40Z

Thanks @gangliao @dzhwinter @typhoonzero !!! This is super helpful, Greatly appreciated!

helinwang · 2017-06-26T23:19:02Z

@typhoonzero mentioned godep, maybe we could consider glide as well. I have not used either of them. Here is some materials I found: https://github.com/Masterminds/glide/wiki/Go-Package-Manager-Comparison , coreos/docs#775

helinwang · 2017-06-26T23:32:39Z

cmake/generic.cmake

@@ -257,31 +258,50 @@ file(MAKE_DIRECTORY ${GOPATH})
 #   tensor # Because ops depend on tensor, this line is optional.


Can you update the comment, currently it's

# go_library(api # SRCS # api.go # DEPS

I think we don't need to provide api.go anymore. Could you mention the use of STATIC and SHARED as well?

helinwang · 2017-06-26T23:33:18Z

cmake/generic.cmake

-    else()
-      set(LIB_NAME "lib${TARGET_NAME}.so")
-    endif()
+    set(LIB_NAME "${CMAKE_SHARED_LIBRARY_PREFIX}${TARGET_NAME}${CMAKE_SHARED_LIBRARY_SUFFIX}")


赞CMAKE_SHARED_LIBRARY_SUFFIX！

helinwang · 2017-06-26T23:35:06Z

cmake/generic.cmake

+    COMMAND rm -rf ${PADDLE_IN_GOPATH}                                                                                                                                         
+    COMMAND ln -sf ${CMAKE_SOURCE_DIR} ${PADDLE_IN_GOPATH}
+    # Automatically get all dependencies specified in the source code                                                                                                                                 
+    COMMAND env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} get -d .


I think we need go get -d ./... instead of go get -d ., ./... tells Go tool to get all dependency for current dir and all sub-dirs.

typhoonzero · 2017-06-27T03:50:33Z

@helinwang check out this wiki: https://github.com/Masterminds/glide/wiki/Go-Package-Manager-Comparison, glide seems cool choice, I'll try add a PR about this!

helinwang · 2017-06-27T05:19:37Z

@typhoonzero Awesome, thanks!!!

helinwang · 2017-06-27T05:19:39Z

@gangliao LGTM++

FIX: Polish go library

869f2b3

gangliao requested review from helinwang, jacquesqiao and wangkuiyi June 23, 2017 09:28

ENH: Change to CMAKE variable

09a50cb

typhoonzero reviewed Jun 26, 2017

View reviewed changes

dzhwinter mentioned this pull request Jun 26, 2017

go packages blocked by network #2605

Closed

FIX: go get dependencies automatically

736af1f

FIX: support shared type

5c67669

helinwang approved these changes Jun 26, 2017

View reviewed changes

ENH: Add go doc

4bfd757

gangliao merged commit de77faf into PaddlePaddle:develop Jun 27, 2017

This was referenced Jun 28, 2017

Using glide for go package vendor #2627

Merged

Design doc for go package management #2643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Polish go library #2583

FIX: Polish go library #2583

gangliao commented Jun 23, 2017 •

edited

Loading

jacquesqiao commented Jun 23, 2017 •

edited

Loading

helinwang commented Jun 23, 2017 •

edited

Loading

helinwang commented Jun 23, 2017

dzhwinter commented Jun 24, 2017

dzhwinter commented Jun 24, 2017

typhoonzero Jun 26, 2017

typhoonzero Jun 26, 2017

helinwang Jun 26, 2017 •

edited

Loading

typhoonzero Jun 27, 2017

gangliao commented Jun 26, 2017

typhoonzero commented Jun 26, 2017

gangliao commented Jun 26, 2017

dzhwinter commented Jun 26, 2017

helinwang commented Jun 26, 2017

helinwang commented Jun 26, 2017 •

edited

Loading

helinwang Jun 26, 2017

gangliao Jun 27, 2017

helinwang Jun 26, 2017

helinwang Jun 26, 2017 •

edited

Loading

gangliao Jun 27, 2017

typhoonzero commented Jun 27, 2017

helinwang commented Jun 27, 2017

helinwang commented Jun 27, 2017

		@@ -257,31 +258,50 @@ file(MAKE_DIRECTORY ${GOPATH})
		# tensor # Because ops depend on tensor, this line is optional.

FIX: Polish go library #2583

FIX: Polish go library #2583

Conversation

gangliao commented Jun 23, 2017 • edited Loading

jacquesqiao commented Jun 23, 2017 • edited Loading

helinwang commented Jun 23, 2017 • edited Loading

helinwang commented Jun 23, 2017

dzhwinter commented Jun 24, 2017

dzhwinter commented Jun 24, 2017

typhoonzero Jun 26, 2017

Choose a reason for hiding this comment

typhoonzero Jun 26, 2017

Choose a reason for hiding this comment

helinwang Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

typhoonzero Jun 27, 2017

Choose a reason for hiding this comment

gangliao commented Jun 26, 2017

typhoonzero commented Jun 26, 2017

gangliao commented Jun 26, 2017

dzhwinter commented Jun 26, 2017

helinwang commented Jun 26, 2017

helinwang commented Jun 26, 2017 • edited Loading

helinwang Jun 26, 2017

Choose a reason for hiding this comment

gangliao Jun 27, 2017

Choose a reason for hiding this comment

helinwang Jun 26, 2017

Choose a reason for hiding this comment

helinwang Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

gangliao Jun 27, 2017

Choose a reason for hiding this comment

typhoonzero commented Jun 27, 2017

helinwang commented Jun 27, 2017

helinwang commented Jun 27, 2017

gangliao commented Jun 23, 2017 •

edited

Loading

jacquesqiao commented Jun 23, 2017 •

edited

Loading

helinwang commented Jun 23, 2017 •

edited

Loading

helinwang Jun 26, 2017 •

edited

Loading

helinwang commented Jun 26, 2017 •

edited

Loading

helinwang Jun 26, 2017 •

edited

Loading