rust-lang / cmake-rs

Rust build dependency for running cmake
https://docs.rs/cmake
Apache License 2.0
310 stars 124 forks source link

Issues with emscripten support #223

Open MoAlyousef opened 1 month ago

MoAlyousef commented 1 month ago

Hello

I've noticed some issues when using cmake-rs to build a project for the wasm32-unknown-emscripten target: 1- Build failure when the host is windows:

thread 'main' panicked at C:\Users\runneradmin/.cargo\registry\src\index.crates.io-6f17d22bba15001f\cmake-0.1.51\src/lib.rs:1100:5:

  failed to execute command: program not found
  is `cmake` not installed?

cmake-rs looks for emcmake as the CMake program, on posix it finds it, however on windows it's called emcmake.bat, so it doesn't find it: https://github.com/MoAlyousef/cmake-rs-emscripten-bug/actions/runs/11159453280/job/31017869199

The repo also shows a minimal repro: https://github.com/MoAlyousef/cmake-rs-emscripten-bug

2- Reconfiguring issues. This I noticed on Linux. When the first cmake configure pass runs it effectively finds emcmake and if the configure succeeds, all is well. If the configure doesn't succeed due to any issues, reconfiguring fails to use emcmake with the following message:

You have changed variables that require your cache to be deleted.
  Configure will be re-run and you may have to reset some variables.
  The following variables have changed:
  CMAKE_CXX_COMPILER= /usr/bin/g++

The CMAKE_INSTALL_PREFIX variable is also reset, so the build either fails due to not finding the emscripten headers or due to failure to install to a system path (permission denied).

For 1, it might just be sufficient to add the .bat extension on windows.

But I propose that building for emscripten uses cmake proper and either searches for the Emscripten.cmake toolchain file which is installed by default with emscripten, or requires explicit passing of the toolchain file in build.rs (where the emscripten toolchain file has a known location relative to the EMSDK env variable).

For example, replacing cmake::Config::new("src/mylib").build(); with:

    let emsdk = std::path::PathBuf::from(std::env::var("EMSDK").unwrap());
    std::process::Command::new("cmake").current_dir("src/mylib").args([
        &format!("-B{out_dir}/bin"),
        &format!("-DCMAKE_TOOLCHAIN_FILE={}", emsdk.join("upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake").display()),
        "-DCMAKE_BUILD_TYPE=Release",
        &format!("-DCMAKE_INSTALL_PREFIX={out_dir}"),
    ]).status().ok();
    std::process::Command::new("cmake")
        .current_dir("src/mylib")
        .args(["--build", &format!("{out_dir}/bin"), "--target", "install"])
        .status()
        .ok();

causes the build to succeed on both systems.

tgross35 commented 1 month ago

The proposal sounds pretty reasonable to me. If you are willing to put up a PR, we ask some of the target maintainers to take a look at it.