Address warnings, fix FreeBSD build #138
Conversation
There was a problem hiding this comment.
Hi, thanks for the PR.
Many of the changes here seem reasonable, but I do not understand how any of these changes are FreeBSD-specific - can you explain why they are needed for FreeBSD specifically?
- The need to wrap the
BUILD_SHARED_LIBSoption in a condition sounds like something specific to either your CMake version or the way you're invoking CMake. - The original use of
pkg_check_modulesseems like something that would also work on both Linux and FreeBSD, unless FreeBSD has a weird pkg-config application.
What compiler and compile flags are you using? We should probably enable those extra flags on the CI too.
Also, I think I'll need to set up a FreeBSD CI to ensure that regressions aren't inadvertently introduced in the future.
| if(NOT DEFINED BUILD_SHARED_LIBS) | ||
| option(BUILD_SHARED_LIBS "Build a shared library instead of static" OFF) | ||
| endif() |
There was a problem hiding this comment.
Can you explain a bit more about what this does? Most examples on the Internet do not put option inside a if condition like this. Some GitHub Actions are configured to pass -DBUILD_SHARED_LIBS=ON, and it doesn't cause any problems with the existing code.
Also, what is making BUILD_SHARED_LIBS different from NFD_BUILD_TESTS and NFD_INSTALL below?
There was a problem hiding this comment.
Yes, I am not building NFDE by itself.
Here is the warning I'm getting:
CMake Warning (dev) at deps/nfde/CMakeLists.txt:9 (option):
Policy CMP0077 is not set: option() honors normal variables. Run "cmake
--help-policy CMP0077" for policy details. Use the cmake_policy command to
set the policy and suppress this warning.
For compatibility with older versions of CMake, option is clearing the
normal variable 'BUILD_SHARED_LIBS'.
This warning is for project developers. Use -Wno-dev to suppress it.
BUILD_SHARED_LIBS is a standard CMake variable used by a lot of different projects, though it might make sense to do that with the others as well for projects like mine that bundle NFDE.
| find_package(PkgConfig REQUIRED) | ||
| # for Linux, we support GTK3 and xdg-desktop-portal | ||
| option(NFD_PORTAL "Use xdg-desktop-portal instead of GTK" OFF) | ||
| if(NOT NFD_PORTAL) | ||
| pkg_check_modules(GTK3 REQUIRED gtk+-3.0) | ||
| message("Using GTK version: ${GTK3_VERSION}") |
There was a problem hiding this comment.
Why remove this but keep the message("Using DBUS version: ${DBUS_VERSION}") message for the portal case below? This message is meant to be here so that users can see that they are using the correct backend.
Also, should we make similar modifications for configuring portals too (i.e. make the same changes for the dbus-1 package)?
There was a problem hiding this comment.
I'm not sure why I removed that message. I put it back.
|
It looks like you guys fixed the FreeBSD build sometime between my last pull and now. |
These changes address some compiler and CMake warnings, and fixes NFDE to build properly on FreeBSD.