Hi Brad,

(Sorry for the late answer (again).)
I addressed your comments in the files attached to this email (please see the remarks below). I have not yet addressed your comment about “MATLAB_ADDITIONAL_VERSIONS” but I think it is a better proposition, so I will do it next.
I updated the documentation also, I think it looks now understandable.

I had a hard time making some stuff compile again with Matlab under Linux. The fact is that Matlab is shipped with its own version of libC, libhdf5, libboost etc, and sometimes the filenames are the same (eg. hdf5, or libc) but the subminor versions are not. If I understand properly, the fact that the filenames are the same prevents the dynamic loader of Linux to load the files that are referred by the mex file in their respective rpath (there is only one libhdf51.8.2.so in the matlab process). Beside that, the symbols also may clash: I had an implementation with a dynamic loader under linux, but yet the boost symbols of my mex files were not loaded because same symbols were already there in the process. 
I found a technical solution to this on Linux: 
- the use of -Wl,--exclude-libs,ALL for the linker. 
- the symbol hiding (both include and function definition) from within the mex file
- exporting only the mexFunction symbol

I know that this scheme is working for statically linked libraries, that then should be recompiled with -fPIC. I know also that this is working for shared libraries that do not have the same name (eg. boost1.49.so vs. boost1.55.so). But I think there is nothing more I can do for the shared library having the same name (like hdf51.8.2.so), having the same symbols but apparently having a different ABI.

I do not know how to do that properly under OSX neither. There is no -Wl,--exclude-libs,ALL option in Clang.

Also, I am checking against CMAKE_CXX_COMPILER_ID, which is I think a not so good idea. Is there anything for having the -Wl,--exclude-libs,ALL like the variable CXX_VISIBILITY_PRESET in cmake? I started using a .map file on these two platforms, but also I would prefer limiting the number of files. 

I am using this FindMatlab in two projects now, under OSX 10.9, Win7 Visual2013 and Ubuntu12.04. 

Best,
Raffi

Attachment: FindMatlab.cmake
Description: Binary data

Attachment: MatlabTestsRedirect.cmake
Description: Binary data




——————

Hi Raffi,

Thanks for your continuing work on this module.

I've made some whitespace and quoting tweaks to the files.  See attached
updated versions.  I also renamed the test helper to not start in "Find"
since no one should call find_package(Matlab_TestsRedirect).  See further
comments below.

On 07/04/2014 08:02 PM, Raffi Enficiaud wrote:
Many of our tests use "cmake -P" to run a cmake script that performs
the test inside.  You can use -Dvar=${val} before the -P option to
pass options to the script, such as $<TARGET_FILE_DIR:my_mex_target>.

Done: this is the add_matlab_unit_test function. In fact, I think I can
remove the log files since they are redundant with the tests logs.

I see no problem with that, but I'm not familiar with the tools.

I added a function add_matlab_mex that is like a add_library

Please make all APIs start in "matlab_”.

Done


The documentation blocks for each command also need to start in "#.rst:"

#.rst:
# .. command:: add_matlab_mex

or they will not be included in the processed documentation.

Ok. I checked the documentation again, and I think it contains now all the necessary information, plus it is now visually more appealing. 



for creating MEX (compiled) extensions. There is an option to this
function called REDUCE_VISIBILITY

I see that implemented here:

if(HAS_VISIBILITY_INLINE_HIDDEN)
 target_compile_options(${${prefix}_NAME} PRIVATE "-fvisibility-inlines-hidden")
endif()
if(HAS_VISIBILITY_HIDDEN)
 target_compile_options(${${prefix}_NAME} PRIVATE "-fvisibility=hidden")
endif()

An upstream version of the module should use the builtin features
for visibility settings:


http://www.cmake.org/cmake/help/v3.0/prop_tgt/LANG_VISIBILITY_PRESET.html


http://www.cmake.org/cmake/help/v3.0/prop_tgt/VISIBILITY_INLINES_HIDDEN.html

I am not sure how to do that, please have a look at my changes. It looks ok to me (generated compilation command include the visibility element) but maybe there is something better.





#    set(MATLAB_ADDITIONAL_VERSIONS
#       "release_name1" "corresponding_version1"
#       "release_name2" "corresponding_version2"
#       ...
#       )

Rather than relying on matched pairs, perhaps use "=" to separate:

#    set(MATLAB_ADDITIONAL_VERSIONS
#       "release_name1=corresponding_version1"
#       "release_name2=corresponding_version2"

?

Right, it is better.



I am not sure how my implementation is compatible with the previous
FindMatlab package. The requirements are to define the same variables,
right? Is there anything else?

It needs to produce the same result variables and also respond to
the old "input" variables (like find_ command cached results) that
the old module did.  That way existing build scripts can continue
to work.

This is something I should now check deeper. Is it an option to call it FindMatlab2 ?




+# * ``MATLAB_USER_ROOT`` the root of the Matlab installation. This is given by the user.

This should be documented in its own section of variables that the
user can set to control the search.  See FindZLIB for example.

+  # 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 it gets the registry view of the calling CMake
unless you tell it what view to use.

Ok


+  find_program(MATLAB_REG_EXE_LOCATION "reg")
+  file(TO_NATIVE_PATH ${MATLAB_REG_EXE_LOCATION} MATLAB_REG_EXE_LOCATION)

The second line should not be needed.  The execute_process command
will take care of the path format.

Done


+  if((NOT DEFINED MATLAB_USER_ROOT) OR (NOT MATLAB_USER_ROOT))

This can be shortened to

if(NOT MATLAB_USER_ROOT)

Done


+    if(NOT ${Matlab_PROGRAM})

and this to

if(NOT Matlab_PROGRAM)

Done


BTW, is there any variable that tells the current cmake list,
even in function in packages?

In the attached version I added

set(_FindMatlab_SELF_DIR "${CMAKE_CURRENT_LIST_DIR}")

to the top of the file to save the location of the file for re-use
inside function calls later.

Good. 


Thanks,
-Brad
-- 

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

Reply via email to