DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.11k stars 256 forks source link

GraphBLAS: Check for ability to link with libdl. #838

Closed mmuetzel closed 3 weeks ago

mmuetzel commented 3 weeks ago

On some platforms (like NetBSD), dlopen and similar functions are not in a library. They can be used in any dynamically linked program instead: https://man.netbsd.org/dlopen.3

Use a feature test during configuration to check whether linking to libdl is necessary (or possible).

Fixes #837.

mmuetzel commented 3 weeks ago

The MSVC runners are failing with errors like the following:

[85/5440] Linking CXX executable Mongoose\Release\suitesparse_mongoose.exe
FAILED: Mongoose/Release/suitesparse_mongoose.exe 
C:\Windows\system32\cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe --intdir=Mongoose\CMakeFiles\mongoose_exe.dir\Release --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\[144](https://github.com/DrTimothyAldenDavis/SuiteSparse/actions/runs/9486692457/job/26141625229#step:16:145)0~1.338\bin\Hostx64\x64\link.exe /nologo Mongoose\CMakeFiles\mongoose_exe.dir\Release\Executable\mongoose.cpp.obj  /out:Mongoose\Release\suitesparse_mongoose.exe /implib:Mongoose\Release\suitesparse_mongoose.lib /pdb:Mongoose\Release\suitesparse_mongoose.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console  Mongoose\Release\suitesparse_mongoose.lib  SuiteSparse_config\Release\suitesparseconfig.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ."
LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1440~1.338\bin\Hostx64\x64\link.exe /nologo Mongoose\CMakeFiles\mongoose_exe.dir\Release\Executable\mongoose.cpp.obj /out:Mongoose\Release\suitesparse_mongoose.exe /implib:Mongoose\Release\suitesparse_mongoose.lib /pdb:Mongoose\Release\suitesparse_mongoose.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console Mongoose\Release\suitesparse_mongoose.lib SuiteSparse_config\Release\suitesparseconfig.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST:EMBED,ID=1" failed (exit code 1120) with the following output:
mongoose.cpp.obj : error LNK2019: unresolved external symbol "private: static float * Mongoose::Logger::times" (?times@Logger@Mongoose@@0PAMA) referenced in function "public: static float __cdecl Mongoose::Logger::getTime(enum Mongoose::TimingType)" (?getTime@Logger@Mongoose@@SAMW4TimingType@2@@Z)

Mongoose\Release\suitesparse_mongoose.exe : fatal error LNK1120: 1 unresolved externals

That is probably a regression from https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/7c5caf65cd9723eb62d6e8d7f4884d315191c4c0.

mmuetzel commented 3 weeks ago

I added a commit to (partly) revert https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/7c5caf65cd9723eb62d6e8d7f4884d315191c4c0. Was there a reason these attributes have been removed from the static class members?

traversaro commented 3 weeks ago

A possible alternative is to rely on the CMAKE_DL_LIBS CMake variable, that should contain the correct library to link (or be empty) on all platforms. However, I guess this is mostly a matter of style, the only platform that I know in which it is required to link a library to have dlopen and this library is not called dl is IBM AIX, and I am not sure if there is anyone interested in building GraphBLAS on AIX.

0-wiz-0 commented 3 weeks ago

cmake provides a handy variable for libdl, so you don't have to write your own test, see https://cmake.org/cmake/help/latest/variable/CMAKE_DL_LIBS.html

mmuetzel commented 3 weeks ago

@traversaro, @0-wiz-0: Thank you for the hint. I wasn't aware that CMAKE_DL_LIBS existed.

I modified the PR to use that variable instead of the configure test.

DrTimothyAldenDavis commented 3 weeks ago

I added a commit to (partly) revert 7c5caf6. Was there a reason these attributes have been removed from the static class members?

Yes, I had to add this to get the MATLAB interface on Windows to compile. The MATLAB interface is compiled inside MATLAB with Mongoose/MATLAB/mongoose_make.m.

It failed to compile, with an error stating that static class members cannot be declspec'd like that.

I could add an #ifdef MATLAB_MEX_FILE so the MONGOOSE_API is not used in those 4 lines if it's being compiled inside MATLAB.

DrTimothyAldenDavis commented 3 weeks ago

I'll merge this in and then add the #ifdef MATLAB_MEX_FILE for Mongoose.

mmuetzel commented 3 weeks ago

Yes, I had to add this to get the MATLAB interface on Windows to compile. The MATLAB interface is compiled inside MATLAB with Mongoose/MATLAB/mongoose_make.m.

It failed to compile, with an error stating that static class members cannot be declspec'd like that.

Could you please show the exact error message that you were seeing?

DrTimothyAldenDavis commented 3 weeks ago

Yes, I can post it shortly. My Windows machine is on campus and I'm at home just now. I added this: https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/928086864c1f529f1e66e95c3bad85754edc2077 which should fix the problem, and launched the CI.

I'm using a 2019 version of VS, so that might be what's going on.

mmuetzel commented 3 weeks ago

I'm not sure yet if that is actually a good change. I'm suspecting that somehow there is a confusion between static and shared libraries...

mmuetzel commented 3 weeks ago

Yeah. Looking at that .m file, you are linking these object files "statically" into the .mex file. So, you'd need to define MONGOOSE_STATIC when building the object files.

Scratch that. .mex files are basically .dlls. You need to define MONGOOSE_BUILDING while building the .mex files.