Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Look for the nddscpp CMake module #20

Merged
merged 3 commits into from
Apr 15, 2015
Merged

Look for the nddscpp CMake module #20

merged 3 commits into from
Apr 15, 2015

Conversation

esteve
Copy link
Member

@esteve esteve commented Apr 14, 2015

No description provided.

@esteve esteve added the in progress Actively being worked on (Kanban column) label Apr 14, 2015
@wjwwood
Copy link
Member

wjwwood commented Apr 14, 2015

@wjwwood
Copy link
Member

wjwwood commented Apr 14, 2015

Now we get a meaningful error w.r.t. pkg-config:

CMake Error at /usr/share/cmake-2.8/Modules/FindPackageHandleStandardArgs.cmake:108 (message):
  Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)
Call Stack (most recent call first):
  /usr/share/cmake-2.8/Modules/FindPackageHandleStandardArgs.cmake:315 (_FPHSA_FAILURE_MESSAGE)
  /usr/share/cmake-2.8/Modules/FindPkgConfig.cmake:106 (find_package_handle_standard_args)
  /usr/share/nddscpp/cmake/nddscppConfig.cmake:8 (find_package)
  cmake/Modules/FindConnext.cmake:220 (find_package)
  CMakeLists.txt:8 (find_package)


-- Configuring incomplete, errors occurred!

@wjwwood
Copy link
Member

wjwwood commented Apr 14, 2015

I'll run again with pkg-config installed, the deb should be updated to depend on it. I think @esteve may already be away of that since we talked about it this morning.

@wjwwood
Copy link
Member

wjwwood commented Apr 14, 2015

@wjwwood
Copy link
Member

wjwwood commented Apr 14, 2015

New problem:

http://54.183.26.131:8080/job/ros2_batch_ci_test_linux/32/console

It doesn't find all of the required libraries. That job is with the four rti binaries (excluding the rti tools deb), NDDS_HOME is not set, pkg-config is explicitly installed, and this pull request's branch is being used.

@esteve esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 15, 2015
@esteve
Copy link
Member Author

esteve commented Apr 15, 2015

The ros2_batch_ci_linux job now finds RTI Connext: http://54.183.26.131:8080/view/ros2/job/ros2_batch_ci_linux/15/console

@esteve
Copy link
Member Author

esteve commented Apr 15, 2015

@dirk-thomas @tfoote @wjwwood please review

@wjwwood
Copy link
Member

wjwwood commented Apr 15, 2015

+1

if(nddscpp_FOUND AND rticonnextmsgcpp_FOUND)
message(STATUS "Found RTI Connext: ${nddscpp_DIR}")
set(Connext_INCLUDE_DIRS ${nddscpp_INCLUDE_DIRS})
set(Connext_LIBRARIES ${rticonnextmsgcpp_LIBRARIES})
Copy link
Member

Choose a reason for hiding this comment

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

Why does this not contain nddscpp_LIBRARIES anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because rticonnextmsgcpp_LIBRARIES already contains them.

Copy link
Member

Choose a reason for hiding this comment

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

If rticonnextmsgcpp contains the superset of nddscpp as well as the messaging libraries should we then just find_package(rticonnextmsgcpp) and use its variables? The mixture looks weird to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add nddscpp_LIBRARIES, even if it's a bit redundant, and remove them from here when/if the messaging libraries are included in RTI Connext.

Copy link
Member

Choose a reason for hiding this comment

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

The PR can be merged as-is but this should be cleaned up sometime in the near future (if we don't a Connext version which just solves that in a single package).

Copy link
Member

Choose a reason for hiding this comment

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

I agree that's weird that we find package both but use INCLUDE_DIRS from one and LIBRARIES from another. At the very least maybe @esteve could add a comment that rticonnextmsgcpp_LIBRARIES contains the nddscpp_LIBRARIES and a TODO comment to clean it up?

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up adding nddscpp_LIBRARIES too, it took less time than adding a comment :-)

@wjwwood
Copy link
Member

wjwwood commented Apr 15, 2015

So, are we going to make an adjustment to this or is it good to go? I don't mind either way, just wondering.

@esteve
Copy link
Member Author

esteve commented Apr 15, 2015

@wjwwood I'm waiting for @tfoote 's review.

@tfoote
Copy link
Contributor

tfoote commented Apr 15, 2015

+1

wjwwood added a commit that referenced this pull request Apr 15, 2015
Look for the nddscpp CMake module
@wjwwood wjwwood merged commit dc0e722 into master Apr 15, 2015
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Apr 15, 2015
@wjwwood wjwwood deleted the find-package-nddscpp branch April 15, 2015 23:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants