-
Notifications
You must be signed in to change notification settings - Fork 276
Makefile modifications required for gcc >= 4.9 (fixes #981) #984
Changes from 2 commits
353a589
3582762
dc3b01b
bb33744
d2a639f
5c4eaef
089e13a
7028279
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,8 +43,35 @@ set(aprlib_cflags "${EXTERNAL_C_FLAGS_OPTIMIZED} ${COMMON_COMPILER_DEFINITIONS_S | |
# Location of apr sources | ||
set(aprlib_url "${REPOSITORY_DIR}/external/common/share/apr/unix/apr-1.5.2.tar.gz") | ||
|
||
# gcc v4.9 requires its own binutils-wrappers for LTO (flag -flto) | ||
# fixes #981 | ||
IF(${CMAKE_SYSTEM_NAME} MATCHES "Linux" AND (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER "4.9" OR CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL "4.9")) | ||
set(aprlib_config_options --enable-static --disable-shared --disable-ipv6) | ||
|
||
if("${CMAKE_BUILD_TYPE}" STREQUAL "Debug") | ||
set(aprlib_config_options ${aprlib_config_options} --enable-debug) | ||
endif() | ||
|
||
ExternalProject_Add(Apr1StaticLib | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just add this (and below) under the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. I'll update the fix. |
||
URL ${aprlib_url} | ||
|
||
UPDATE_COMMAND "" | ||
PATCH_COMMAND "" | ||
|
||
CONFIGURE_COMMAND | ||
<SOURCE_DIR>/configure AR=gcc-ar NM=gcc-nm RANLIB=gcc-ranlib | ||
--prefix=${aprlib_install_prefix} | ||
${aprlib_config_options} | ||
CFLAGS=${aprlib_cflags} | ||
|
||
BUILD_COMMAND | ||
make -f Makefile all | ||
|
||
INSTALL_COMMAND | ||
make -f Makefile install | ||
) | ||
# Get it built! | ||
if (UNIX) | ||
elseif (UNIX) | ||
set(aprlib_config_options --enable-static --disable-shared --disable-ipv6) | ||
|
||
if("${CMAKE_BUILD_TYPE}" STREQUAL "Debug") | ||
|
@@ -89,7 +116,6 @@ else() | |
-DCMAKE_INSTALL_PREFIX=${aprlib_install_prefix} | ||
-DINSTALL_PDB=OFF | ||
|
||
|
||
#LOG_INSTALL 1 | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,13 @@ project(nupic_core_main CXX) | |
|
||
set(CMAKE_VERBOSE_MAKEFILE OFF) | ||
|
||
# gcc v4.9 requires its own binutils-wrappers for LTO (flag -flto) | ||
# fixes #981 | ||
IF(${CMAKE_SYSTEM_NAME} MATCHES "Linux" AND (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER "4.9" OR CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL "4.9")) | ||
set(CMAKE_AR "/usr/bin/gcc-ar") | ||
set(CMAKE_RANLIB "/usr/bin/gcc-ranlib") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both of these variables were already set in the top-level CMakeLists.txt. Shouldn't need to set here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, changes in external/CMakeLists.txt are obsolete. Will change that. |
||
ENDIF() | ||
|
||
set_directory_properties(PROPERTIES EP_BASE "${EP_BASE}") | ||
|
||
# Shorter aliases for static library prefix and suffix. | ||
|
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.
I think that the intention was that these variables -
CMAKE_RANLIB
andCMAKE_AR
would be used in other modules, but the variables aren't used anywhere, and each of the modules presently uses "/usr/bin/gcc-ar" and "/usr/bin/gcc-ranlib" verbatim, which doesn't sound right.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.
Will remove the absolute path in CMAKE_AR and CMAKE_RANLIB.
Both variables seem to be used in the config process (if you remove the variables, you'll get a lot of "lto plugin needed" errors and build fails).
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 absolute paths for "/usr/bin/gcc-ar" and "/usr/bin/gcc-ranlib" may be a bad idea, since it only works if the toolchain is installed using those absolute paths versus relying on the environment. Those absolute paths may work in your deployment, but could fail for others. Also, your changes in Apr1Lib.cmake and AprUtil1Lib.cmake don't use absolute paths.
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.
Since you have these variables defined at root level, are the explicit -DCMAKE_AR= and -DCMAKE_RANLIB= in external .cmake files actually needed?
Perhaps it would suffice to set up
CMAKE_AR
andCMAKE_RANLIB
in CommonCompilerConfig.cmake, with the exception of Apr1 and AprUtil1 external libs, which are configure-based and don't use cmake? In that case, you probably won't need theCOMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED
suggested in my comment #984 (comment).