Skip to content

fix wrong FOUND flag on repeated inclusion#146

Merged
dirk-thomas merged 2 commits intomasterfrom
avoid_recursive_find
Jul 17, 2018
Merged

fix wrong FOUND flag on repeated inclusion#146
dirk-thomas merged 2 commits intomasterfrom
avoid_recursive_find

Conversation

@dirk-thomas
Copy link
Copy Markdown
Contributor

@dirk-thomas dirk-thomas commented Jul 16, 2018

Without this patch if a first find of a package sets the FOUND flag to FALSE a repeated find would set the flag to TRUE (which happens implicitly by CMake). While setting it explicitly to FALSE in a CMake config file results in a CMake warning that is at least better then setting the wrong flag.

The first condition to check if the variable is actually being defined can only fail if the variable was set explicitly manually (which should never be the case) or when the CMake config file is being included recursive (which shouldn't be done, ros2/rmw_implementation#44 fixes that case for the rmw_implementation package).

A fat archive with these changes is available from this build: https://ci.ros2.org/job/ci_packaging_linux/126/

Connect to ros2/ros2#544.

@dirk-thomas dirk-thomas added bug Something isn't working in review Waiting for review (Kanban column) labels Jul 16, 2018
@dirk-thomas dirk-thomas self-assigned this Jul 16, 2018
mikaelarguedas
mikaelarguedas previously approved these changes Jul 16, 2018
Copy link
Copy Markdown
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm

Nit: comment L5 should be on L10 shouldn't it ?

@mikaelarguedas
Copy link
Copy Markdown
Contributor

Can you provide details on what should be tested on the resulting archive ?

@dirk-thomas
Copy link
Copy Markdown
Contributor Author

comment L5 should be on L10 shouldn't it ?

The comment in line 5 covers the whole code block so i think the current position makes sense.

@dirk-thomas
Copy link
Copy Markdown
Contributor Author

Can you provide details on what should be tested on the resulting archive ?

See ros2/ros2#544.

@dirk-thomas
Copy link
Copy Markdown
Contributor Author

In the overlay case the variable seems to actually be undefined. So replacing the FATAL_ERROR with just explicitly setting it to FALSE: 8003349

@dirk-thomas
Copy link
Copy Markdown
Contributor Author

Building a new fat archive is in progress: https://ci.ros2.org/job/ci_packaging_linux/127/

@mikaelarguedas mikaelarguedas dismissed their stale review July 16, 2018 22:47

changes happened

@mikaelarguedas
Copy link
Copy Markdown
Contributor

Reproduced the issue with the Bouncy archive and confirmed that the new archive, while printing multiple CMake warnings, can successfully compile and run test from rosidl_generator_py

Copy link
Copy Markdown
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm

@dirk-thomas dirk-thomas merged commit e4562f0 into master Jul 17, 2018
@dirk-thomas dirk-thomas deleted the avoid_recursive_find branch July 17, 2018 21:49
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jul 17, 2018
@mikaelarguedas mikaelarguedas mentioned this pull request Jul 17, 2018
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants