Fixed X11 linking on BSD systems

[zjeffer]

According to this comment, the current linking of the X11 libraries in CMakeLists.txt will not work for systems like FreeBSD who install third-party libraries in a location diffrent than the default library paths.

This PR aims to fix this issue by using find_package first.

[zjeffer]

@danfe I tried it your way first but then the 3rdParty library QXT failed to build with this error:

[build] /usr/bin/ld: CMakeFiles/Notes.dir/3rdParty/qxt/qxtglobalshortcut_x11.cpp.o: undefined reference to symbol 'XSetErrorHandler'
[build] /usr/bin/ld: /usr/lib/libX11.so.6: error adding symbols: DSO missing from command line
[build] collect2: error: ld returned 1 exit status
[build] ninja: build stopped: subcommand failed.

[danfe]

/usr/lib/libX11.so.6: error adding symbols: DSO missing from command line

Hmm, I cannot reproduce it locally neither with Clang nor GCC, but Google returns plenty of results with this error. Could it be due to underlinking? Try this, how would that change the linking log I wonder:

-  target_link_libraries(${PROJECT_NAME} PUBLIC X11)
+  find_package(X11 REQUIRED)
+  target_link_libraries(${PROJECT_NAME} PUBLIC ${X11_LIBRARIES})

[zjeffer]

Hmm, I cannot reproduce it locally neither with Clang nor GCC, but Google returns plenty of results with this error. Could it be due to underlinking? Try this, how would that change the linking log I wonder:

-  target_link_libraries(${PROJECT_NAME} PUBLIC X11)
+  find_package(X11 REQUIRED)
+  target_link_libraries(${PROJECT_NAME} PUBLIC ${X11_LIBRARIES})

Isn't this exactly what I did in the PR? :smile: That works.

[danfe]

Isn't this exactly what I did in the PR?

Ah, sorry, I carelessly assumed you have used find_library(X11_LIBRARY X11) as in my original quick'n'dirty patch. So looks like the best way is to still link as -lX11 but accompanied with proper -L/usr/local/lib which is currently missing. I'll take a closer look tomorrow unless you or someone else beats me on this.

[danfe]

Could you try this one please:

-  target_link_libraries(${PROJECT_NAME} PUBLIC X11)
+  include(FindPkgConfig)
+  pkg_search_module(X11 REQUIRED x11)
+  target_link_libraries(${PROJECT_NAME} PUBLIC ${X11_LIBRARIES})
+  target_link_directories(${PROJECT_NAME} PUBLIC ${X11_LIBRARY_DIRS})

[danfe]

That works.

Oh, I think I've misread you there. Feel free to ignore the comment above and go with whatever patch you like best.

[zjeffer]

@danfe Can you check whether my patch works on BSD? I don't have a BSD system set up right now to test this on.

[guihkx]

I can't test this on BSDs either, but as long as our CI passes, I'm fine with the changes.

[danfe]

Can you check whether my patch works on BSD?

Given that I've also suggested the same patch on the second iteration, it does work. The only thing that slightly bugs me is that with find_package(), the final program now links not only to libX11, but to libSM, libICE, and libXext, but then again, maybe it's the right thing after all.