laytan / setup-odin

MIT License
15 stars 2 forks source link

wasm-ld: not found #30

Closed thetarnav closed 5 months ago

thetarnav commented 5 months ago

When trying to build to wasm, I get an error that wasm-ld is not found. I believe this could be solved by adding llvm-<version>/bin to the PATH. In my own script I was doing this before and it did the trick. Although it may only be the case on ubuntu.

echo "/usr/lib/llvm-14/bin" >>$GITHUB_PATH
export PATH="/usr/lib/llvm-14/bin:$PATH"
laytan commented 5 months ago

Curiously that is what should be done already: https://github.com/laytan/setup-odin/blob/fbed82c2fbba4012331d14d89eae76953a4bcb0c/src/index.js#L181 I will have to see where it is going wrong.

thetarnav commented 5 months ago

Yeah it's not getting added to path, can't call wasm-ld nor llvm-config. Weird because /home/runner/odin is being added properly.

laytan commented 5 months ago

It does not add it when you are pulling a pre-compiled release (cause llvm isn't needed with releases), is that what you are doing?

thetarnav commented 5 months ago

yes, just this on ubuntu-latest

- uses: laytan/setup-odin@v2
  with:
    token: ${{ secrets.GITHUB_TOKEN }}
    release: latest
laytan commented 5 months ago

Aha, then I don't really know how to fix this, the release just bundles llvm and not any of the wasm-ld or other binaries. And if I make it still install llvm alongside the release it kinda defeats the purpose.

I think the ubuntu runners come with llvm 14 installed, so maybe I can just add it to path in that case but idk.

thetarnav commented 5 months ago

I think the ubuntu runners come with llvm 14 installed, so maybe I can just add it to path in that case but idk.

This is what I'm naively doing in my own script.

If Odin releases were to switch to building with LLVM 17, would it be a problem if wasm-ld from 14 was used?

laytan commented 5 months ago

I don't think that is a problem, you can in general use older linkers with newer code (as long as the version difference isn't enormous). The next Odin release will be LLVM 17 btw.

I think adding the preinstalled llvm to path is ultimately the best thing here.