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

fix USD -> SDF converter build #821

Closed

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Jan 13, 2022

Signed-off-by: Ashton Larkin [email protected]

🦟 Bug fix

requires gazebosim/gz-math#366

Summary

This builds on top of #819 to resolve build issues I faced when trying to setup the USD -> SDF converter. The main changes are:

  1. Remove usage of the c++ filesystem library and use ignition::common::FileSystem instead
  2. Enforce CGAL version 5 in the CMakeLists.txt
  3. Add symbol visibility to resolve linking errors
  4. Remove things like unused variables to help with compiler warnings

Note: CGAL version 5 binaries are not available on bionic. To setup CGAL 5 on bionic, follow steps 3.2 and 4 here: https://doc.cgal.org/latest/Manual/usage.html#secusingwebsite

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Ashton Larkin <[email protected]>
@adlarkin
Copy link
Contributor Author

adlarkin commented Jan 13, 2022

@ahcorde @koonpeng: in order to keep the commit history clean in #819, we should merge #819 into the USD -> SDF converter first before merging this PR. Also, I faced a build error from ign-math, which I believe is resolved by gazebosim/gz-math#366. So, if it turns out that we do need this fix upstream in ign-math, then we need to make a release of ign-math before merging this as well.

@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Jan 13, 2022
@koonpeng
Copy link

@ahcorde @koonpeng: in order to keep the commit history clean in #819, we should merge #819 into the USD -> SDF converter first before merging this PR. Also, I faced a build error from ign-math, which I believe is resolved by gazebosim/gz-math#366. So, if it turns out that we do need this fix upstream in ign-math, then we need to make a release of ign-math before merging this as well.

Should this target ahcorde/usd/prototype_main instead? I should remove CGAL in #819 since it should no longer be used.

@ahcorde
Copy link
Collaborator

ahcorde commented Jan 13, 2022

I cherry-picked this commit in the ahcorde/usd/prototype_main branch

@ahcorde ahcorde closed this Jan 13, 2022
@adlarkin adlarkin deleted the adlarkin/usd_sdf_fix_build branch March 24, 2022 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants