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

Look for the nddscpp CMake module#20

Merged
wjwwood merged 3 commits intomasterfrom
find-package-nddscpp
Apr 15, 2015
Merged

Look for the nddscpp CMake module#20
wjwwood merged 3 commits intomasterfrom
find-package-nddscpp

Conversation

@esteve
Copy link
Copy Markdown
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
Copy Markdown
Member

wjwwood commented Apr 14, 2015

Testing here: http://54.183.26.131:8080/job/ros2_batch_ci_test_linux/28/console

@wjwwood
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

wjwwood commented Apr 14, 2015

New job: http://54.183.26.131:8080/job/ros2_batch_ci_test_linux/29/console

@wjwwood
Copy link
Copy Markdown
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.

@wjwwood wjwwood mentioned this pull request Apr 15, 2015
Closed
7 tasks
@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
Copy Markdown
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
Copy Markdown
Member Author

esteve commented Apr 15, 2015

@dirk-thomas @tfoote @wjwwood please review

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 15, 2015

+1

dirk-thomas
dirk-thomas reviewed Apr 15, 2015
View reviewed changes
connext_cmake_module/cmake/Modules/FindConnext.cmake Outdated
Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Apr 15, 2015

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
Copy Markdown
Member Author

@esteve esteve Apr 15, 2015

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
Copy Markdown
Member

@dirk-thomas dirk-thomas Apr 15, 2015

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
Copy Markdown
Member Author

@esteve esteve Apr 15, 2015

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
Copy Markdown
Member

@dirk-thomas dirk-thomas Apr 15, 2015

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
Copy Markdown
Member

@wjwwood wjwwood Apr 15, 2015

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
Copy Markdown
Member Author

@esteve esteve Apr 15, 2015

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 wjwwood mentioned this pull request Apr 15, 2015
Closed
2 tasks
@wjwwood
Copy link
Copy Markdown
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
Copy Markdown
Member Author

esteve commented Apr 15, 2015

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

@tfoote
Copy link
Copy Markdown
Contributor

tfoote commented Apr 15, 2015

+1

wjwwood added a commit that referenced this pull request Apr 15, 2015
@wjwwood
Merge pull request #20 from ros2/find-package-nddscpp
dc0e722 
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.

Reviewers

No reviews

Assignees

@esteve esteve

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@esteve @wjwwood @tfoote @dirk-thomas