> On 19 Feb 2015, at 14:53, Brad King <brad.k...@kitware.com> wrote: > > On 02/19/2015 08:39 AM, Raffi Enficiaud wrote: >> Please find attached the merge of the two previous patches, rebased on >> 5dae6cf. > > Applied, thanks: > > FindMatlab: Further revisions > http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=1416d214 > Thanks.
>>> Those issues are related to the space in the test folder in the dashboard > > Quoting of arguments in the cmake language: > > http://www.cmake.org/cmake/help/v3.2/manual/cmake-language.7.html#command-arguments > > is not necessary to deal with spaces that are not literally written in > the argument. Separation of unquoted arguments after variable evaluation > only happens on ";". However, any unnecessary quoting you added also > won't hurt anything and makes it easier to read anyway. Good then. Matlab supports not very well spaces in the log file name, I suppose that if I do execute_process(COMMAND matlab -logfile "some path with spaces") this is correctly escaped by cmake. > > Returning to a previous comment: > >> On 02/18/2015 09:13 AM, Brad King wrote: >>> Why do you need so many different find_program calls for matlab? >>> There should be exactly one for Matlab_MAIN_PROGRAM > > I still see four places that try to find "matlab", quoted below. > It looks like you're trying to make Matlab_MAIN_PROGRAM defined > if the MAIN_PROGRAM component was requested. Relates to my previous answer on this topic. > >> find_program( >> _matlab_main_tmp >> NAMES matlab) >> >> if(NOT _matlab_main_tmp) >> execute_process( >> COMMAND "which" matlab >> OUTPUT_VARIABLE _matlab_main_tmp >> ERROR_VARIABLE _matlab_main_tmp_err) >> # the output should be cleaned up >> string(STRIP "${_matlab_main_tmp}" _matlab_main_tmp) >> endif() > > If find_program doesn't find it, "which" won't have better luck. Which is also what I thought but this is factually incorrect. I tested that yesterday on a regular LTS14.04 server. find_program fails while "which matlab" does not. > >> if(Matlab_MAIN_PROGRAM) >> set(_matlab_main_tmp ${Matlab_MAIN_PROGRAM}) >> else() >> find_program( >> _matlab_main_tmp >> matlab >> PATHS ${Matlab_ROOT_DIR} ${Matlab_ROOT_DIR}/bin >> DOC "Matlab main program" >> NO_DEFAULT_PATH >> ) >> endif() > > We should not risk finding the wrong matlab here. See below. > >> # todo cleanup with code above >> if(NOT DEFINED Matlab_MAIN_PROGRAM) >> find_program( >> Matlab_MAIN_PROGRAM >> matlab >> PATHS ${Matlab_ROOT_DIR} ${Matlab_ROOT_DIR}/bin >> DOC "Matlab main program" >> NO_DEFAULT_PATH >> ) >> endif() > > This looks like the component-specific search. If we are to present > a component-specific result variable name then it can simply be > populated from the "one true" location found. In the code just above, the "if()" condition is not needed anymore since now there is no aliasing with the previous searches. I'll fix that. On Windows, we have the Matlab_ROOT and the version from the registry, so we need to run find_program there. On OSX, if the previous find_program or which matlab did not result in anything relevant, we parse the /Application/MATLAB_xxx folders so we end up with a root and a version without a main_program, so the find_program above is also needed. > > If "matlab" is needed to compute information for other components > then finding it should not be optional. There should be a single > > find_program(Matlab_EXECUTABLE ...) > > whose result is cached and re-used everywhere that "matlab" is > needed. Separate symlink-resolving on it can be done when needed > but does not need to be cached. I agree with that, but this behaviour is not consistent across every platforms. My preference goes to the kind of modular approach currently implemented in the module, which is: 1- find all possible matlab roots with the tools provided by the system, or just stick to the user input 2- for each or one of them, find the version if information is lacking 3- return the one that fits best to the find_matlab options Finding the matlab program from time to time is for me an implementation detail: examples - if the user give the Matlab_ROOT, the version is lacking, I then need to find matlab in the second step. - if the user gave no input, I need to find Matlab in the first step, conditionally on the platform, to extract Matlab_ROOT (and not to find matlab itself). I then run the find_matlab in the second step conditionally on the platform again. In the third step, I gather all the information I have so far, which is the intersection for all platforms: MatlabROOT and MatlabVERSION. - on win32, if there is not user defined Matlab_ROOT, we have the root and version from the registry, there is only the last component oriented find_program running, only if required by the user. Also the main functionality is not performance oriented. Caching is in fact an undesirable side-effect for what I want to achieve. While finding matlab is certainly not optimal, this is clearly taking much less time than just launching matlab. If I start trying to optimize all those calls, I would have complicated execution paths. What I can do is to output in the first step the root AND the version in all case, instead of dedicating a step for that. We will then save a find_program and have a cleaner/clearer structure. What do you think? Best, Raffi -- 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
