-
Notifications
You must be signed in to change notification settings - Fork 192
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
wasi-sdk.cmake: Add wasi-sdk/cmake to CMAKE_MODULE_PATH #182
Conversation
@@ -3,6 +3,10 @@ | |||
# This is arbitrary, AFAIK, for now. | |||
cmake_minimum_required(VERSION 3.4.0) | |||
|
|||
# Until Platform/WASI.cmake is upstream we need to inject the path to it | |||
# into CMAKE_MODULE_PATH. | |||
list(APPEND CMAKE_MODULE_PATH "${WASI_SDK_PREFIX}/cmake") |
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 am using the wasi-sdk release tarball, which has share/cmake/wasi-sdk.cmake
and does not contain the platform. Should the platform file be included at share/cmake/Platform/WASI.cmake
, the following change would be needed:
list(APPEND CMAKE_MODULE_PATH "${WASI_SDK_PREFIX}/cmake") | |
list(APPEND CMAKE_MODULE_PATH "${WASI_SDK_PREFIX}/share/cmake") |
Doing these manually locally does seem to make it work.
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.
@sbc100 Does this change make sense?
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.
There are two minor issue to address here before we land:
- We need to start shipping
share/cmake/Platform/WASI.cmake
in the tarball (I believe we don't ship that at all). - Is this file used in-place? (i.e. do we use this from the git checkout before install?) If so we have two different ways we might want to find WASI.cmake and we need to fix it so it work both in-tree and out-of-tree.
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.
We should probably add some kind of minimal cmake test to the CI here too.
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.
Just to confirm, the release tarball (for 12.0 at least) does not contain the platform file. I placed it to SDK/share/cmake/Platform/WASI.cmake
to mirror the directory layout here.
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.
As @sbc100 mentioned, is there a way we can add a minimal test here to the CI to ensure that this keeps working?
Any updates on this? |
I think this PR has long been forgotten, but from glancing at it it seems like a reasonable change. Are you interested in finishing this out? I think what is left are the "add this file to the release packages" and "check that this works in CI" discussed above. I'll be glad to merge this. |
I certainly have an interest in seeing this fixed, so that I don't have to carry a local patch. To spare me having to hunt around: how/where are the release packages built? EDIT: I found the location. |
Quoting @sbc100 above:
I don't know the answer to this. It is unfortunate that the files are in different location in the release and the source. Perhaps one could be moved to the other? For example using On the other hand, it seems that the Makefile explicitly sets |
I have made a branch in a fork of this repo for this. I assume a separate PR will be needed for it. However, I'm not sure how you would check that this works in CI. Some advise would be appreciated on what a suitable CI check for this would be. Maybe we could need to test building with release SDK, and check that WASI is set in cmake? However, looking at the current test suite, it does not seem to be setup for using cmake currently, and this might be a larger effort. |
Hm, yeah, I see what you mean about no current CMake support in the CI infrastructure. But maybe the place to inject that is here: Lines 32 to 40 in 00cb3b2
I don't use CMake very often but is there a way to run those compile-only tests with CMake? I'm wondering if there is a single-line CMake command that can be slotted in for |
Not sure what you mean, but we could set up a small cmake project that verifies that WASI is set (which will be a good indicator that the platform file exists). Additionally we should probably build a small test program, and preferably execute it too. Assuming cmake is already installed in your CI images, I could definitely cook up something that does this (I use cmake daily). The only unknown is how to execute a built program, as my use case at work (where we use cmake) is using an embedded wasm engine (WAMR). |
What version of cmake do you have in your CI? So I can figure out what minimum version to put in the cmake file. I doubt I will need anything super-modern, so targeting what is in the current Ubuntu LTS is probably a good start? |
I have implemented this, and I will open a separate PR |
This is superseded by #335. |
Fixes: #181