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

Add target_include_directories and rtf_add_plugin #82

Merged
merged 10 commits into from
Oct 6, 2017

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Oct 5, 2017

This PR add add target_include_directories and cxx11 target_compile features in RTF_dll RTF_ruby RTF_lua RTF_python.
Moreover it adds the cmake macro rtf_add_plugin.

Please review code.

@Nicogene Nicogene changed the title Ifi day nico Add target_include_directories and rtf_add_plugin Oct 5, 2017
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

In general it is ok, but it would be good to avoid having public properties set on exectuable. Furthermore, it would be good to cleanup the existing macros for adding plugins : https://github.com/robotology/robot-testing/blob/master/conf/RTFTestHelpers.cmake#L30 .

@@ -32,6 +32,11 @@ source_group("Header Files" FILES ${folder_header})
add_executable(${PROJECT_NAME} ${folder_source} ${folder_header})
target_link_libraries(${PROJECT_NAME} ${RTF_LIBS} ${RTF_INTERNAL_LIBS} ${TinyXML_LIBRARIES})

target_compile_features(${PROJECT_NAME} PUBLIC cxx_nullptr)

target_include_directories(${PROJECT_NAME} PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
Copy link
Member

Choose a reason for hiding this comment

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

Not a big problem, but it is a bit strange to have PUBLIC include directories/compile features for 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.

Fixed in b85b8da .

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling 46b979c on Nicogene:IfiDayNico into c75ba53 on robotology:devel.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling 46b979c on Nicogene:IfiDayNico into c75ba53 on robotology:devel.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling 4760811 on Nicogene:IfiDayNico into c75ba53 on robotology:devel.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.263% when pulling 4760811 on Nicogene:IfiDayNico into c75ba53 on robotology:devel.

@aerydna aerydna merged commit 2084067 into robotology:devel Oct 6, 2017
@aerydna
Copy link
Contributor

aerydna commented Oct 6, 2017

merged thanks!

@traversaro
Copy link
Member

My bad, the existing macros are just to add internal tests of RTF.

@Nicogene Nicogene deleted the IfiDayNico branch October 6, 2017 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants