-
Notifications
You must be signed in to change notification settings - Fork 285
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
Make the monolith test a C++ binary. #711
Conversation
This PR seems to have some unrelated changes in it. Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion. CMakeLists.txt, line 464 at r1 (raw file):
Wouldn't this change make C++ compiler required for building toxav? This is a bit undesirable since it's not actually necessary for building toxav. For example, when building toxcore in Docker you generally want to install as few packages as necessary to save time and disk space. Comments from Reviewable |
Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. CMakeLists.txt, line 464 at r1 (raw file): Previously, nurupo wrote…
As you have noted, cmake requires a C++ compiler regardless. I agree, it'll add a few seconds to the build, maybe up to 10 on a slow system/network, but adding more ifs around it adds maintenance cost for rare and negligible benefit (libstdc++ is 1.5M). We can reconsider if this actually becomes a problem. Comments from Reviewable |
Reviewed 6 of 6 files at r1. CMakeLists.txt, line 464 at r1 (raw file): Previously, iphydf wrote…
CMake requires C++ compiler regardless? I didn't know that. I only noted that this change will now make toxcore's cmake require C++ compiler, when before this change that wasn't the case. If CMake, and I'm not talking about toxcore's cmake setup but the actual CMake software, does indeed require C++ compiler for C projects, then alright. I was under the impression it didn't. Comments from Reviewable |
Review status: all files reviewed at latest revision, 1 unresolved discussion. CMakeLists.txt, line 464 at r1 (raw file): Previously, nurupo wrote…
Yeah, you are right, CMake does require C++ compiler for C projects, just did a fast test in a docker container: -- The C compiler identification is GNU 6.3.0
-- The CXX compiler identification is unknown
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
CMake Error at CMakeLists.txt:19 (project):
No CMAKE_CXX_COMPILER could be found.
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.
-- Configuring incomplete, errors occurred!
See also "/root/c-toxcore/build/CMakeFiles/CMakeOutput.log".
See also "/root/c-toxcore/build/CMakeFiles/CMakeError.log". Comments from Reviewable |
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. CMakeLists.txt, line 464 at r1 (raw file): Previously, nurupo wrote…
Wait a moment, C++ compiler is required only because we don't specify the programming lanauge of the project(toxcore) If you set it to project(toxcore C) then it doesn't look for C++ compiler. I wonder if there is a way to add a language on a condition, like we have this Comments from Reviewable |
Review status: all files reviewed at latest revision, 2 unresolved discussions. CMakeLists.txt, line 463 at r1 (raw file):
Adding CXX executable using CMakeLists.txt, line 464 at r1 (raw file): Previously, nurupo wrote…
Found it. You can optionally enable C++. So we could have toxcore as a C project by default but enable C++ if project(toxcore C)
if(COMPILE_AS_CXX)
language_enable(CXX)
endif() You will also need to put a So, what do you say about this (about making the C++ compiler optional)? Should probably be a separate PR, so I will let this go through. Comments from Reviewable |
Review status: 4 of 6 files reviewed at latest revision, 2 unresolved discussions. CMakeLists.txt, line 463 at r1 (raw file): Previously, nurupo wrote…
Done. CMakeLists.txt, line 464 at r1 (raw file): Previously, nurupo wrote…
Maybe separate PR. Comments from Reviewable |
Reviewed 2 of 2 files at r2. Comments from Reviewable |
This way, developers compile toxcore, toxav, and toxencryptsave as C++ at least once at home, reducing the likelyhood of running into travis failures where we compile as C++ in the windows build.
This way, developers compile toxcore, toxav, and toxencryptsave as C++ at
least once at home, reducing the likelyhood of running into travis
failures where we compile as C++ in the windows build.
This change is