Makefile modifications required for gcc >= 4.9 (fixes #981) #984

ghost · 2016-06-18T10:40:37Z

Fixes #981

This fix adds conditional cmake parameters that are required to build nupic.core with gcc version 4.9 (and higher) in combination with LTO (-flto). gcc >= 4.9 requires to use its own wrappers for ar, nm and ranlib instead of the regular binutils versions.

numenta-ci · 2016-06-18T10:40:46Z

By analyzing the blame information on this pull request, we identified @vitaly-krugl and @scottpurdy to be potential reviewers

ghost · 2016-06-18T11:18:46Z

For the records: Don't try to use conditional statements within ExternalProject_Add() functions.

rhyolight · 2016-06-18T15:31:40Z

I want to test this with Ubuntu 15 / 16 Monday. I will also ask @vitaly-krugl and @breznak to review.

ghost · 2016-06-18T16:30:04Z

Great, thank you! I haven't had the chance to test the build in an Ubuntu 16.04./gcc 5.x environment yet, but a build on Ubuntu 15.10 amd64 (4.2.0-38-generic #45-Ubuntu SMP) with gcc version 4.9.3 performed well with all tests passed.

ghost · 2016-06-18T22:08:43Z

Just tested:

Ubuntu 16.04 amd64 fresh install w/ latest official updates (4.4.0-24-generic #43-Ubuntu SMP)
gcc version 5.3.1 20160413 (Ubuntu 5.3.1-14ubuntu2.1)
cmake version 3.5.1

Standard config/build procedure:
cmake $NUPIC_CORE -DCMAKE_INSTALL_PREFIX=../release -DPY_EXTENSIONS_DIR=$NUPIC_CORE/bindings/py/nupic/bindings
make
=> Build successful.

Tests
$NUPIC_CORE/build/release/bin/cpp_region_test
$NUPIC_CORE/build/release/bin/unit-tests
=> all pass.

@rhyolight If you can confirm this on Monday, this means that nupic.core now builds on gcc versions 4.9 and 5.x (and the latest Ubuntu LTS version).

breznak · 2016-06-20T09:48:47Z

external/Apr1Lib.cmake

+        set(aprlib_config_options ${aprlib_config_options} --enable-debug)
+    endif()
+
+    ExternalProject_Add(Apr1StaticLib


Maybe just add this (and below) under the UNIX IF() and add IF(gcc == 4.9)?

Good idea. I'll update the fix.

breznak · 2016-06-20T09:51:20Z

Great work @hstm ! We needed such patch very much for the coming OSes 👍
I'll be able to test later this week, but you've showed it runs well.

rhyolight · 2016-06-20T14:08:47Z

I just confirmed that this works on ubuntu 15 with GCC 4.9 and 5.2, and that's good enough for a 👍 from me. Would still like for @vitaly-krugl to review, if possible.

vitaly-krugl · 2016-06-20T18:31:37Z

Reviewing ... will add a comment when I'm done

vitaly-krugl · 2016-06-20T18:45:26Z

Pls fix indentations - use spaces instead of tabs. There are many tabs in the PR.

vitaly-krugl · 2016-06-20T18:46:19Z

@hstm, please hold off on pushing new commits to this PR until I let you know that I am completely done with this round of code review. Thx.

vitaly-krugl · 2016-06-20T18:54:22Z

external/CMakeLists.txt

+# fixes #981
+IF(UNIX AND CMAKE_COMPILER_IS_GNUCXX 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")


Both of these variables were already set in the top-level CMakeLists.txt. Shouldn't need to set here.

You're right, changes in external/CMakeLists.txt are obsolete. Will change that.

vitaly-krugl · 2016-06-20T21:52:01Z

Since the new parameters are tied to LTO, and LTO optimization flags are set up in https://github.com/numenta/nupic.core/blob/master/CommonCompilerConfig.cmake, I think it's reasonable to expect that the correlated parameters should be set up in the same file for readability and maintainability. Here is what I am thinking:

In CommonCompilerConfig.cmake:
1. Export a new list variable COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED (default to empty list) that contains -DCMAKE_AR=/usr/bin/gcc-ar and -DCMAKE_RANLIB=/usr/bin/gcc-ranlib if supported by the compiler toolchain (compiler version check, GNU compiler, etc.) and if optimizations are enabled (non-debug build). This should track the presence of lto-related flags in optimization_flags_cc - see https://github.com/numenta/nupic.core/blob/8b6caad44b3593e2c29b91c194cc619114eb9c6a/CommonCompilerConfig.cmake#L170 and https://github.com/numenta/nupic.core/blob/8b6caad44b3593e2c29b91c194cc619114eb9c6a/CommonCompilerConfig.cmake#L240 (here it's reset if build type is "Debug"). By the way, the absolute paths may be a bad idea, since it only works if the toolchain is installed using those absolute paths versus relying on the environment (also, your changes in Apr1Lib.cmake and AprUtil1Lib.cmake don't use an absolute path).
2. Export a new string variable COMMON_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR (default to empty string) that contains AR=gcc-ar RANLIB=gcc-ranlib (and NM=gcc-nm if it's really necessary???) for use in configure-based external project builds (see Apr1Lib.cmake and AprUtil1Lib.cmake). Its value should track COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED.
Add ${COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED} to the cmake args lists of libraries that use the optimized C or CXX flags, similarly to how ${CAPNP_CMAKE_DEFINITIONS} is used in CapnProto. Note that CapnProto does't used optimized compiler flags, so it doesn't need COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED.
Update Apr1Lib.cmake and AprUtil1Lib.cmake to make use of the new variable COMMON_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR

This should also simplify many of the changes in external .cmake files. In particular, it should eliminate the duplications in the external .cmake files.

vitaly-krugl · 2016-06-20T22:22:23Z

Are AR and RANLIB set properly for src_lib_static_nupiccore_solo by cmake add_library tooling? (reference https://github.com/numenta/nupic.core/blob/eaa5fefa86c6c6c18b89e9189dcc8d1e18346713/src/CMakeLists.txt#L366)

Or is this resolved by the new variables CMAKE_AR and CMAKE_RANLIB in the root CMakeLists.txt? And if so, then are the explicit -DCMAKE_AR= and -DCMAKE_RANLIB= in external .cmake files actually needed?

vitaly-krugl · 2016-06-20T22:25:52Z

@hstm, thank you for your work in figuring out the necessary parameters. I am done with the first round of code review. Please see my feedback.

CC @rhyolight @scottpurdy

vitaly-krugl · 2016-06-20T22:28:00Z

There may be eventual merge and logic conflicts with #975.

ghost · 2016-06-20T22:44:16Z

@vitaly-krugl Thanks for the review!

ghost · 2016-06-21T11:20:13Z

removed redundancies by introducing two new variables in CommonCompilerConfig.cmake

COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED
COMMON_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR

CMAKE_AR and CMAKE_RANLIB are now set in root level CMakeLists.txt only
removed absolute paths

Notes:

Seems we need both settings in CMakeLists.txt and CommonCompilerConfig.cmake for the build to succeed, otherwise CMAKE_AR and CMAKE_RANLIB won't be propagated correctly (this also makes sure that AR and RANLIB are set properly for src_lib_static_nupiccore_solo by cmake add_library tooling).
We don't need to check if LTO is enabled in optimization_flags_cc since nupic.core builds fine with the gcc-*-wrappers even with LTO disabled.
Successfully built and tested with gcc 4.9.x (Ubuntu 15.10) and gcc 5.x (Ubuntu 16.04)

@vitaly-krugl Could you please have a look at the changes? Thx.

vitaly-krugl · 2016-06-21T21:18:06Z

CommonCompilerConfig.cmake

@@ -91,6 +91,8 @@ set(EXTERNAL_CXX_FLAGS_OPTIMIZED)
 set(EXTERNAL_LINKER_FLAGS_UNOPTIMIZED)
 set(EXTERNAL_LINKER_FLAGS_OPTIMIZED)

+set(COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED)
+set(COMMON_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR "")


The file header contains documentation of each variable exported by this module. Pls document the two new variables there, too.

vitaly-krugl · 2016-06-21T21:33:08Z

@hstm, I noticed that the new COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED parameters are not propagated to CapnProto.cmake. Yet, I assume that nupic.core built successfully for you in the "gcc >= 4.9" environment. If so, this might confirm that setting CMAKE_AR and CMAKE_RANLIB in the root CMakeLists.txt propagates the values to all externals, making COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED unnecessary. As far as I remember, cmake variable values propagate from higher-level to lower-level in the hierarchy. If so, then you only need to set CMAKE_AR and CMAKE_RANLIB in CommonCompilerConfig.cmake and don't need COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED. However, you would still need COMMON_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR, since the Apache Portable Runtime (Apr) library builds are not using cmake under the covers (and thus are not aware of CMAKE_AR and CMAKE_RANLIB), so we still need to pass COMMON_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR to their configure scripts.

vitaly-krugl · 2016-06-21T21:37:22Z

@hstm, I am done with the second round of code review. Please see my feedback. Thank you.

ghost · 2016-06-22T07:44:44Z

@vitaly-krugl Thank you very much for the review.

CapnProto is built without optimizations (EXTERNAL_CXX_FLAGS_UNOPTIMIZED), that means with LTO disabled, so we don't have to use COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED there.

Just tried a build with gcc 4.9.3 and COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED unset:

[ 62%] Building C object CMakeFiles/yaml.dir/src/scanner.c.o
[ 75%] Building C object CMakeFiles/yaml.dir/src/dumper.c.o
[ 87%] Building C object CMakeFiles/yaml.dir/src/api.c.o
[100%] Building C object CMakeFiles/yaml.dir/src/writer.c.o
Linking C static library libyaml.a
/usr/bin/ar: CMakeFiles/yaml.dir/src/parser.c.o: plugin needed to handle lto object
/usr/bin/ar: CMakeFiles/yaml.dir/src/emitter.c.o: plugin needed to handle lto object
/usr/bin/ar: CMakeFiles/yaml.dir/src/loader.c.o: plugin needed to handle lto object
/usr/bin/ar: CMakeFiles/yaml.dir/src/reader.c.o: plugin needed to handle lto object

Of course ar should be gcc-ar in this case, otherwise the build fails. This indicates that CMAKE_AR and CMAKE_RANLIB set in the root CMakeLists.txt aren't propagated correctly to the externals (maybe this is a bug, see https://cmake.org/Bug/view.php?id=15547).

As a consequence, we have to keep the variables in CMakeLists.txt as well as both COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED and
COMMON_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR in CommonCompilerConfig.cmake.

ghost · 2016-06-22T11:36:57Z

@vitaly-krugl We could possibly add ${COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED} to CapnProto.cmake although it's not required right now. It wouldn't hurt (we'd add just one line of code) and we'd make sure everything still builds in case somebody switches the CapnProto build from EXTERNAL_CXX_FLAGS_UNOPTIMIZED to EXTERNAL_CXX_FLAGS_OPTIMIZED (which could happen one day). What do you think?

rhyolight · 2016-06-22T15:01:51Z

We could possibly add ${COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED} to CapnProto.cmake although it's not required right now. It wouldn't hurt (we'd add just one line of code) and we'd make sure everything still builds in case somebody switches the CapnProto build from EXTERNAL_CXX_FLAGS_UNOPTIMIZED to EXTERNAL_CXX_FLAGS_OPTIMIZED (which could happen one day). What do you think?

@scottpurdy could have an opinion on this.

vitaly-krugl · 2016-06-22T20:25:02Z

@hstm, regarding

@vitaly-krugl We could possibly add ${COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED} to CapnProto.cmake although it's not required right now. It wouldn't hurt (we'd add just one line of code) and we'd make sure everything still builds in case somebody switches the CapnProto build from EXTERNAL_CXX_FLAGS_UNOPTIMIZED to EXTERNAL_CXX_FLAGS_OPTIMIZED (which could happen one day). What do you think?

Let's leave COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED out of CapnProto. We don't want to mix unoptimized with optimized settings. Who knows what else might be added to COMMON_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED in the future?

vitaly-krugl · 2016-06-22T20:38:29Z

CommonCompilerConfig.cmake

@@ -220,6 +229,12 @@ elseif(${CMAKE_CXX_COMPILER_ID} MATCHES "Clang")
  set(shared_linker_flags_unoptimized "${shared_linker_flags_unoptimized} -Wl,-undefined,error")
 endif()

+# Compatibility with gcc >= 4.9 which requires the use of gcc's own wrappers for ar and ranlib in combination with LTO
+# works also with LTO disabled
+IF(UNIX AND CMAKE_COMPILER_IS_GNUCXX AND (NOT "${CMAKE_BUILD_TYPE}" STREQUAL "Debug") AND (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER "4.9" OR CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL "4.9"))


To avoid duplication of logic, let's set CMAKE_AR and CMAKE_RANLIB here as well, instead of doing it separately in the root CMakeLists.txt.

Also, add a short explanatory comment to the effect that the repetition of these settings in EXTERNAL_STATICLIB_CMAKE_DEFINITIONS_OPTIMIZED and EXTERNAL_STATICLIB_CONFIGURE_DEFINITIONS_OPTIMIZED_STR are a workaround for a cmake bug related to propagation of CMAKE_AR (including a link to https://gitlab.kitware.com/cmake/cmake/issues/15547).

Also, document CMAKE_AR and CMAKE_RANLIB alongside the CMAKE_LINKER doc there.

vitaly-krugl · 2016-06-22T21:00:15Z

@hstm, thanks for tracking down the CMAKE_AR bug. We're almost there. I have just a couple of simple feedback items this time.

scottpurdy · 2016-06-22T21:37:12Z

@rhyolight I'll defer to @vitaly-krugl on this discussion.

ghost · 2016-06-22T21:43:49Z

@vitaly-krugl Thanks for the review. I've just pushed the updated files:

root CMakelists.txt cleaned up, CMAKE_AR and CMAKE_RANLIB moved to CommonCompilerConfig.cmake
variables renamed to EXTERNAL*
documentation updated

Successfully tested with gcc 4.9.3 and 5.3.1.

vitaly-krugl · 2016-06-22T21:46:30Z

CommonCompilerConfig.cmake

 # INTERNAL_CXX_FLAGS_OPTIMIZED: string of C++ flags with explicit optimization flags for internal sources
 #
 # INTERNAL_LINKER_FLAGS_OPTIMIZED: string of linker flags for linking internal executables
 #                      and shared libraries (DLLs) with optimizations that are
 #                      compatible with INTERNAL_CXX_FLAGS_OPTIMIZED
-#
+#                    
 # CMAKE_LINKER: updated, if needed; use ld.gold if available. See cmake
 #               documentation
 #


Pls add blurbs for CMAKE_AR and CMAKE_RANLIB in this documentation.

vitaly-krugl · 2016-06-22T21:48:30Z

@hstm, LGTM after adding blurbs for CMAKE_AR and CMAKE_RANLIB in CommonCompilerConfig.cmake header (near the blurb for CMAKE_LINKER). Thanks!

ghost · 2016-06-22T21:54:39Z

@vitaly-krugl Done.

vitaly-krugl · 2016-06-22T21:56:59Z

@hstm, thank you for your effort and patience.

@rhyolight: LGTM at 7028279 as soon as all tests pass.

vitaly-krugl · 2016-06-22T23:44:44Z

@hstm, this PR is merged - thank you!

I verified the build and tests on Ubuntu 16.04 running in VirtualBox; gcc v5.3.1, python v2.7.11+.

CC @rhyolight

rhyolight · 2016-06-23T16:23:41Z

Yes! Great work you all!

ghost · 2016-06-23T17:31:25Z

@vitaly-krugl Once again thank you for the reviews - the patch now looks much prettier than in my first version ;-)

Helge Stahlmann added 2 commits June 17, 2016 14:11

fixes #981

353a589

updated conditional cmake_args, fixes #981

3582762

rhyolight self-assigned this Jun 18, 2016

ghost changed the title ~~Makefile modifications required for gcc-4.9 (fixes #981)~~ Makefile modifications required for gcc >= 4.9 (fixes #981) Jun 18, 2016

ghost mentioned this pull request Jun 18, 2016

Linux 32b nupic.core build ends with "recipe for target 'helloregion' failed" #956

Closed

rcrowder mentioned this pull request Jun 19, 2016

Rc Raspberry Pi 3 Build (#955) #975

Merged

breznak reviewed Jun 20, 2016
View reviewed changes

Helge Stahlmann added 2 commits June 20, 2016 12:56

fix now valid for all UNIX-like OSes, cleanup

dc3b01b

make sure fix is applied for gcc only

bb33744

rhyolight added the status:ready label Jun 20, 2016

vitaly-krugl reviewed Jun 20, 2016
View reviewed changes

cleanup after code review, removed redundancies

d2a639f

vitaly-krugl reviewed Jun 21, 2016
View reviewed changes

added documentation for new variables

5c4eaef

vitaly-krugl reviewed Jun 22, 2016
View reviewed changes

CMakeLists.txt cleaned up, vars renamed, docs updated

089e13a

vitaly-krugl reviewed Jun 22, 2016
View reviewed changes

added blurps for CMAKE_AR and CMAKE_RANLIB

7028279

vitaly-krugl merged commit a5d7d8f into numenta:master Jun 22, 2016

ghost deleted the gcc4.9-fix branch June 23, 2016 20:24

Makefile modifications required for gcc >= 4.9 (fixes #981) #984

Makefile modifications required for gcc >= 4.9 (fixes #981) #984

Conversation

ghost commented Jun 18, 2016

numenta-ci commented Jun 18, 2016

ghost commented Jun 18, 2016

rhyolight commented Jun 18, 2016

ghost commented Jun 18, 2016 • edited by ghost

ghost commented Jun 18, 2016

breznak Jun 20, 2016

Choose a reason for hiding this comment

ghost Jun 20, 2016

Choose a reason for hiding this comment

breznak commented Jun 20, 2016

rhyolight commented Jun 20, 2016

vitaly-krugl commented Jun 20, 2016

vitaly-krugl commented Jun 20, 2016

vitaly-krugl commented Jun 20, 2016

vitaly-krugl Jun 20, 2016

Choose a reason for hiding this comment

ghost Jun 20, 2016

Choose a reason for hiding this comment

vitaly-krugl commented Jun 20, 2016 • edited

vitaly-krugl commented Jun 20, 2016 • edited

vitaly-krugl commented Jun 20, 2016 • edited

vitaly-krugl commented Jun 20, 2016

ghost commented Jun 20, 2016

ghost commented Jun 21, 2016 • edited by ghost

vitaly-krugl Jun 21, 2016

Choose a reason for hiding this comment

vitaly-krugl commented Jun 21, 2016

vitaly-krugl commented Jun 21, 2016

ghost commented Jun 22, 2016

ghost commented Jun 22, 2016

rhyolight commented Jun 22, 2016

vitaly-krugl commented Jun 22, 2016 • edited

vitaly-krugl Jun 22, 2016 • edited

Choose a reason for hiding this comment

vitaly-krugl commented Jun 22, 2016

scottpurdy commented Jun 22, 2016

ghost commented Jun 22, 2016

vitaly-krugl Jun 22, 2016

Choose a reason for hiding this comment

vitaly-krugl commented Jun 22, 2016

ghost commented Jun 22, 2016

vitaly-krugl commented Jun 22, 2016 • edited

vitaly-krugl commented Jun 22, 2016 • edited

rhyolight commented Jun 23, 2016

ghost commented Jun 23, 2016

ghost commented Jun 18, 2016 •

edited by ghost

vitaly-krugl commented Jun 20, 2016 •

edited

vitaly-krugl commented Jun 20, 2016 •

edited

vitaly-krugl commented Jun 20, 2016 •

edited

ghost commented Jun 21, 2016 •

edited by ghost

vitaly-krugl commented Jun 22, 2016 •

edited

vitaly-krugl Jun 22, 2016 •

edited

vitaly-krugl commented Jun 22, 2016 •

edited

vitaly-krugl commented Jun 22, 2016 •

edited