-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Remove duplicated linking of libraries. #4890
Conversation
cmake/cross_compiling/ios.cmake
Outdated
@@ -247,7 +247,8 @@ endif() | |||
|
|||
set(IOS_COMPILER_FLAGS "${XCODE_IOS_PLATFORM_VERSION_FLAGS} ${XCODE_IOS_BITCODE_FLAGS}") | |||
|
|||
# Hidden visibilty is required for cxx on iOS | |||
# Hidden visibilty is required for c++ on iOS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
那么到底应该 hide visibility 呢还是不应该呢?
Hidden visibilty is required for c++ on iOS.
的意思是需要
Remove -fvisibility=hidden because warp-ctc failed with it.
的意思是不需要
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
没有hide visibility这个flags,Paddle能成功编译,但是在链接时将会产生很多warning。
加上这个flag之后,warp-ctc编译fail了,所以暂时去掉这个flag,后面再来修复。
注释写的不太好,我修复一下。:smile:
cmake/external/glog.cmake
Outdated
@@ -55,7 +55,6 @@ ExternalProject_Add( | |||
ADD_LIBRARY(glog STATIC IMPORTED GLOBAL) | |||
SET_PROPERTY(TARGET glog PROPERTY IMPORTED_LOCATION ${GLOG_LIBRARIES}) | |||
ADD_DEPENDENCIES(glog extern_glog gflags) | |||
LINK_LIBRARIES(glog gflags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for curiosity, why don't we need LINK_LIBRARIES
anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LINK_LIBRARIES
statement will enable the link of gflags
and glog
to all executables. I add the statement back, because I found many modules use glog
but not add glog
to their DEPS
in cc_library
.
endif() | ||
endforeach() | ||
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") | ||
if(NOT IOS_ENABLE_BITCODE) | ||
list(APPEND LIBS "-undefined dynamic_lookup") | ||
endif() | ||
endif() | ||
list(REVERSE libsInArgn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for curiosity, why don't we need list(REVERSE libsInArgn)
anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As shown in #4892, on Mac platform I found so many libraries are linked twice, one in a given order, another in the reversed order.
-Wl,-force_load ../../gserver/libpaddle_gserver.a -Wl,-force_load ../../function/libpaddle_function.a ../../pserver/libpaddle_pserver.a ../../trainer/libpaddle_trainer_lib.a ../../pserver/libpaddle_network.a ../../math/libpaddle_math.a ../libpaddle_utils.a ../../parameter/libpaddle_parameter.a ../../../proto/libpaddle_proto.a ../../cuda/libpaddle_cuda.a ../../optimizer/libpaddle_optimizer.a
../../../../third_party/install/gflags/lib/libgflags.a
../../../../third_party/install/glog/lib/libglog.a
/usr/local/opt/openblas/lib/libopenblas.dylib
../../../../third_party/install/protobuf/lib/libprotobuf.a
../../../../third_party/install/zlib/lib/libz.a
/usr/lib/libpython2.7.dylib
-undefined dynamic_lookup
/usr/lib/libpython2.7.dylib
../../../../third_party/install/zlib/lib/libz.a
../../../../third_party/install/protobuf/lib/libprotobuf.a
/usr/local/opt/openblas/lib/libopenblas.dylib
../../../../third_party/install/glog/lib/libglog.a
../../../../third_party/install/gflags/lib/libgflags.a
../../optimizer/libpaddle_optimizer.a ../../cuda/libpaddle_cuda.a ../../../proto/libpaddle_proto.a ../../parameter/libpaddle_parameter.a ../libpaddle_utils.a ../../math/libpaddle_math.a ../../pserver/libpaddle_network.a ../../trainer/libpaddle_trainer_lib.a ../../pserver/libpaddle_pserver.a ../../function/libpaddle_function.a ../../gserver/libpaddle_gserver.a
Here, LIBS
contains all dependent libraries in given order, libsInArgn
contains all the dependent in the reverse order. The reverse is done by line 32
. And both LIBS
and libsInArgn
are linked to TARGET_NAME
. I don't know why it was written like this. When I remove libsInArgn
, this PR also works on Mac.
@@ -21,7 +21,6 @@ | |||
#include <thrust/system/cuda/experimental/pinned_allocator.h> | |||
#endif | |||
|
|||
#include <glog/logging.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the very detail-oriented work style -- moving the inclusion of glog header file into the .cu file.
@@ -1,582 +0,0 @@ | |||
# Ceres Solver - A fast non-linear least squares minimizer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So glad to see that we can remove this very long source file!
Just for curiosity, why could we remove it? I noticed that in this change to the CMakeLists.txt file, we don't need to specify that a target depends on glog, but only on gflags. Is this related with the removal of this FindGFlags.cmake
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two files, FindGflags.cmake
and FindGlog.cmake
, are used to find the glog
and gflags
installed on the system. Since we always use ExternalProject_Add
to download and compile those two libraries, the two cmake files actually are not used in Paddle's cmake system. So, they are safe to remove. We can use grep -r "find_package(Gflags)"
and grep -r "find_package(Glog)"
to check this.
The reason of the change , removing DEPS
of glog
from cpu_info
and gpu_info
, is because no glog
functions are called in the codes.
…depend them but not add them to DEPS.
844c88d
to
5a8a81c
Compare
Fixes #4892