[cmake-developers] Introduction and volunteering for the Matlab package

[Raffi Enficiaud]

Please find attached the patch addressing the issues + some others, rebased against 5dae6cf. 
I tested it on the 3 target platforms.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: application/octet-stream
Size: 6174 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20150218/92ff32f1/attachment.obj>
-------------- next part --------------


> On 18 Feb 2015, at 15:13, Brad King <brad.king at 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).