CMake: Move generator conditional expressions

[SmallJoker]

OPENGL_LIBRARIES may contain multiple paths, separated by semicolons. In CMake 3.22.1 this results in an improper list of libraries. The workaround is to instead clear library paths that are not supposed to be linked.

make output right after commit 88ca26c: (notice the stray > after libGLU.so)

make[2]: *** No rule to make target '/usr/lib/x86_64-linux-gnu/libGLU.so>', needed by 'lib/Linux/libIrrlichtMt.so.1.9.0.13'.  Stop.
make[1]: *** [CMakeFiles/Makefile2:289: source/Irrlicht/CMakeFiles/IrrlichtMt.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

Investigating the variables reveals that there seems to be an issue with termination handling of semicolon-separated lists:

message("${OPENGL_LIBRARIES}")
--> /usr/lib/x86_64-linux-gnu/libGL.so;/usr/lib/x86_64-linux-gnu/libGLU.so

This change works on my side but if there's any saner solution, please let me know.

[JosiahWI]

I have been working on a refactor now that we're on CMake 3.12, and I ran across a bug where some of these variables have paths in them and are linked when they should not be. I also remember finding problems with using generator expressions for linking - we should not be doing that. But we should also not be including libraries we don't want to link in the link lists.

I don't think this needs to be made more complicated with the use of a macro. Simple conditional logic should suffice:

if(USE_SDL2)
    list(APPEND link_libs ${SDL2_LIBRARIES})
endif()

It is normal behavior for these variables to have contents even when they are not enabled, because there may be leftover cache entries from a previous configuration in which they were enabled. Clearing the contents of the variable is a very interesting idea, but is there a benefit to doing that instead of a simple conditional that expresses the intent?

[JosiahWI]

I have cleaned up and submitted my in-progress changes as a draft PR (#282) so that we can discuss it.

[SmallJoker]

but is there a benefit to doing that instead of a simple conditional that expresses the intent?

No. It just happened to be the first solution that worked for me. If you pass a semicolon-separated ${variable} to a macro, it seems that it would expand automatically. Now with indirection ${${varname}} and list(APPEND ...) it works just fine.
Either way solves the issue for me. Without these changes I cannot build IrrlichtMt, thus I would like to have this fixed.

[sfan5]

That's weird because generator expressions are that cool new thing you are supposed to use.

FWIW OPENGL_LIBRARIES expands to /usr/lib/libOpenGL.so;/usr/lib/libGLX.so;/usr/lib/libGLU.so on my system and I have no issues with CMake 3.28.1.

[sfan5]

Can you check if this works?

diff --git a/source/Irrlicht/CMakeLists.txt b/source/Irrlicht/CMakeLists.txt
--- a/source/Irrlicht/CMakeLists.txt
+++ b/source/Irrlicht/CMakeLists.txt
@@ -320,7 +320,6 @@ set(link_libs
        "${PNG_LIBRARY}"
        "$<$<BOOL:${USE_SDL2}>:${SDL2_LIBRARIES}>"
 
-       "$<$<BOOL:${OPENGL_DIRECT_LINK}>:${OPENGL_LIBRARIES}>"
        "$<$<BOOL:${OPENGLES_DIRECT_LINK}>:${OPENGLES_LIBRARY}>"
        "$<$<BOOL:${OPENGLES2_DIRECT_LINK}>:${OPENGLES2_LIBRARIES}>"
        ${EGL_LIBRARY}
@@ -533,7 +532,10 @@ target_include_directories(IrrlichtMt
                ${link_includes}
 )
 
-target_link_libraries(IrrlichtMt PRIVATE ${link_libs})
+target_link_libraries(IrrlichtMt PRIVATE
+       ${link_libs}
+       "$<$<BOOL:${OPENGL_DIRECT_LINK}>:${OPENGL_LIBRARIES}>"
+)
 
 if(WIN32)
        target_compile_definitions(IrrlichtMt INTERFACE _IRR_WINDOWS_API_) # used in _IRR_DEBUG_BREAK_IF definition in a public header

If yes, then we might have a problem with quoting.

[SmallJoker]

Can you check if this works?

@sfan5 Yes, this works.

EDIT: This too:

diff --git a/source/Irrlicht/CMakeLists.txt b/source/Irrlicht/CMakeLists.txt
index f90728b..ebe8ed6 100644
--- a/source/Irrlicht/CMakeLists.txt
+++ b/source/Irrlicht/CMakeLists.txt
@@ -533,7 +533,7 @@ target_include_directories(IrrlichtMt
                ${link_includes}
 )
 
-target_link_libraries(IrrlichtMt PRIVATE ${link_libs})
+target_link_libraries(IrrlichtMt PRIVATE "${link_libs}")
 
 if(WIN32)
        target_compile_definitions(IrrlichtMt INTERFACE _IRR_WINDOWS_API_) # used in _IRR_DEBUG_BREAK_IF definition in a public header

I have no idea how this possibly solves the original > error but if it works that's fine by me 🤷

[SmallJoker]

It seems that quoting the libraries variables is not sufficient to solve this issue (). zlib, libjpeg and libpng have "optimized" and "debug" paths available. Why are both included in the same variable? Other projects have run into this as well:
microsoft/vcpkg#6436
Slicer/Slicer#4898

Removing the quotation marks solves this issue but I do not know whether the previous code picked the proper library according to the build type (Debug/Release) or just any of them. because these keywords are then evaluated.

51cc9a8 results in the failure https://github.com/minetest/irrlicht/actions/runs/7601122102/job/20700165967

Thus, I will reside to the macro approach that worked on all build actions.

[JosiahWI]

Maybe getting the correct zlib, libjpeg, and libpng libraries would be fixed by using their targets ZLIB::ZLIB, PNG::PNG, and JPEG::JPEG, which should all be available on CMake 3.12.

[sfan5]

Removing the quotation marks solves this issue

Removing quotes around generator expression is not correct. The CMake docs say so.

@sfan5 Yes, this works.

And that doesn't work on MSVC or what was the issue?

[SmallJoker]

Removing quotes around generator expression is not correct. The CMake docs say so.

By that I meant the quotation marks around the target_link_libraries change that I made in 63fb413 to fix the issue on my end. That however prevents the keyword evaluation for MSVC. The generator expressions must have quotation marks around them.

And that doesn't work on MSVC or what was the issue?

It does work properly for MSVC and on my end but it makes the code messier.

EDIT: Question is what kind of fix we prefer. Currently there's the following options: (meeting topic)

1. macros for link_libs
2. splitting the code: link_libs + target_link_libraries
3. even more splits into multiple target_link_libraries (WIP PR Specify dependencies on object library targets #282)

[SmallJoker]

Adopted the diff proposed in https://irc.minetest.net/minetest-dev/2024-01-21#i_6147394

[appgurueu]

I can compile again with this in its current state.

[JosiahWI]

Straightforward change.