Hi, My comments below:
> On 13 Feb 2015, at 15:33, Brad King <brad.k...@kitware.com> wrote: > > On 02/12/2015 11:19 AM, Raffi Enficiaud wrote: >> Please find attached the reworked patch > > Great, thanks. Now that we have the nightly testing worked out I've > committed this with minor tweaks as a draft of the change for testing: > > FindMatlab: Rewrite module and provide a usage API > http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=1c7710e9 > > I have a few more comments to be addressed before merge to 'master'. > You can base further patches on the above-linked commit. > > * Why is Matlab_VERSION_STRING cached? Shouldn't it be computed > every time from the matlab that was found? In case the version is not found with an obvious method (on OSX /Applications/MATLABVersion, on Win32, the version also is given by the registry key), we have to find the version of matlab by running matlab itself. I am caching the version once I have it to prevent any further execution of matlab for retrieving this information. Launching Matlab is kind of heavy, involving also network connection sometimes (due to floating license). > > * No find modules ever REALPATH the found values in case the user > has a reason to keep the symlinks. Why do we need to resolve > symlinks in Matlab_MAIN_PROGRAM? In case a symlink of the binary called "matlab" exists in /usr/local/bin for instance, I need to retrieve the path of the libraries mex, mx etc, that are relative to the real installation path of matlab. This is what is happening in my institute, the installation being made on a netshare by IT ppl, that can switch the version in demand. When matlab is launched from a symlink, it is executed in its installation path anyway (the matlab program is a stub to another script), so I need also this piece of information. > > * Several if() calls are using explicit ${VAR} variable dereferences. > Those should be converted to just if(VAR ...) to allow if() to > implicitly dereference them and avoid surprises when their value > happens to name another variable. Ok will do. > > * I will remove the conditions on CMAKE_VERSION in the final upstream > version because we know it always runs with the current CMake > version. You'll have to maintain a small patch on your external > copy of the module for use with older CMake versions. No problem, hope the informations above are clear enough and justify the choices made. Thanks, Raffi Enficiaud -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
