Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

mmuetzel
Copy link
Contributor

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
Copy link
Contributor Author

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 7c5caf6.

@mmuetzel
Copy link
Contributor Author

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

@traversaro
Copy link

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
Copy link

0-wiz-0 commented Jun 12, 2024

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

Most UNIX-like systems provide `dlopen` and similar functions in a
library named libdl.

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

Other platforms (e.g., IBM AIX) provide these functions in a differently
named library (e.g., libld).

Use the name of the library containing `dlopen` and similar functions as
provided by CMake.
This (partly) reverts 7c5caf6

Static members of a class can be accessed without instantiating the
class:
https://en.cppreference.com/w/cpp/language/static
That is conceptionally different from static data. They require
`dllexport`/`dllimport` attributes to be correctly exported from dlls
using MSVC.
@mmuetzel
Copy link
Contributor Author

mmuetzel commented Jun 13, 2024

@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
Copy link
Owner

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
Copy link
Owner

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

@DrTimothyAldenDavis DrTimothyAldenDavis merged commit db3773a into DrTimothyAldenDavis:dev2 Jun 13, 2024
27 checks passed
@mmuetzel
Copy link
Contributor Author

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
Copy link
Owner

Yes, I can post it shortly. My Windows machine is on campus and I'm at home just now. I added this: 9280868 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
Copy link
Contributor Author

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
Copy link
Contributor Author

mmuetzel commented Jun 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants