Skip to content
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

Fixing issue on linking user module libraries with recent GCC versions #871

Merged
merged 3 commits into from Feb 13, 2018

Conversation

@jgarridoalcazar
Copy link
Contributor

@jgarridoalcazar jgarridoalcazar commented Dec 11, 2017

This changes are intended to fix issue #869.

It explicitely defines "-Wl,--no-as-needed" linking flag on nest executable, nest library and pynest library. By default, recent versions of GCC set --as-needed flag. This option allows the linker to ignore, i.e., not link against, some of the libraries supplied on its command line if they are not actually used by the shared library that is being created. This option avoids external modules to be linked in NEST even if they are defined with -Dexternal-modules=....

By setting --no-as-needed GCC will link against all the libraries explicitely indicated (including external modules). It may produce slower executable/library startup if more libraries are added than required.

This PR requires PR #870 to be added in order to fix issue #868.

This PR has only been tested with GCC 5.4.0 and Ubuntu 16.04. It should be tested on different compilers and OS.

This changes are intended to fix issue #869.

It explicitely defines "-Wl,--no-as-needed" linking flag on nest executable, nest library and pynest library. By default, recent versions of GCC set --as-needed flag. This option allows the linker to ignore, i.e., not link against, some of the libraries supplied on its command line if they are not actually used by the shared library that is being created. This option avoids external modules to be linked in NEST even if they are defined with -Dexternal-modules=....

By setting --no-as-needed GCC will link against all the libraries explicitely indicated (including external modules). It may produce slower executable/library startup if more libraries are added than required.

This PR requires PR #870 to be added in order to fix issue #868.

This PR has only been tested with GCC 5.4.0 and Ubuntu 16.04. It should be tested on different compilers and OS.
Copy link
Contributor

@hakonsbm hakonsbm left a comment

Thank you for fixing this. It looks mostly good to me, except for a couple trailing spaces. Also, I can confirm that it works using GCC 6.3.0 on Ubuntu 16.04.

@@ -31,10 +31,16 @@ if ( IS_BLUEGENE AND static-libraries )
)
endif ()

set_target_properties( nest
PROPERTIES
LINK_FLAGS "-Wl,--no-as-needed"

This comment has been minimized.

@hakonsbm

hakonsbm Jan 2, 2018
Contributor

Remove trailing space.

add_library( nest_lib ${nest_sources} )
set_target_properties( nest_lib
PROPERTIES
OUTPUT_NAME nest
LINK_FLAGS "-Wl,--no-as-needed"

This comment has been minimized.

@hakonsbm

hakonsbm Jan 2, 2018
Contributor

Remove trailing space here too.

@jgarridoalcazar
Copy link
Contributor Author

@jgarridoalcazar jgarridoalcazar commented Jan 8, 2018

Thanks @hakonsbm for reviewing this PR. The trailling spaces have already been removed.

Copy link
Contributor

@hakonsbm hakonsbm left a comment

Great! 👍

Copy link
Contributor

@heplesser heplesser left a comment

@jgarridoalcazar The changes are fine in principle, but they break NEST builds under OSX, so --as-needed must not be added when building for OSX. I am not sure if the flag works on special systems such as BlueGene or K. The best way would be a test to check if the linker on a given system accepts the flag, but I am not sure how to implement that in CMake.

@@ -31,10 +31,16 @@ if ( IS_BLUEGENE AND static-libraries )
)
endif ()

set_target_properties( nest
PROPERTIES
LINK_FLAGS "-Wl,--no-as-needed"

This comment has been minimized.

@heplesser

heplesser Jan 9, 2018
Contributor

This flag is not supported by the Apple linker, so you need to protect this by an if ( NOT APPLE ) ... endif(). I am also not sure about the linkers for more special systems, such as BlueGene and K. Maybe the best thing would be to add a test whether --no-as-needed is supported by the linker and only apply it if it is. Unfortunately, this does not seem straightforward in CMake, see https://public.kitware.com/pipermail/cmake/2016-January/062623.html; we need to support older CMake versions, back to 2.8.12.

add_library( nest_lib ${nest_sources} )
set_target_properties( nest_lib
PROPERTIES
OUTPUT_NAME nest
LINK_FLAGS "-Wl,--no-as-needed"

This comment has been minimized.

@heplesser

heplesser Jan 9, 2018
Contributor

See above.

@@ -46,8 +46,9 @@ if ( HAVE_PYTHON )
endif ()
python_add_module( pynestkernel ${pynestkernel_generated_file} pynestkernel.pxd )
if ( APPLE )
set_target_properties( pynestkernel PROPERTIES LINK_FLAGS "-undefined dynamic_lookup" )
set_target_properties( pynestkernel PROPERTIES LINK_FLAGS "-undefined dynamic_lookup -Wl,--no-as-needed" )

This comment has been minimized.

@heplesser

heplesser Jan 9, 2018
Contributor

See above, leave the Apple-case unchanged.

This commit fixed the broken link in OSX systems due to --no-as-needed linker flag. The flag is now added only in non-apple systems.

Fixing bug in #869.
@jgarridoalcazar
Copy link
Contributor Author

@jgarridoalcazar jgarridoalcazar commented Jan 9, 2018

You are right, @heplesser . Removing the --no-as-needed flag in OSX systems should not be an issue since you already tested this bug in OSX and it was not reproducible (see the mailing list). After the last commit the flag is only added to non-Apple systems. It could be very helpful if anyone having access to BlueGene and/or K could test the building in these systems.

@jougs
Copy link
Contributor

@jougs jougs commented Jan 12, 2018

I have just tested the branch on BlueGene/Q (JUQUEEN) and it worked flawlesly. @heplesser: can you please confirm that it now also works on OS X so we can merge? Thanks!

@heplesser
Copy link
Contributor

@heplesser heplesser commented Feb 13, 2018

@jgarridoalcazar Works like a charm on macOS as well now (requires #870 merged, which is in place now). Thanks!

@heplesser heplesser merged commit e2e58c0 into nest:master Feb 13, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jgarridoalcazar jgarridoalcazar deleted the jgarridoalcazar:fix-ext-module-link-flags branch Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants