Skip to content
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: Do not require C++ by default #3956

Merged
merged 1 commit into from
Dec 20, 2024

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 top-level project under the default settings.

Some configurations might still require a C++ toolchain, so enable_language is selectively called under such circumstances.

Copy link
Collaborator

@xujuntwt95329 xujuntwt95329 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yamt
Copy link
Collaborator

yamt commented Dec 19, 2024

how have you tested this? on which platform?
our CI coverage here is weak. #2686

Copy link
Collaborator

@yamt yamt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable to me.

i bulid-tested this on macos with and without fast-jit.
(with #3968)

@lum1n0us
Copy link
Collaborator

Using the same reasoning, I suppose core/iwasm/compilation/iwasm_compl.cmake requires a similar modification.

@yamt
Copy link
Collaborator

yamt commented Dec 19, 2024

Using the same reasoning, I suppose core/iwasm/compilation/iwasm_compl.cmake requires a similar modification.

good point.

@midokura-xavi92 please test WAMR_BUILD_JIT=1 as well.

@midokura-xavi92
Copy link
Contributor Author

midokura-xavi92 commented Dec 19, 2024

Using the same reasoning, I suppose core/iwasm/compilation/iwasm_compl.cmake requires a similar modification.

Not really: all files that include(${IWASM_DIR}/compilation/iwasm_compl.cmake) always do so from their own CMakeLists.txt (e.g.: wamr-compiler/CMakeLists.txt), and also from their own project, whose languages always default to C CXX, according to the documentation:

By default C and CXX are enabled if no language options are given.

So it is the same rationale I already provided for #3926 .

@midokura-xavi92 please test WAMR_BUILD_JIT=1 as well.

That would require me to build the wamr-compiler from source:

$ cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DCMAKE_BUILD_TYPE=Debug
CMake Error at build-scripts/config_common.cmake:106 (message):
  Cannot find LLVM dir: /home/xavier/wasm-micro-runtime/core/deps/llvm/build
Call Stack (most recent call first):
  build-scripts/runtime_lib.cmake:169 (include)
  CMakeLists.txt:122 (include)

I am not against it, but building LLVM and friends from source will take some time. I will let you know once this is done, but in the meantime please take the rationale above into consideration.

Edit: I built wamrc from source so that the error above does not happen again. As expected, the top-level project configures and builds successfully with WAMR_BUILD_JIT. See logs below:

As shown in the build step, no C++ files are built.

Edit 2: actually, no C++ files were built above only because CXX was not enabled, so CMake would not attempt to build them, even if listed by IWASM_COMPL_SOURCE, so indeed the call to enable_language(CXX) in core/iwasm/compilation/iwasm_compl.cmake is mandatory.

@yamt
Copy link
Collaborator

yamt commented Dec 19, 2024

Using the same reasoning, I suppose core/iwasm/compilation/iwasm_compl.cmake requires a similar modification.

Not really: all files that include(${IWASM_DIR}/compilation/iwasm_compl.cmake) always do so from their own CMakeLists.txt (e.g.: wamr-compiler/CMakeLists.txt), and also from their own project, whose languages always default to C CXX, according to the documentation:

it's included via runtime_lib.cmake.

top-level cmake -> runtime_lib.cmake -> iwasm_compl.cmake

@midokura-xavi92 please test WAMR_BUILD_JIT=1 as well.

That would require me to build the wamr-compiler from source:

you don't need to build wamr-compiler. just llvm.
if you are lucky, you might be able to use the existing llvm binary by setting LLVM_DIR.

$ cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DCMAKE_BUILD_TYPE=Debug
CMake Error at build-scripts/config_common.cmake:106 (message):
  Cannot find LLVM dir: /home/xavier/wasm-micro-runtime/core/deps/llvm/build
Call Stack (most recent call first):
  build-scripts/runtime_lib.cmake:169 (include)
  CMakeLists.txt:122 (include)

I am not against it, but building LLVM and friends from source will take some time. I will let you know once this is done, but in the meantime please take the rationale above into consideration.

as our CI in this area is weak, manual testing is a must.

alternatively you can improve our CI coverage. it's probably a better idea anyway. :-)

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 top-level project under the default settings.

Some configurations might still require a C++ toolchain, so
enable_language is selectively called under such circumstances.

[1]: https://cmake.org/cmake/help/latest/command/project.html
@midokura-xavi92
Copy link
Contributor Author

it's included via runtime_lib.cmake.

top-level cmake -> runtime_lib.cmake -> iwasm_compl.cmake

You are right - sorry for missing that. I have just updated the branch so that CXX is enabled when core/iwasm/compilation/iwasm_compl.cmake is included.

Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lum1n0us lum1n0us merged commit 9598611 into bytecodealliance:main Dec 20, 2024
383 checks passed
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