wasmtime / PR #8642 Refactor installation of C API and fe... · git-wasmtime

Sus. option is for cache variables that are booleans, but it looks like WASMTIME_USER_CARGO_BUILD_OPTIONS is supposed to be a string. This doesn't look correct.

Wasmtime GitHub notifications bot (May 17 2024 at 15:03):

dundargoc created PR review comment:

Same thing, option is for booleans and this expects a string.

Wasmtime GitHub notifications bot (May 17 2024 at 15:03):

dundargoc submitted PR review.

Wasmtime GitHub notifications bot (May 17 2024 at 15:27):

dundargoc submitted PR review.

Wasmtime GitHub notifications bot (May 17 2024 at 15:27):

dundargoc created PR review comment:

Nitpick: arguably cleaner

  set(feature_default "${default} AND NOT ${WASMTIME_DISABLE_ALL_FEATURES}")

Wasmtime GitHub notifications bot (May 17 2024 at 15:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 17 2024 at 15:37):

alexcrichton created PR review comment:

Thanks! I didn't really realize that set(...) here was doing that. I'll switch back.

Wasmtime GitHub notifications bot (May 17 2024 at 15:37):

alexcrichton updated PR #8642.

Wasmtime GitHub notifications bot (May 17 2024 at 15:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 17 2024 at 15:39):

alexcrichton created PR review comment:

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)

Wasmtime GitHub notifications bot (May 17 2024 at 16:21):

dundargoc submitted PR review.

Wasmtime GitHub notifications bot (May 17 2024 at 16:21):

dundargoc created PR review comment:

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.

You might need to delete the build folder. Since cache variables are persistent they will keep their values unless you change them specifically. If you build with WASMTIME_DISABLE_ALL_FEATURES=OFF, and then rebuild it with WASMTIME_DISABLE_ALL_FEATURES=ON, then it won't do anything since the other cache variables have already got a value. This problem exists in the original version of the code too. Cmake cache variables are in general footgunny.

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)

Nah, if(${cmake_name}) is correct. For some reason if expects the variable name and not the value itself. I'm not sure how to feel about it tbh :).

In any case, I think the original code is fine as is, just wanted to present an alternative.

Wasmtime GitHub notifications bot (May 17 2024 at 16:23):

dundargoc deleted PR review comment.

Wasmtime GitHub notifications bot (May 17 2024 at 16:26):

dundargoc submitted PR review.

Wasmtime GitHub notifications bot (May 17 2024 at 16:26):

dundargoc created PR review comment:

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.

Wasmtime GitHub notifications bot (May 17 2024 at 18:08):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 17 2024 at 18:08):

alexcrichton created PR review comment:

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

Wasmtime GitHub notifications bot (May 17 2024 at 22:03):

rockwotj submitted PR review:

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

Wasmtime GitHub notifications bot (May 17 2024 at 22:03):

rockwotj submitted PR review:

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

Wasmtime GitHub notifications bot (May 17 2024 at 22:03):

rockwotj created PR review comment:

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

Wasmtime GitHub notifications bot (May 17 2024 at 22:03):

rockwotj created PR review comment:

I generally see people do if not debug (inverting this check). It seems better to default to release builds for an external library

Wasmtime GitHub notifications bot (May 17 2024 at 22:03):

rockwotj created PR review comment:

nit: you could move the WASMTIME_FEATURE_ prefix creation into the macro

Wasmtime GitHub notifications bot (May 20 2024 at 14:59):

alexcrichton updated PR #8642.

Wasmtime GitHub notifications bot (May 20 2024 at 15:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 20 2024 at 15:01):

alexcrichton created PR review comment:

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?

Wasmtime GitHub notifications bot (May 20 2024 at 15:21):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (May 20 2024 at 15:32):

alexcrichton commented on PR #8642:

Thanks @dundargoc and @rockwotj!

Wasmtime GitHub notifications bot (May 20 2024 at 15:46):

alexcrichton merged PR #8642.

Last updated: Nov 22 2024 at 16:03 UTC