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 GTSAM MATLAB Wrapper #698

Merged
merged 4 commits into from
Feb 16, 2021
Merged

Fix GTSAM MATLAB Wrapper #698

merged 4 commits into from
Feb 16, 2021

Conversation

ProfFan
Copy link
Collaborator

@ProfFan ProfFan commented Feb 16, 2021

This PR should

  1. Update wrap to latest version
  2. Unbreak the MATLAB wrapper (we still need to figure out a way to do templates in global function later)
  3. Unbreak the CI pipeline (especially the macOS CI)

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
@ProfFan ProfFan changed the title Fix GTSAM MATLAB Fix GTSAM MATLAB Wrapper Feb 16, 2021
@ProfFan
Copy link
Collaborator Author

ProfFan commented Feb 16, 2021

@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.

Copy link
Collaborator

@varunagrawal varunagrawal left a 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)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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)

@ProfFan ProfFan merged commit cb05d01 into develop Feb 16, 2021
@ProfFan ProfFan deleted the feature/wrap_update branch February 16, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants