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.

Sign up for GitHub

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

Jump to bottom

FindUUID: Export include path as expected by Ignition Libraries #104

Merged
merged 1 commit into from
Jul 27, 2020
Merged

FindUUID: Export include path as expected by Ignition Libraries #104

merged 1 commit into from
Jul 27, 2020

Conversation

traversaro
Copy link
Contributor

With this fix, UUID headers are correctly found even if UUID is not installed in a system prefix.

Fix #103 .

@traversaro traversaro requested a review from mxgrey as a code owner July 11, 2020 15:51
@chapulina chapulina added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Jul 11, 2020
@traversaro traversaro mentioned this pull request Jul 12, 2020
Merged
@traversaro
Copy link
Contributor Author

I just realized that this patch is not ideal. If someone calls multiple times find_package(UUID), /usr is also added to the include directories, and at some point get_filename_component(include_dir_parent "/" DIRECTORY) is called. It is probably better to first check if the include directory ends with uuid before adding its parent directory.

@chapulina
Copy link
Contributor

@osrf-jenkins run tests please

@chapulina
Copy link
Contributor

@traversaro , from your last comment, I assume you're still working on this PR, right?

@scpeters
Copy link
Member

@osrf-jenkins run tests please

@traversaro
Copy link
Contributor Author

@traversaro , from your last comment, I assume you're still working on this PR, right?

Yes, I need to do a small modification to it, I hope to be able to add it today or tomorrow (CEST).

@traversaro
Copy link
Contributor Author

@traversaro , from your last comment, I assume you're still working on this PR, right?

Yes, I need to do a small modification to it, I hope to be able to add it today or tomorrow (CEST).

The updated version that does not suffer the problems described in #104 (comment) has been pushed to the PR.

@chapulina chapulina requested a review from caguero July 20, 2020 18:20
@caguero caguero merged commit 43e0df4 into gazebosim:ign-cmake2 Jul 27, 2020
@traversaro traversaro mentioned this pull request Nov 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caguero caguero caguero approved these changes

@mxgrey mxgrey mxgrey approved these changes

Assignees
No one assigned
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@traversaro @chapulina @scpeters @mxgrey @caguero