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

[ompl] Fix linking using ${OMPL_LIBRARIES} #18908

Merged
merged 12 commits into from
Jul 22, 2021
Merged

Conversation

Ace314159
Copy link
Contributor

Describe the pull request

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@ghost
Copy link

ghost commented Jul 11, 2021

CLA assistant check
All CLA requirements met.

@Ace314159 Ace314159 marked this pull request as ready for review July 11, 2021 08:56
@Ace314159 Ace314159 changed the title [ompl] Fix _IMPORT_PREFIX [ompl] Fix linking using ${OMPL_LIBRARIES} Jul 11, 2021
@NancyLi1013 NancyLi1013 added the category:port-bug The issue is with a library, which is something the port should already support label Jul 12, 2021
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Jul 12, 2021
@NancyLi1013
Copy link
Contributor

LGTM, thanks for your fixing @Ace314159.

@PhoebeHui
Copy link
Contributor

Actually, ompl has the following issue:

  1. Usage issue: ompl need to export the cmake targets file, in order to avoid linking to release libs when build with debug configuration.
  2. Cmake config: need to add the dependency port boost and eigen in omplConfig.cmake file.
  3. Need to divide the feature 'omplapp' into seperate port. and let it depend on ompl. since they has different source structure, it's hard to apply patch to them, eg, we patch the omplConfig.cmake file to ompl, however, the when build it with feature 'omplapp', it will fail to apply the patch. I assume the feature 'omplapp' would fail here.

@PhoebeHui PhoebeHui removed the info:reviewed Pull Request changes follow basic guidelines label Jul 13, 2021
@Ace314159
Copy link
Contributor Author

Sorry for the delay. I'm not that familiar with the cmake targets, so I'm not sure if I did it correctly, but do these changes address the issues?

@Ace314159
Copy link
Contributor Author

The failures with static seem to be because omlapp fails to link with boost and ccd. This same issue occurs even when installing the feature directly, before I made it a separate port.

I'm not sure how to fix this. Do you have any suggestions?

@Ace314159
Copy link
Contributor Author

I was able to fix the build issues. Does everything look good now?

@PhoebeHui
Copy link
Contributor

Others LGTM.

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 21, 2021
@vicroms vicroms merged commit d12cbb4 into microsoft:master Jul 22, 2021
@Ace314159 Ace314159 deleted the fix-ompl branch July 22, 2021 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ompl] Cannot find lib files when using CMake
4 participants