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

remove unneeded line from CMakeLists #70

Merged
merged 1 commit into from
Sep 27, 2019
Merged

remove unneeded line from CMakeLists #70

merged 1 commit into from
Sep 27, 2019

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Sep 25, 2019

I don't think we need to tweak the link_directories to get this Poco to link properly

@dirk-thomas
Copy link
Member

This builds on top of #67.

As mentioned in that ticket please consider to provide the additional information so that we can be certain that the link_directories(${Poco_LIBRARY_DIR}) call can be safely removed.

@rotu
Copy link
Contributor Author

rotu commented Sep 25, 2019

What additional information is needed? Correctly building on target platforms should prove that this PR is correct. Please run CI to confirm.

@rotu rotu force-pushed the pr-1 branch 2 times, most recently from 2e340b6 to 0507a0b Compare September 25, 2019 23:00
@dirk-thomas
Copy link
Member

dirk-thomas commented Sep 26, 2019

What additional information is needed?

It helps to understand why it was used and necessary in the past, what has changed in the meantime, and confirms it can be safely removed now.

Correctly building on target platforms should prove that this PR is correct. Please run CI to confirm.

Passing builds only show that it works on the exact configurations used but doesn't provide us with any understanding.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@rotu
Copy link
Contributor Author

rotu commented Sep 26, 2019

You made that change, so you tell me. It looks to me like it was never necessary.

52a46ba#diff-0c408aa74d7519e29ea65afd30874fcf

#17

Passing builds only show that it works on the exact configurations used but doesn't provide us with any understanding.

True but (1) that applies to everything (2) CI gives the best assurance that this works across supported build configurations.

@dirk-thomas
Copy link
Member

True but (1) that applies to everything (2) CI gives the best assurance that this works across supported build configurations.

I like to understand why things work / don't work and what has changed.

I am pretty sure for earlier versions of Poco which didn't bundle a CMake config this variable was provided by either a distro specific patch or a vendor-like wrapper package.

Anyway as of version 1.6.0 Poco include a CMake config file (see https://github.com/pocoproject/poco/blob/poco-1.6.0-release/cmake/PocoConfig.cmake.in). So it seems to be fine to remove.

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed more-information-needed Further information is required labels Sep 26, 2019
@dirk-thomas
Copy link
Member

With #67 merged this needs a rebase.

I don't think we need to tweak the link_directories to get this Poco to link properly

Signed-off-by: Dan Rose <dan@digilabs.io>
@rotu
Copy link
Contributor Author

rotu commented Sep 27, 2019

Rebased

@dirk-thomas dirk-thomas merged commit 454dd53 into ros2:master Sep 27, 2019
@rotu rotu deleted the pr-1 branch September 27, 2019 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants