Stream: git-wasmtime

Topic: wasmtime / PR #8642 Refactor installation of C API and fe...


view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 03:55):

alexcrichton edited PR #8642.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 04:23):

alexcrichton commented on PR #8642:

@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!

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 04:23):

alexcrichton has marked PR #8642 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 04:23):

alexcrichton requested elliottt for a review on PR #8642.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 04:23):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8642.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 04:23):

alexcrichton requested wasmtime-default-reviewers for a review on PR #8642.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 15:02):

dundargoc submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 15:02):

dundargoc created PR review comment:

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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 15:03):

dundargoc submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 15:27):

dundargoc submitted PR review.

view this post on Zulip 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}")

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 15:37):

alexcrichton submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 15:37):

alexcrichton updated PR #8642.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 15:39):

alexcrichton submitted PR review.

view this post on Zulip 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)

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 16:21):

dundargoc submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 16:23):

dundargoc deleted PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 16:26):

dundargoc submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (May 17 2024 at 18:08):

alexcrichton submitted PR review.

view this post on Zulip 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!

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 14:59):

alexcrichton updated PR #8642.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 15:01):

alexcrichton submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 15:21):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 15:32):

alexcrichton commented on PR #8642:

Thanks @dundargoc and @rockwotj!

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2024 at 15:46):

alexcrichton merged PR #8642.


Last updated: Dec 23 2024 at 12:05 UTC