Skip to content

Faster glide #2869

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.

Sign up for GitHub

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

Closed
typhoonzero wants to merge 3 commits into from
Closed

Faster glide #2869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2c1d77
faster glide
Jul 14, 2017
872bb8c
remove comment
Jul 14, 2017
dba4fe0
revert build
Jul 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions cmake/configure.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,12 @@ if(WITH_GOLANG)
file(MAKE_DIRECTORY "${PADDLE_IN_GOPATH}")
set(PADDLE_GO_PATH "${CMAKE_SOURCE_DIR}/go")

add_custom_target(go_path)
add_custom_command(TARGET go_path
message(STATUS "link ${CMAKE_SOURCE_DIR} to ${PADDLE_IN_GOPATH}...")

execute_process(
Copy link
Contributor

@helinwang helinwang Jul 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think the execute_process will only run when cmake is executed (when running cmake .., or when cmake file changed). But it will not run every time make is called, or when glide.lock is changed.

Also it may be better not to delete the add_custom_target(go_vendor), since go build need to add dependency to go_vendor target as well, otherwise go build may start before or concurrently with glide install.

This is an interesting problem, I gave it a try as well, can you take a look? #2884

Copy link
Contributor Author

@typhoonzero typhoonzero Jul 15, 2017

Choose a reason for hiding this comment

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

Interesting, don't know why travis didn't run successfully. Well, if compiling with cmake, the vendor will be prepared at cmake time, and following go build will pass. Seems #2884 is better~ Thanks~

# Symlink Paddle directory into GOPATH
COMMAND mkdir -p ${PADDLE_IN_GOPATH}
COMMAND rm -rf ${PADDLE_IN_GOPATH}
COMMAND rm -rf ${PADDLE_IN_GOPATH}/paddle
COMMAND ln -sf ${CMAKE_SOURCE_DIR} ${PADDLE_IN_GOPATH}
# Automatically get all dependencies specified in the source code
# We can't run `go get -d ./...` for every target, because
Expand All @@ -102,12 +103,17 @@ if(WITH_GOLANG)
message(FATAL_ERROR "no glide executeble found: $ENV{GOPATH}/bin/glide")
endif()

add_custom_target(go_vendor)
add_custom_command(TARGET go_vendor
message(STATUS "glide installing...")

execute_process(
COMMAND env GOPATH=${GOPATH} ${GLIDE} install
WORKING_DIRECTORY "${PADDLE_IN_GOPATH}/go"
WORKING_DIRECTORY "${PADDLE_IN_GOPATH}/paddle/go"
RESULT_VARIABLE res_var
)
add_dependencies(go_vendor go_path)
if(NOT "${res_var}" STREQUAL "0")
message(FATAL_ERROR "glide install error")
endif()

endif()

endif(WITH_GOLANG)
5 changes: 2 additions & 3 deletions cmake/generic.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,6 @@ function(go_library TARGET_NAME)
"./${CMAKE_CURRENT_SOURCE_REL_DIR}/${GO_SOURCE}"
# must run under GOPATH
WORKING_DIRECTORY "${PADDLE_IN_GOPATH}/go")
add_dependencies(${TARGET_NAME} go_vendor)
endfunction(go_library)

function(go_binary TARGET_NAME)
Expand All @@ -325,7 +324,7 @@ function(go_binary TARGET_NAME)
-o "${CMAKE_CURRENT_BINARY_DIR}/${TARGET_NAME}"
"./${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}_timestamp ${go_binary_DEPS})
add_custom_target(${TARGET_NAME} ALL DEPENDS ${TARGET_NAME}_timestamp ${go_binary_DEPS})
install(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${TARGET_NAME} DESTINATION bin)
endfunction(go_binary)

Expand All @@ -335,7 +334,7 @@ function(go_test TARGET_NAME)
set(multiValueArgs DEPS)
cmake_parse_arguments(go_test "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
string(REPLACE "${PADDLE_GO_PATH}" "" CMAKE_CURRENT_SOURCE_REL_DIR ${CMAKE_CURRENT_SOURCE_DIR})
add_custom_target(${TARGET_NAME} ALL DEPENDS go_vendor ${go_test_DEPS})
add_custom_target(${TARGET_NAME} ALL DEPENDS ${go_test_DEPS})
add_custom_command(TARGET ${TARGET_NAME} POST_BUILD
COMMAND env GOPATH=${GOPATH} ${CMAKE_Go_COMPILER} test
-c -o "${CMAKE_CURRENT_BINARY_DIR}/${TARGET_NAME}"
Expand Down
1 change: 0 additions & 1 deletion paddle/scripts/docker/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ cmake .. \
-DWITH_STYLE_CHECK=${WITH_STYLE_CHECK:-OFF} \
-DWITH_TESTING=${WITH_TESTING:-OFF} \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON

cat <<EOF
========================================
Building in /paddle/build ...
Expand Down