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

Update FindMatlab.cmake to CMake's v3.14.3 #11

Merged
merged 3 commits into from
May 7, 2019
Merged

Conversation

traversaro
Copy link
Contributor

Import latest CMake's FindMatlab.cmake file, from CMake v3.14.3
Exact file URL: https://raw.githubusercontent.com/Kitware/CMake/v3.14.3/Modules/FindMatlab.cmake

@traversaro
Copy link
Contributor Author

@Giulero can you try if this PR fixes #9 ?

For future debug, it would also be useful to log the output of nm nameOfTheMexFile for both when the library is working and when the library is not working, so we can understand which symbols we were missing.

@traversaro
Copy link
Contributor Author

For future debug, it would also be useful to log the output of nm nameOfTheMexFile for both when the library is working and when the library is not working, so we can understand which symbols we were missing.

Actually I do not think there is anything strange, this is exactly the problem described in robotology/blockfactory#4 (comment) , so there is not a lot to understand. Feel free not to check the output of nm, and just test if this PR fixes the problem, thanks!

To support the features required in the latest FindMatlab.cmake
@traversaro
Copy link
Contributor Author

Friendly ping @Giulero .

@Giulero
Copy link
Collaborator

Giulero commented May 5, 2019

Sorry for the delay! :/
Probably I did something wrong.
I've imported the latest CMake's FindMatlab.cmake file in ~robotology-superbuild/external/qpOases/cmake.
Then, from the build folder, make update-all and make.
I got this error:

-- Configuring incomplete, errors occurred!
See also "/home/giuseppe/robotology-superbuild/build/external/qpOASES/CMakeFiles/CMakeOutput.log".
CMakeFiles/qpOASES.dir/build.make:101: recipe for target 'external/qpOASES/CMakeFiles/YCMStamp/qpOASES-configure' failed
make[2]: *** [external/qpOASES/CMakeFiles/YCMStamp/qpOASES-configure] Error 1
CMakeFiles/Makefile2:2202: recipe for target 'CMakeFiles/qpOASES.dir/all' failed
make[1]: *** [CMakeFiles/qpOASES.dir/all] Error 2
Makefile:140: recipe for target 'all' failed
make: *** [all] Error 2

Here the log:
CMakeOutput.log

@traversaro
Copy link
Contributor Author

Sorry, I probably had to provide some detail about how to test a PR such as this. First of all, make sure that you do not have any custom modification in your qpOASES source repo, i.e. :

cd ~/robotology-superbuild/build/external/qpOASES/
git status

should give no local modified files, including my workaround in #9 . You can use git stash for quickly storing local modification and restoring them later.
Once you have no local modified files, you can switch to the branch that you want to test, in the case of this PR update-find-matlab:

git checkout update-find-matlab

At this point, you can try to configure, compile and install the qpOASES project to verify if it is working correctly. You can just go back to the superbuild root and run make:

cd ~/robotology-superbuild/build/
make

After the command is run successfully, you can test if the matlab bindings are working correctly or not.
In this state, do not run make update-all, as you have one repo in a branch to which you manually switched. If you want to run make update-all to update all the other repos, you can set qpOASES in DEVEL mode, by setting the YCM_EP_DEVEL_MODE_qpOASES option to ON. See the last lines in https://github.com/robotology/robotology-superbuild/blob/master/README.md#update for more info.

@traversaro
Copy link
Contributor Author

The additional fix 899afd8 was necessary.

@Giulero if you can approve the PR ( https://help.github.com/en/articles/about-pull-request-reviews ) then I can merge it, thanks!

@Giulero Giulero merged commit 38ca942 into master May 7, 2019
@Giulero
Copy link
Collaborator

Giulero commented May 7, 2019

Done :)
Thank you!

@traversaro
Copy link
Contributor Author

traversaro commented May 7, 2019

@Giulero just to let you know, approving a PR is different from merging it (I had to gave you permissions to the repo to enable you to review the PR). In general, it is a good practice to only merge commits that would successfully configure and compile, to readability and to permit in the future to easily git bisect if a problem is detected. For this reason, it would have been slightly better to Squash and merge this PR, instead of just merging. "Squash and merge" (https://help.github.com/en/articles/about-pull-request-merges) combines all the commits of the PR in a single commit, that is a commit that successful configure and generates. Not a big problem for now, just a suggestion for the future.

@Giulero
Copy link
Collaborator

Giulero commented May 7, 2019

@traversaro I understand.
In this regard, I think I will see other PRs to understand how to do them.

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