-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add docker image that can be used to build projects with wasi-sdk sysroot #271
Conversation
sdk.Dockerfile
Outdated
\ | ||
curl -sS https://apt.llvm.org/llvm-snapshot.gpg.key | gpg --dearmor > /etc/apt/trusted.gpg.d/llvm.gpg && \ | ||
echo "deb [signed-by=/etc/apt/trusted.gpg.d/llvm.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${LLVM_VERSION} main" >> /etc/apt/sources.list.d/llvm.list && \ | ||
echo "deb-src [signed-by=/etc/apt/trusted.gpg.d/llvm.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${LLVM_VERSION} main" >> /etc/apt/sources.list.d/llvm.list && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe just use the llvm that ships with jammy? Is 13.0 recent enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would worry me to use a version of LLVM older than what's published with wasi-sdk for potential incompatibilities, currently it looks like wasi-sdk is on 15.0.6
sdk.docker.cmake
Outdated
set(CMAKE_C_COMPILER /usr/bin/clang-15) | ||
set(CMAKE_CXX_COMPILER /usr/bin/clang++-15) | ||
set(CMAKE_AR /usr/bin/llvm-ar-15) | ||
set(CMAKE_RANLIB /usr/bin/llvm-ranlib-15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that you set CC/CXX etc in the environment maybe this is redundant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With cmake, CC/CXX do get sourced from env variables, but AR RANLIB still don't unfortunately. It makes the state clearest I think to have it all in one place here rather than relying on environment for some but not all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sbc100 - applied some cleanups and added some responses
sdk.Dockerfile
Outdated
\ | ||
curl -sS https://apt.llvm.org/llvm-snapshot.gpg.key | gpg --dearmor > /etc/apt/trusted.gpg.d/llvm.gpg && \ | ||
echo "deb [signed-by=/etc/apt/trusted.gpg.d/llvm.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${LLVM_VERSION} main" >> /etc/apt/sources.list.d/llvm.list && \ | ||
echo "deb-src [signed-by=/etc/apt/trusted.gpg.d/llvm.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-${LLVM_VERSION} main" >> /etc/apt/sources.list.d/llvm.list && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would worry me to use a version of LLVM older than what's published with wasi-sdk for potential incompatibilities, currently it looks like wasi-sdk is on 15.0.6
sdk.docker.cmake
Outdated
set(CMAKE_C_COMPILER /usr/bin/clang-15) | ||
set(CMAKE_CXX_COMPILER /usr/bin/clang++-15) | ||
set(CMAKE_AR /usr/bin/llvm-ar-15) | ||
set(CMAKE_RANLIB /usr/bin/llvm-ranlib-15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With cmake, CC/CXX do get sourced from env variables, but AR RANLIB still don't unfortunately. It makes the state clearest I think to have it all in one place here rather than relying on environment for some but not all
docker/Dockerfile.dockerignore
Outdated
@@ -0,0 +1 @@ | |||
# Overrides top level .dockerignore to allow adding files to the image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this? What top level file is being refereed too? Can this file simply be call .dockerignore
or does it need the longer name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the .dockerignore
file in the root of the repository, which excludes all files since they aren't used in that build. This build does need files added.
The naming scheme is required to be like this for docker to pick it up as an override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead just remove the top level one.. I don't know why that exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sbc100 - I've removed both. I suspect one motivation may have been the size of the submodules when initializing docker, but even on a slowish CI machine it seems to be just 4s so seems like an overoptimization
https://github.com/anuraaga/wasi-sdk/actions/runs/3728956980/jobs/6324422776#step:9:174
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain more? Surly docker should not need to read the sub modules directory should it? Doesn't it just include what we tell it to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately no, docker begins by sending the entire context to the daemon - usually this is the current directory,
.
and is recursive. So to prevent transferring things during this step you need to use dockerignore, the values in ADD statements in the Dockerfile don't influence it.
That seems rather unfortunate! So even if I only add one tiny file to the image the docker create command will be O(size of everything my my CWD)
. I did not know that.
In that case maybe lets leave the top level .dockerignore
but have it only ignore the large directories such as dist
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, can we run docker from the subdirectory and ADD ../dist
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, docker won't allow adding files that are't in the context.
In that case maybe lets leave the top level .dockerignore but have it only ignore the large directories such as dist.
Since this image needs dist (to be able to package it), I think it would result in the two dockerignore files again, one ignoring dist at top level and the other not. Just to confirm that's ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think we should just have one top level dockerignore, and it should just specify the stuff we don't want (i.e. the submodules).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sbc100 - added a top level .dockerignore that excludes /src, the submodules folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sbc100 applied some fixes and clarifications
docker/Dockerfile.dockerignore
Outdated
@@ -0,0 +1 @@ | |||
# Overrides top level .dockerignore to allow adding files to the image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the .dockerignore
file in the root of the repository, which excludes all files since they aren't used in that build. This build does need files added.
The naming scheme is required to be like this for docker to pick it up as an override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comment/question.
Not sure if @sunfishcode or @pchickey have more to add?
I don't have anything to add here; if people would find this useful, let's do it. |
docker/Dockerfile
Outdated
ENV RANLIB llvm-ranlib-${LLVM_VERSION} | ||
|
||
ENV CFLAGS --target=wasm32-wasi --sysroot=/wasi-sysroot | ||
ENV CXXFLAGS --target=wasm32-wasi --sysroot=/wasi-sysroot -fno-exceptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we don't currently automatically set -fno-exceptions
in wasi-sdk.cmake
, does it make sense to set it here? Seems odd to diverge from the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks - I've found the flag helpful when building with wasi-sdk but agree both should be changed at the same time so removed it for now
Thanks for merging @sbc100! I see the action logs look fine and show a pushed image https://github.com/WebAssembly/wasi-sdk/actions/runs/3787010175/jobs/6438424135 But the package isn't showing up here https://github.com/orgs/WebAssembly/packages?repo_name=wasi-sdk I believe this could be because of a repository or organization setting preventing public packages - would you be able to check on that? Thanks! |
The binary linux SDK in version 17 requires a newer glibc than we have available, so we have to use our own patched clang. This follows the approach in: WebAssembly/wasi-sdk#271
Indeed, it shows up for me there, but is marked as "private". I'll see if I can find the setting to make it public. |
It looks like changing the package visibility settings is an organization-wide permission which I don't have. @dschuff @sunfishcode might be able to do it: https://github.com/orgs/WebAssembly/packages/container/wasi-sdk/settings |
The binary linux SDK in version 17 requires a newer glibc than we have available, so we have to use our own patched clang. This follows the approach in: WebAssembly/wasi-sdk#271
@anuraaga, once the package permissions are figured out, can you follow this up with a quick note (here or in the project |
Fixes #176
Based on the issue it seems there is interest in this with some help, so sent a PR to add building and publishing of a wasi-sdk docker image. emscripten provides an official emsdk docker image to make it easier to build existing projects to webassembly, for example wabt uses it to build their emscripten artifact. A similar docker image for wasi-sdk can allow C++ projects to more easily provide webassembly for use outside the browser - for example, building wabt and binaryen to wasi will allow full webassembly toolchains without the need for installing multiple tools or C toolchains.
The docker image is built from main pushes with
main
and the github sha as tags, and for tag pushes with the tag name. You can see what it looks like in my fork https://github.com/anuraaga/wasi-sdk/pkgs/container/wasi-sdkUsing ghcr allows publishing docker with no worries of account or secret management. In the future, it could also be published to Docker Hub if desired but I personally don't see much point in OSS projects doing so anymore.
I have confirmed that wabt (cmake project) and re2 (make project) build fine with this image.
This approach does not use the LLVM built with wasi-sdk but instead installs packages and configures environment variables to use the wasi sysroot / target. This is to support multiplatform docker images - with arm64 becoming more popular it is important to provide images that work with it. While it's possible to build arm64 with qemu on GitHub Actions, for a monster like LLVM it's impractical.
autotools, cmake and ninja are also included to represent a common set of build tools used by C++ projects out there - the more included the bigger the image gets, but this feels like good coverage.
Let me know what you think