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

Refactor installation of C API and features supported #8642

Merged

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented May 17, 2024

This commit overhauls and refactors the management of the building of
the C API. Instead of this being script-based it's now done entirely
through CMake and CMake is the primary focus for building the C API. For
example building the C API release artifacts is now done through CMake
instead of through a shell script's cargo build and manually moving
artifacts.

The benefits that this brings are:

  • The C API now properly hides symbols in its header files that weren't
    enabled at build time. This is done through a new build-time generated
    conf.h templated on a conf.h.in file in the source tree.

  • The C API's project now supports enabling/disabling Cargo features to
    have finer-grained support over what's included (plus auto-management
    of the header file).

  • Building the C API and managing it is now exclusively done through
    CMake. For example invoking doxygen now lives in CMake, installation
    lives there, etc.

The CMakeLists.txt file for the C API is overhauled in the process of
doing this. The build now primarily matches on the Rust target being
built rather than the host target. Additionally installation will now
install both the static and shared libraries instead of just one.
Additionally during this refactoring various bits and pieces of
Android-specific code were all removed. Management of the C toolchain
feels best left in scope of the caller (e.g. configuring CC_* env vars
and such) rather than here.

This commit overhauls and refactors the management of the building of
the C API. Instead of this being script-based it's now done entirely
through CMake and CMake is the primary focus for building the C API. For
example building the C API release artifacts is now done through CMake
instead of through a shell script's `cargo build` and manually moving
artifacts.

The benefits that this brings are:

* The C API now properly hides symbols in its header files that weren't
  enabled at build time. This is done through a new build-time generated
  `conf.h` templated on a `conf.h.in` file in the source tree.

* The C API's project now supports enabling/disabling Cargo features to
  have finer-grained support over what's included (plus auto-management
  of the header file).

* Building the C API and managing it is now exclusively done through
  CMake. For example invoking `doxygen` now lives in CMake, installation
  lives there, etc.

The `CMakeLists.txt` file for the C API is overhauled in the process of
doing this. The build now primarily matches on the Rust target being
built rather than the host target. Additionally installation will now
install both the static and shared libraries instead of just one.
Additionally during this refactoring various bits and pieces of
Android-specific code were all removed. Management of the C toolchain
feels best left in scope of the caller (e.g. configuring `CC_*` env vars
and such) rather than here.

prtest:full
@alexcrichton alexcrichton changed the title Generate a conf.h in the C API, add CMake features Refactor installation of C API and features supported May 17, 2024
@alexcrichton
Copy link
Member Author

@rockwotj would you be able to give this a once-over perhaps? If not no worries, but you probably know more about CMake than I!

@alexcrichton alexcrichton marked this pull request as ready for review May 17, 2024 04:23
@alexcrichton alexcrichton requested review from a team as code owners May 17, 2024 04:23
@alexcrichton alexcrichton requested review from elliottt and removed request for a team May 17, 2024 04:23
@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label May 17, 2024
Comment on lines +27 to +35
if(${default})
if(${WASMTIME_DISABLE_ALL_FEATURES})
set(feature_default OFF)
else()
set(feature_default ON)
endif()
else()
set(feature_default OFF)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: arguably cleaner

Suggested change
if(${default})
if(${WASMTIME_DISABLE_ALL_FEATURES})
set(feature_default OFF)
else()
set(feature_default ON)
endif()
else()
set(feature_default OFF)
endif()
set(feature_default "${default} AND NOT ${WASMTIME_DISABLE_ALL_FEATURES}")

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I fully understand cmake evaluation contexts and strings and what's a variable when, but when I use your suggestion it looks like it sets feature_default to the string ON AND NOT OFF and I'm not sure why but the default build has all features off.

I'm also not sure if I'm supposed to do something like if(${${cmake_name}}) below instead of just if(${cmake_name}) (sorry I'm very new to cmake)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I retried it and my suggestion was wrong. My bad, I should've investigated better beforehand. Ignore my comment, your method is the way to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok makes sense, and no worries! Thanks for taking a look regardless!

Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

Very nice! This is a great change. Just a few small things

crates/c-api/CMakeLists.txt Outdated Show resolved Hide resolved
crates/c-api/CMakeLists.txt Outdated Show resolved Hide resolved
wasmtime-crate
DOWNLOAD_COMMAND ""
CONFIGURE_COMMAND ""
INSTALL_COMMAND "${WASMTIME_INSTALL_COMMAND}"
BUILD_COMMAND
${CMAKE_COMMAND} -E env ${CARGO_PROFILE_PANIC}=abort
Copy link
Contributor

Choose a reason for hiding this comment

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

While I am thinking about it - the c compiler should be set as an environment variable for the cc crate. There are a number of ways to set it in cmake that don't involve the environment variables that the cc crate uses

Copy link
Member Author

Choose a reason for hiding this comment

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

True! I'm a bit wary to do that though since cmake's cross-compilation defaults are not always as good as the cc crate's auto-detection. Do you know if there's a way to only pass the C compiler env vars and such if explicitly configured in cmake?

@alexcrichton alexcrichton added this pull request to the merge queue May 20, 2024
@alexcrichton
Copy link
Member Author

Thanks @dundargoc and @rockwotj!

Merged via the queue into bytecodealliance:main with commit 566669e May 20, 2024
115 checks passed
@alexcrichton alexcrichton deleted the c-api-features-in-cmake branch May 20, 2024 15:46
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request May 20, 2024
This commit fixes a mistake in bytecodealliance#8642 where a late-minute refactor
accidentally ended up disabling parallel compilation by default.
github-merge-queue bot pushed a commit that referenced this pull request May 20, 2024
This commit fixes a mistake in #8642 where a late-minute refactor
accidentally ended up disabling parallel compilation by default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants