Skip to content

Enable shared linking#1133

Merged
bilke merged 5 commits intoufz:masterfrom
endJunction:EnableSharedLinking
May 4, 2016
Merged

Enable shared linking#1133
bilke merged 5 commits intoufz:masterfrom
endJunction:EnableSharedLinking

Conversation

@endJunction
Copy link
Copy Markdown
Member

@endJunction endJunction commented Apr 8, 2016

Fixing metis linkage; Remove STATIC from add_library().

With this changes and configuring the build for debug and with -DBUILD_SHARED_LIBS=On the compile + linking time for the ogs target (from scratch, no ccache) reduces (for that particular branch endJunction/M2 with dynamic eigen matrices) from 3:45 to 2:55.

The default build process should not change with this PR, because the static linking is chosen by default.

Update: On another machine there is no change, unfortunately, which I don't understand yet.

@chleh
Copy link
Copy Markdown
Collaborator

chleh commented Apr 9, 2016

Having the additional option of shared linking is good, in general.
However, could you please give some information on:

  • whether debugger starts faster/slower (just run ogs in the debugger w/o args)
  • some details of the build that gave the build time improvement from 3:45 to 2:55: Was it a clean build? Did you use ccache? (Probably the answers to these two questions are yes and no, respectively.)

At least this PR seems to clean up the cmake scripts a bit, so ⏩

@norihiro-w
Copy link
Copy Markdown
Collaborator

I couldn't compile this branch on my Mac with clang. So far I found two problems

Comment thread MathLib/CMakeLists.txt Outdated
)

if(METIS_FOUND)
target_link_libraries(MathLib INTERFACE metis)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use ${METIS_LIBRARY} ✅

@endJunction
Copy link
Copy Markdown
Member Author

@norihiro-w The undefined symbols and INTERFACE vs PRIVATE/PUBLIC is easily fixable. The circular dependencies are more difficult, I'm working.

I suggest to fix the first point, but leave the circular dependencies for further PRs.

@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented Apr 11, 2016

Build finished.

Comment thread MathLib/LinAlg/Solvers/BiCGStab.cpp Outdated
*/

#include "BiCGStab.h"
#include "../Sparse/CRSMatrix.h"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we allow .. in an include path? ✅

@endJunction
Copy link
Copy Markdown
Member Author

Before this PR is forgotten I propose following compromise: Static linking is removed for all but circularly dependent libraries until we are working on the resolution of those circular dependencies.

When a circular dependency is broken, the STATIC flag must be removed.

Further I propose to change one (or multiple) of the jenkins linux builds to dynamic linking, such that any new circular library dependencies can be recognized early on.

Comment thread scripts/cmake/CompilerSetup.cmake Outdated
set( CMAKE_INCLUDE_SYSTEM_FLAG_CXX "-isystem" CACHE STRING "" FORCE )
endif()

# When static libraries are going to be used in other libraries position
Copy link
Copy Markdown
Collaborator

@chleh chleh Apr 29, 2016

Choose a reason for hiding this comment

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

static libraries

I thought, this commit is about shared libs. (I'm not an expert, so maybe I'm simply confused)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

"... in other libraries..." means the shared libraries which are dependent on the static libraries.
Comment improvements are welcome.

Copy link
Copy Markdown
Collaborator

@chleh chleh Apr 29, 2016

Choose a reason for hiding this comment

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

When static libraries are used in some shared libraries it is required that
also the static libraries have position independent code.

@endJunction
Copy link
Copy Markdown
Member Author

I have added a Linux-PRs-dynamic test on jenkins building a debug config with shared libs. Currently testing, so if some of your PRs rise a red flag for Linux-PRs-dynamic ignore it for now.

@TomFischer
Copy link
Copy Markdown
Member

Ready for merge?!

@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented May 3, 2016

Jenkins: OGS-6/Gui/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Win-PRs/1673/

@endJunction
Copy link
Copy Markdown
Member Author

Rebased on ufz/master. It's ready if the tests are all green. Maybe one more time "Jenkins ... this please."...

@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented May 3, 2016

Jenkins: OGS-6/Gui/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Win-PRs/1674/

endJunction and others added 5 commits May 3, 2016 23:26
When building shared libraries -DBUILD_SHARED_LIBS=On must be passed to the
cmake invokation. Otherwise the default static linking applies.

This reduces the compile time, which is especially interesting for the debug
builds.
@bilke bilke merged commit 5026765 into ufz:master May 4, 2016
@endJunction endJunction deleted the EnableSharedLinking branch May 4, 2016 07:32
@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants