Please find attached the patch addressing the issues + some others, rebased against 5dae6cf. I tested it on the 3 target platforms.
patch.diff
Description: Binary data
> On 18 Feb 2015, at 15:13, Brad King <brad.k...@kitware.com> wrote:
>
> On 02/17/2015 07:28 PM, Raffi Enficiaud wrote:
>> The tests were failing because of the following modification:
>>
>> - matlab_get_version_from_matlab_run(${Matlab_MAIN_PROGRAM}
>> matlab_list_of_all_versions)
>> + matlab_get_version_from_matlab_run("${Matlab_MAIN_PROGRAM}"
>> matlab_list_of_all_versions)
>>
>> Apparently the quotes here are interpreted as part of the binary name,
>> which prevents the proper call to matlab using the execute_process function.
>
> That should not be possible. The quotes are needed in case the variable
> value is an empty string. They will not be treated as part of the value
> passed to the function argument.
I restored the quotes. Maybe I experienced a caching issue: I run ctest with
"FindMatlab" regex, and from time to time the cache is messed while I am
working and I do not clean the folders systematically.
>
>> I kept the symlink resolution, but I narrowed the case those should be
>> resolved. I added a variable pointing to the (symlink resolved) location
>> of the binary from which the version is retrieved.
>
> Yes, thanks.
>
> I squashed the changes into 9d414ca2 and rebased again. Everything
> so far is now in:
>
> FindMatlab: Rewrite module and provide a usage API
> http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=5dae6cfc
>
> and merged to 'next' for testing. Please base fixes for the below
> on that.
Couple of questions:
- does the script of the dashboard clean the folders? Or I have to do that
manually? (cf caching issues above)
- is it the "next" branch that is tested on the "nightly" section of the
dashboard?
>
> More comments:
>
> Why do you need so many different find_program calls for matlab?
> There should be exactly one for Matlab_MAIN_PROGRAM, and it does
> not need to be guarded by if(NOT Matlab_MAIN_PROGRAM) because
> find_program does that already. Any symlink resolution can be
> done on the result.
I wanted to separate the parts in some kind of modules.
- The first part is supposed to output the Matlab_ROOT and nothing else, and
the other parts are relying on that. Finding a matlab_program is an
implementation "detail", which is not cross platform. Yet, the method is kind
of robust to find a proper installation ROOT.
- The second part deals with the version, in case we have no other way than
from asking Matlab. Since at this point, we have a ROOT, either given by the
user or from the first part, we search for the matlab program using this
information alone. In case the user gave the ROOT but not the version, we still
have to find the program under this ROOT. In case the user gave nothing, we
have to find the ROOT and the version, the former maybe implying a
matlab_program search. Again, I think this is an implementation detail that the
second part should not rely on.
- The third part is the user facing matlab_program, that we find on demand.
I agree this can be "optimized" in terms of find_program calls, but I would
like to keep this structure for finding in the appropriate sequence all the
information needed by the module.
The symlink resolutions are made on the appropriate places now.
>
> The get_filename_component(PROGRAM) mode is intended to take a
> command line string and figure out which leading piece is an
> existing program in case it is an unquoted path with spaces.
> While it may be a bug that this can return a directory, there
> should be no use case for this functionality in FindMatlab.
I did not understood that from the documentation ("the program in filename will
be found in the system search path"): I thought it was another way of finding
programs. I removed the corresponding lines.
>
>> # list the keys under HKEY_LOCAL_MACHINE\SOFTWARE\mathworks but the call to
>> # reg does not work from cmake, curiously, as is. The command provides the
>> # desired result under the command line though.
>> # Fix: this is because "/reg:64" should appended to the command, otherwise
>> # it gets on the 32 bits software key (curiously again)
>
> This is because the default registry view depends on which "reg"
> tool gets executed. These comments do not belong in the final
> version of the module.
Yes, we exchanged on this point already. I removed the comments. Basically, at
some point I thought it would have been useful to use cmake as a make that can
run matlab commands through the matlab_program (and not obliged to link
anything to it). This is not possible in the current state of the module, but
would be possible readily in the future. BTW, I volunteered for the maintenance
of the module, so I guess these would be future extensions.
>
>> find_program(MATLAB_REG_EXE_LOCATION "reg")
>
> Many projects just execute_process() the "reg" tool directly
> without finding it first. It is reliably available on Windows.
Ok, I made the change. I thought it would be the "proper" method to find things
first.
>
>> execute_process(
>> COMMAND ${matlab_binary_program} -nosplash -nojvm
>> ${_matlab_additional_commands} -logfile
>> ${_matlab_temporary_folder}/matlabVersionLog.cmaketmp -nodesktop -nodisplay
>> -r "version, exit"
>> OUTPUT_VARIABLE _matlab_version_from_cmd_dummy
>> RESULT_VARIABLE _matlab_result_version_call
>> TIMEOUT 30
>> )
>
> This should quote "${matlab_binary_program}" in case it is
> empty for some reason. Also you should capture the stderr
> output with ERROR_VARIABLE so it does not leak to the output
> of the CMake configuration process.
Done (+ all other calls).
-- 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
