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

Check for MEX Compiler #554

Merged
merged 5 commits into from
Nov 29, 2020
Merged

Check for MEX Compiler #554

merged 5 commits into from
Nov 29, 2020

Conversation

varunagrawal
Copy link
Collaborator

  • Check if mex compiler exists for Matlab wrapper.
  • Formatting and restructuring.

@varunagrawal varunagrawal added matlab Related to MATLAB wrapper cmake Related to CMake and build system labels Oct 2, 2020
@varunagrawal varunagrawal self-assigned this Oct 2, 2020
CMakeLists.txt Outdated
set(CURRENT_POSTFIX ${CMAKE_${CMAKE_BUILD_TYPE_UPPER}_POSTFIX})
endif()
if(GTSAM_INSTALL_MATLAB_TOOLBOX)
find_package(Matlab COMPONENTS MEX_COMPILER REQUIRED)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should probably remove the relevant code in cmake/GtsamMatlabWrap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is any related code in GtsamMatlabWrap? I may be wrong though. My understanding is that the code in GtsamMatlabWrap is primarily for wrapping and I didn't want to mess with it for minor things.

@varunagrawal varunagrawal requested a review from ProfFan November 12, 2020 03:51
@varunagrawal
Copy link
Collaborator Author

Thanks for the recommendation @ProfFan. I've reworked this PR to be a lot better.

@varunagrawal
Copy link
Collaborator Author

@ProfFan gentle reminder.

@ProfFan
Copy link
Collaborator

ProfFan commented Nov 29, 2020

@varunagrawal Sorry I don't have capacity for this now, if you have tested on both Linux and Windows it's good I think.

@varunagrawal
Copy link
Collaborator Author

@ProfFan cool. Can you please approve this PR then? If any issues are raised for the Matlab wrapper, just assign them to me.

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure :)

@varunagrawal varunagrawal merged commit e90bfdb into develop Nov 29, 2020
@varunagrawal varunagrawal deleted the fix/check-mex branch November 29, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Related to CMake and build system matlab Related to MATLAB wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants