Skip to content

Conversation

@midokura-xavi92
Copy link
Contributor

By default, the project() CMake command defaults to C and C++. 1 Therefore, CMake might perform tests for both C and C++ compilers as part of the configuration phase.

However, this has the consequence of the configuration phase to fail if the system does not have a C++ toolchain installed, even if C++ is not really used by the project.

By default, the project() CMake command defaults to C and C++. [1]
Therefore, CMake might perform tests for both C and C++ compilers as
part of the configuration phase.

However, this has the consequence of the configuration phase to fail if
the system does not have a C++ toolchain installed, even if C++ is not
really used by the project.

[1]: https://cmake.org/cmake/help/latest/command/project.html
@lum1n0us
Copy link
Collaborator

even if C++ is not really used by the project.

Not true. like:

./libraries/wasi-nn/src/wasi_nn_tensorflowlite.cpp
./libraries/lib-wasi-threads/unit-test/test_tid_allocator.cpp
./compilation/aot_orc_extra2.cpp
./compilation/aot_llvm_extra2.cpp
./compilation/debug/dwarf_extractor.cpp
./compilation/aot_llvm_extra.cpp
./compilation/aot_orc_extra.cpp
./fast-jit/cg/x86-64/jit_codegen_x86_64.cpp

@loganek
Copy link
Collaborator

loganek commented Dec 10, 2024

@midokura-xavi92 I'm closing the PR as it looks like there are indeed C++ files for AOT/JIT modes. Feel free to re-open the PR with more context though.

@loganek loganek closed this Dec 10, 2024
@midokura-xavi92
Copy link
Contributor Author

Sorry for the delayed answer.

even if C++ is not really used by the project.

Not true.

Let me rephrase: C++ is not really used by the top-level project by default.

About the files listed above:

./compilation/aot_orc_extra2.cpp
./compilation/aot_llvm_extra2.cpp
./compilation/debug/dwarf_extractor.cpp
./compilation/aot_llvm_extra.cpp
./compilation/aot_orc_extra.cpp

All these files are GLOBbed by iwasm_compl.cmake, which is only included by other CMakeLists.txt that already define their own project, and therefore their own language set (C and CXX by default).

Therefore, these files have no effect on the top-level project.

./libraries/lib-wasi-threads/unit-test/test_tid_allocator.cpp

This file is only used by lib_wasi_threads_unit_tests.cmake, which in turn is only included by tests/unit/tid-allocator/CMakeLists.txt, which already sets its own project, and therefore its own language set.

Therefore, these files have no effect on the top-level project.

./libraries/wasi-nn/src/wasi_nn_tensorflowlite.cpp

wasi-nn is only used when at least WAMR_BUILD_WASI_NN and a backend are selected. WAMR_BUILD_WASI_NN is disabled by default, so no .cpp source files from wasi-nn would be built by default.

That said, if WAMR_BUILD_WASI_NN and WAMR_BUILD_WASI_NN_TFLITE are enabled, enable_language(CXX) should be called. I will add this change to the PR.

./fast-jit/cg/x86-64/jit_codegen_x86_64.cpp

Similarly, this .cpp is only GLOBbed by iwasm_fast_jit.cmake (which is only included when WAMR_BUILD_JIT is set) when WAMR_BUILD_TARGET equals either X86_64 or AMD_64.

Then, enable_language(CXX) should be called only under such conditions. I will also add this change to the PR.

Given the rationale above, I will re-open this PR because IMHO it is still relevant.

@midokura-xavi92
Copy link
Contributor Author

@loganek it looks like it is not possible for me to re-open this PR. Could you please re-open it? Thank you!

@yamt
Copy link
Collaborator

yamt commented Dec 11, 2024

i guess it isn't about the permission.
Screenshot 2024-12-11 at 19 09 25

@midokura-xavi92
Copy link
Contributor Author

@yamt the branch was force-pushed to include the suggestions related to enable_language(CXX) mentioned above. Why isn't it possible to re-open the PR in such case? Should I create a new PR with the force-pushed branch?

@yamt
Copy link
Collaborator

yamt commented Dec 11, 2024

i don't know why. but github has always been that way as far as i know.
cf. isaacs/github#361
while there seems to be a workaround, i guess it's simpler to create another PR and mention it here.

@midokura-xavi92
Copy link
Contributor Author

Superseded by #3956.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants