-
Notifications
You must be signed in to change notification settings - Fork 767
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 GTSAM MATLAB Wrapper #698
Conversation
b28b3570d Merge pull request #30 from borglab/feature/remove_install cc2b07193 Cleanup 610ca176b Allow GTWRAP to be installed in a prefix 193b922c6 Merge pull request #29 from borglab/feature/remove_install 6d2b6ace6 fix path to package e5f220759 clean up some leftover code b0b158a0a install python package as a directory 3f4a7c775 Allow usage without install into global env 5040ba415 Merge pull request #28 from borglab/readme-update 14a7452fe updated README Getting Started section git-subtree-dir: wrap git-subtree-split: b28b3570d221b89f3568f44ed439d3a444903570
@varunagrawal @jlblancoc @dellaert Could you do a quick review on this? This PR should also unbreak the macOS Python CI because of upstream changes in Homebrew. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the one comment.
# We use DESTINATION (instead of TYPE) so we can support older CMake versions. | ||
install(PROGRAMS scripts/pybind_wrap.py scripts/matlab_wrap.py | ||
DESTINATION ${CMAKE_INSTALL_BINDIR}) | ||
install(PROGRAMS scripts/pybind_wrap.py scripts/matlab_wrap.py TYPE BIN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using TYPE? I left a comment saying that we need DESTINATION for older versions of CMake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed :) I'll push a change to this to wrap (but let's first merge this)
This PR should
wrap
to latest version