cjntaylor / node-cmake

CMake-based build system for node.js native modules
ISC License
78 stars 20 forks source link

Use set_target_properties instead of target_link_libraries to add LINK_FLAGS #20

Closed kkaefer closed 7 years ago

kkaefer commented 7 years ago

Since 2.0, node-cmake passes the LINK_FLAGS via target_link_libraries, but it should continue to use set_target_properties as per the CMake document:

Link flags specified here are inserted into the link command in the same place as the link libraries. This might not be correct, depending on the linker. Use the LINK_FLAGS target property to add link flags explicitly. The flags will then be placed at the toolchain-defined flag position in the link command.

Fixes for https://github.com/cjntaylor/node-cmake/issues/17 for non-Windows builds.

rayglover-ibm commented 7 years ago

This change breaks linking on VS2015/CMake 3.7 for me; the "${NODEJS_LINK_FLAGS}" expression expands to a ; delimited string, which is bad because we need a white-space delimited string for LINK_FLAGS (which itself is a string property).

kkaefer commented 7 years ago

This is a case for separate_arguments.

rayglover-ibm commented 7 years ago

I think seperate_arguments does the opposite of what's needed. Do you have an example?

A naive way of doing this could be string(REPLACE ";" " " LINK_FLAGS_STR "${NODEJS_LINK_FLAGS}") and then:

set_target_properties(${NAME} PROPERTIES
    ...
    LINK_FLAGS "${LINK_FLAGS_STR}"
)
kkaefer commented 7 years ago

You're right, sorry.