-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
CMakeLists.txt: allow building without a C++ compiler #872
Conversation
Do you have some comments on this PR? |
No feedback on this PR opened more than one year ago? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a maintainer but this seems fine with me in general, just one comment added.
We have one example hiredis-example-qt
that requires C++, but this is not (currently) included in the CMake build.
Maybe this can be handled with CMakes enable_language when the time comes.
Sorry, this was already mentioned, maybe I should start to read everything :)
@@ -1,6 +1,5 @@ | |||
CMAKE_MINIMUM_REQUIRED(VERSION 3.4.0) | |||
INCLUDE(GNUInstallDirs) | |||
PROJECT(hiredis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of removing this line I would like to have a
PROJECT(hiredis LANGUAGES "C")
before
INCLUDE(GNUInstallDirs)
since GNUInstallDirs
needs a language to be set:
CMake Warning (dev) at /snap/cmake/580/share/cmake-3.17/Modules/GNUInstallDirs.cmake:225 (message):
Unable to determine default CMAKE_INSTALL_LIBDIR directory because no
target architecture is known. Please enable at least one language before
including GNUInstallDirs.
Call Stack (most recent call first):
CMakeLists.txt:2 (INCLUDE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review, I updated the PR to move INCLUDE(GNUInstallDirs)
after the PROJECT
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, even better! The provided CMAKE_INSTALL_
variables weren't needed until the install section a bit later..
Define hiredis as a C project (and use a single PROJECT statement) to avoid the following build failure if a C++ compiler is not found: CMake Error at CMakeLists.txt:3 (PROJECT): The CMAKE_CXX_COMPILER: /srv/storage/autobuild/run/instance-1/output-1/host/bin/arm-linux-g++ is not a full path to an existing compiler tool. Tell CMake where to find the compiler by setting either the environment variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path to the compiler, or to the compiler name if it is in the PATH. The only cpp source file is examples/example-qt.cpp which is never compiled with cmake buildsystem. This file is compiled only with the Makefile buildsystem so perhaps it should be removed. If it is added to the cmake buildsystem, a call to enable_language(CXX) will have to be added. Fixes: - http://autobuild.buildroot.org/results/830ec3398cd29b9fc5cde06a225ef531d7a9d850 Signed-off-by: Fabrice Fontaine <[email protected]>
Thank you for the review @bjosv I'll get this merged over the weekend. The issue with cmake fixes is that I'm not very familiar with it. |
Define hiredis as a C project (and use a single
PROJECT
statement) toavoid the following build failure if a C++ compiler is not found:
The only cpp source file is examples/example-qt.cpp which is never
compiled with cmake buildsystem. This file is compiled only with the
Makefile buildsystem so perhaps it should be removed. If it is added to
the cmake buildsystem, a call to
enable_language(CXX)
will have to beadded.
Fixes:
Signed-off-by: Fabrice Fontaine [email protected]