Fix build and tests for wasm32-wasip2 target

[catamorphism]

Trying to build wasi-libc for the wasm32-wasip2 target and run the tests, I noticed a few issues:

• TARGET_TRIPLE was being used before definition, in the definition of OBJDIR.
• The ?= operator instead of = was being used to set TARGET_TRIPLE to wasm32-wasip2.
  This always left the target as wasm32-wasi, since it was defined to that in a previous line.
• builtins wasn't being built by default, so building the tests would fail because it was unable to find libclang_rt.builtins.a.
• make test didn't take theWASI_SNAPSHOT variable into consideration.
• When building tests, clang --target=wasm32-wasip2 was building a P2 component,
  which caused the subsequent wasm-tools component new to fail because the input
  was already a component.

This PR fixes these issues. Now I'm able to run the tests with WASI_SNAPSHOT=p2.

[alexcrichton]

FWIW this is the configuration CI uses to test with an example log here. Can you clarify in the context of that the command you're using which is failing before and passing after? I've no doubt the makefiles could be cleaned up a bit, but at the source it'd be unexpected if p2 tests didn't work at all

[catamorphism]

@alexcrichton I'll take a look. Also, I pushed some code that should have been on a different branch -- I'll fix that.

[catamorphism]

Starting from scratch, what I'm running is:

$ WASI_SNAPSHOT=p2 make CC=clang AR=llvm-ar NM=llvm-nm

and looking at the compile commands, I see:

clang -O2 -DNDEBUG --target=wasm32-wasi [...]

The build of wasi-libc does work for me if I use, as in the CI config:

$ WASI_SNAPSHOT=p2 TARGET_TRIPLE=wasm32-wasip2 make CC=clang AR=llvm-ar NM=llvm-nm

But the logic in the Makefile seems to suggest that this should work with just WASI_SNAPSHOT being set, without having to explicitly set TARGET_TRIPLE in the command line.

[catamorphism]

(I know there are some CI errors, which I'll also look at -- but in the meantime, would appreciate comments on whether this is the right direction.)

[alexcrichton]

Ok yeah that sounds reasonable to support using just WASI_SNAPSHOT without TARGET_TRIPLE too. Otherwise it looks CI previously succeeded because it's using clang-16 which I think predates wasm-component-ld and such. What I might recommend here is:

• Update the clang used in CI to clang-19 (that repro's for me locally). That should reproduce the failures you're seeing locally in CI too.
• Keep the TARGET_TRIPLE-related fixes as that's good no matter what.
• Refactor how components are created/tested
  • Previously wasm-tools component new was executed, but that predates a lot of bits and pieces today so I'd drop that entirely as well as the whole *.core.wasm and *.component.wasm distinction, nowadays we should only emit *.wasm.
  • For wasip2 testing clang will complain that it can't execute wasm-component-ld (which internally does basically what wasm-tools component new does). For that you can arrange for wasm-component-ld to be installed on the builder (e.g. cargo install wasm-component-ld@VERSION, or something similar)

For the component-ld bits, if you're willing, my goal is to get the testing pretty close to what the toolchain does "for real" in the wasi-sdk artifacts which is to use wasm-component-ld to produce a component natively. If you'd prefer though I can also work on said refactoring if it's not clear how to apply it.

Also, if you'd like, it might be good to land some sort of small PR first so that way your PRs will auto-trigger CI. The permissions on this repository are such that first-time-contributors need approval for workflows so it might help the feedback cycle if you get a PR landed on the side (e.g. a typo fix, add a newline somewhere, etc)

[catamorphism]

Oh sure, I'll be happy to land a small PR. Will work on the rest of it once I've done that.

[alexcrichton]

For the various errors on CI here (sorry for the state of things, this is a lot...)

• The Windows failure I suspect is due to the inclusion of builtins which was, I think, never executed before on Windows. So I suspect you've discovered a preexisting issue where there's just some lack of windows compat in commands here-and-there. In the logs the tar command is tar xzf - -C build/wasm32-wasi/resource-dir/\lib\wasi\ --strip-components 1 so I think the \ characters are causing problems (probably a case we need to normalize / to \ for that specific command)
• The wasm32-wasip1-threads failures look like it's due to the sysroot being built as wasm32-wasi-threads instead of wasm32-wasip1-threads. I think that's due to TARGET_TRIPLE = wasm32-wasi-threads in this PR vs ?= before perhaps?
• The wasm32-wasip2 failure is due to using clang-16 which predates wasm-component-ld which means that your linker flag is unrecognized (as it's using wasm-ld, not wasm-component-ld). That should get sorted out by updating Clang.

For the second point about wasm32-wasi-threads vs wasm32-wasip1-threads what's happening is IMO basically the result of organic growth over time. Various knobs/inputs have been added to the Makefile here and there over the years for targets and such. Personally I wouldn't consider it out-of-scope to refactor this and slim things down to "just specify TARGET_TRIPLE and that's it" or something like that. Basically like you're finding with WASI_SNAPSHOT and TARGET_TRIPLE it's a bit weird keeping them in sync so I think it'd be reasonable to instead only have a single source of truth, TARGET_TRIPLE, and things are inferred from that. I realize though you probably didn't intend and/or want to do said refactoring, so that's also totally fine to defer to some future change/contributor. Mostly I wanted to at least put in writing that I think it's fine to change the build variables and don't see a need to rigorously commit to exactly what's already there.

[catamorphism]

@alexcrichton Re: updating clang to clang-19: I noticed that in https://github.com/WebAssembly/wasi-libc/blob/45252554b765e3db11d0ef5b41d6dd290ed33382/.github/workflows/main.yml there are several different clang versions used. For MacOS, for example, it's 15.0.7 instead of 16. And I assume you don't want to break clang 11, since there's an item for that. So are you suggesting changing the clang version to 19 everywhere, or only in the section under "Test various combinations of target triples"? Basically, do you want only clang-19 to be supported?

[alexcrichton]

If that works yeah I think that's reasonable to upgrade across-the-board. If that runs into snags though I think it's also reasonable to just upgrade wasm32-wasip2 and leave the others for later. I'd agree though that we don't want to drop older clang support just yet (at least not while it's "easy" to retain support for older lcang), so keeping 11 in the list of versions tested is fine.

[catamorphism]

@alexcrichton I've upgraded to clang-19 and fixed the Windows and threads builds. For wasm32-wasip2, the build is now failing in a way I can't reproduce locally. It looks like the check-symbols step fails, but with no output. If you get a chance to look at it and have any ideas, let me know. I'll keep trying to reproduce it in the meantime. Never mind, the output was misleading due to parallel make. check-symbols was fine, but wasm-component-ld fails. I'll take a look at that.

[alexcrichton]

Ah good point! I'd recommend editing the setup action to download precompiled binaries of wasm-component-ld and adding them to $PATH, and I think that should fix the final error

[catamorphism]

Waiting for CI so I can verify that wasm32-wasip2 is fixed, but I think all that's left to do is:

• your suggestion to refactor how components are created/tested
• removing WASI_SNAPSHOT (and THREAD_MODEL maybe?) and making TARGET_TRIPLE the single source of truth

[alexcrichton]

Nice! This looks reasonable to me to land, so wanna go ahead and land this as-is? The other bits can be follow-ups as necessary. Removing --skip-wit-component I think would be a nice-to-have, but the other bits of WASI_SNAPSHOT and THREAD_MODEL I think are fine to indefinitely defer to as-needed.

[catamorphism]

Sounds good! I'll squash and mark as ready for review.

[sbc100]

Why this version update? If we are bumping the min clang version needed to build libc do we also need to update the docs somewhere?

Should this be its own change, or is it related the wasip2 stuff?

[catamorphism]

Alex suggested the version update in #599 (comment) . It is related to the wasip2 stuff. I thought it made the most sense to update the version for all targets (except the LLVM 11 target).

Currently the root README.md doesn't specify a minimum clang version, although maybe it should?

[alexcrichton]

Yeah it's required to update to work with wasm32-wasip2, and if we're updating one I figured it was fine to update them all.

[sbc100]

If we still have at least one builder on LLVM 11 then i guess that means we are still testing the same min version. SGTM.

BTW why do the other needed updating but not that one?

[sbc100]

Oh, I guess its because we don't test any of the p3 stuff with the llvm 11 clang versio. lgtm then.

[alexcrichton]

Updating other builders is not required, but I'd also say it's more consistent to test the same version in the "main body of tests" instead of having a random mish mash of versions which is a historical record of latest-when-someone-last-touched-this which is tough to explain.