Skip to content

Conversation

@fuchsto
Copy link
Member

@fuchsto fuchsto commented May 16, 2017

When linking DART/DASH libraries in a shared library (as required for pydash bindings), static libraries must be build with -fPIC (Position Independent Code).

Using PIC per default increases library filesize, but it shouldn't hurt to enable it by default.

Also, with @fmoessbauer 's changes in dash::Array, the build broke when logging was enabled as no stream conversion for std::unique_ptr is defined. Fixed by removing a log message.

@fuchsto fuchsto self-assigned this May 16, 2017
this, std::bind(&Array::deallocate, this));
// Actual destruction of the array instance:
DASH_LOG_TRACE_VAR("Array.deallocate()", m_globmem);
// DASH_LOG_TRACE_VAR("Array.deallocate()", m_globmem);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the commented out code.

${DASH_DART_BASE_LIBRARY}
PROPERTIES POSITION_INDEPENDENT_CODE TRUE
)

Copy link
Member

Choose a reason for hiding this comment

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

Fine with me, couldn't see a performance impact significant to stick out of the noise in my experiments. That might be different on 32bit systems, though (in case anyone still uses them for real).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmyes. I'd prefer a more canonical way myself to toggle this setting, like BUILD_SHARED_LIBS, but I'm not aware of any. Still, will wrap it in a build option BUILD_PIC_LIBS and disable it by default. It's sensible to assume that the common use case is to link DASH with an executable.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Will pydash be integrated into the main build system? If so, we might just trigger PIC builds if pydash is being built.

@devreal devreal merged commit 9e5c611 into development Feb 19, 2018
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.

3 participants