Skip to content
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.

Sign up for GitHub

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

chore: Build Wasmer on musl #2003

Merged
merged 1 commit into from
Mar 5, 2021
Merged

chore: Build Wasmer on musl #2003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 75 additions & 25 deletions .github/workflows/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ jobs:
strategy:
fail-fast: false
matrix:
build: [linux-x64, macos-x64, macos-arm64, windows-x64, linux-aarch64]
MarkMcCaskey marked this conversation as resolved.
Show resolved Hide resolved
include:
- build: linux-x64
os: ubuntu-18.04
Expand Down Expand Up @@ -77,14 +76,15 @@ jobs:
CARGO_SCCACHE_VERSION: 0.2.14-alpha.0-parity
SCCACHE_AZURE_BLOB_CONTAINER: wasmerstoragesccacheblob
SCCACHE_AZURE_CONNECTION_STRING: ${{ secrets.SCCACHE_AZURE_CONNECTION_STRING }}
TARGET: ${{ matrix.target }}
steps:
- uses: actions/checkout@v2
- name: Set up libstdc++ on Linux
if: matrix.build == 'linux-x64'
run: |
sudo apt-get update -y
sudo apt-get install -y --allow-downgrades libstdc++6=8.4.0-1ubuntu1~18.04
sudo apt-get install --reinstall g++-8
if: matrix.os == 'ubuntu-18.04'
- name: Install Rust ${{ matrix.rust }}
uses: actions-rs/toolchain@v1
with:
Expand Down Expand Up @@ -125,7 +125,6 @@ jobs:
~/.cargo/registry
~/.cargo/git
key: ${{ matrix.build }}-${{ matrix.target }}-cargo-${{ hashFiles('Cargo.lock') }}-v1
# Install sccache
- uses: actions/cache@v2
with:
path: ${{ runner.tool_cache }}/cargo-sccache
Expand Down Expand Up @@ -173,7 +172,7 @@ jobs:
- name: Test
run: |
make test
if: matrix.target != 'aarch64-apple-darwin'
if: matrix.build != 'macos-arm64'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are target and build the same here? It seems like they could be different

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matrix.target and matrix.os are names defined by either Rust or GitHub. matrix.build is something we chosse on our side so I prefer to rely on our naming convention here.

If GitHub forces us to change the os this will not impact the workflow. Same for target (even if it's unlikely they will change)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that makes sense, I just want to check that there aren't extra semantics here. If I remember correctly, arm64 Mac used target or some other key for a very specific purpose and I think the difference might have been important because of cross-compilation or something

Copy link
Contributor Author

@jubianchi jubianchi Mar 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkMcCaskey yeah, to build for M1 we cross compile so we define (in the matrix) the Rust target to use and we were also using this in our conditionals.

But now, we have a problem: when GitHub will support M1 directly, we won't cross-compile anymore (we won't force the target through the matrix). Without my change, this would also require to change all the conditionals. With my change, we only update the matrix and we are done.

- name: Test C API
run: |
make test-capi
Expand All @@ -190,9 +189,9 @@ jobs:
run: |
make build-wasmer
- name: Build Wapm binary
if: needs.setup.outputs.DOING_RELEASE == '1'
run: |
make build-wapm
if: needs.setup.outputs.DOING_RELEASE == '1'
- name: Install Nightly Rust for Headless
uses: actions-rs/toolchain@v1
with:
Expand All @@ -202,6 +201,7 @@ jobs:
components: "rust-src"
if: needs.setup.outputs.DOING_RELEASE == '1'
- name: Build Minimal Wasmer Headless
if: needs.setup.outputs.DOING_RELEASE == '1'
run: |
cargo install xargo
echo "" >> Cargo.toml
Expand All @@ -216,17 +216,6 @@ jobs:
echo "codegen-units = 1" >> Cargo.toml
echo "rpath = false" >> Cargo.toml
make build-wasmer-headless-minimal
if: needs.setup.outputs.DOING_RELEASE == '1'
- name: Copy target binaries
run: |
mkdir -p target/release
cp target/${{matrix.target}}/release/wasmer* target/release
cp target/${{matrix.target}}/release/libwasmer* target/release
if [ -d "wapm-cli" ]; then
mkdir -p wapm-cli/target/release
cp wapm-cli/target/${{matrix.target}}/release/wapm* wapm-cli/target/release
fi
if: matrix.target
jubianchi marked this conversation as resolved.
Show resolved Hide resolved
- name: Dist
run: |
make distribution
Expand All @@ -243,7 +232,7 @@ jobs:
make test-integration
if: matrix.run_integration_tests && matrix.os != 'windows-latest'
- name: Cross compile from Linux
if: matrix.os == 'ubuntu-18.04'
if: matrix.build == 'linux-x64'
shell: bash
run: |
ls target/release
Expand Down Expand Up @@ -290,6 +279,64 @@ jobs:
if-no-files-found: error
retention-days: 1

test-docker:
name: Test on ${{ matrix.build }}
runs-on: ubuntu-latest
needs: setup
strategy:
fail-fast: false
matrix:
include:
- build: linux-musl-x64
image: alpine:latest
rust: 1.49
artifact_name: 'wasmer-linux-musl-amd64'
steps:
- uses: actions/checkout@v2

- uses: addnab/docker-run-action@v1
with:
image: ${{ matrix.image }}
options: -v ${{ github.workspace }}:/work
jubianchi marked this conversation as resolved.
Show resolved Hide resolved
run: |
set -e

# Set up tools
apk add musl-dev curl make libtool libffi-dev gcc automake autoconf git

# Install Rust ${{ matrix.rust }}
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rustup.sh
sh ./rustup.sh -y
source ~/.cargo/env
rm -f ./rustup.sh

# Set up Rust
rustup toolchain install ${{ matrix.rust }}
rustup default ${{ matrix.rust }}

# Change working directory
cd /work

# Test
make test test-capi

# Build C API, Wasmer & Wapm
make build-capi build-wasmer build-wapm

# Dist
make distribution

# Run integration tests
export WASMER_DIR=`pwd`/package
make test-integration
- name: Upload Artifacts
uses: actions/upload-artifact@v2
with:
name: ${{ matrix.artifact_name }}
path: dist
if-no-files-found: error
retention-days: 1

test-cross-compile-on-linux:
name: Test cross-compile on linux
needs: [setup, test]
Expand Down Expand Up @@ -340,15 +387,14 @@ jobs:
# wasmer run artifacts/cross/cross_compiled_from_mac/win_from_mac.wjit -- --eval "function greet(name) { return JSON.stringify('Hello, ' + name); }; print(greet('World'));"

release:
needs: [setup, test, test-cross-compile-on-linux, test-cross-compile-on-mac] #, test-cross-compile-on-win]
needs: [setup, test, test-docker, test-cross-compile-on-linux, test-cross-compile-on-mac] #, test-cross-compile-on-win]
runs-on: ubuntu-latest
if: needs.setup.outputs.DOING_RELEASE == '1'
steps:
- name: Download the Artifacts
uses: actions/download-artifact@v2
with:
path: artifacts

- name: Create Release
id: create_release
uses: actions/create-release@v1
Expand All @@ -359,7 +405,6 @@ jobs:
release_name: Release ${{ needs.setup.outputs.VERSION }}
draft: true
prerelease: false

- name: Upload Release Asset Windows Installer
uses: actions/upload-release-asset@v1
env:
Expand All @@ -369,7 +414,6 @@ jobs:
asset_path: artifacts/wasmer-windows-amd64/WasmerInstaller.exe
asset_name: wasmer-windows.exe
asset_content_type: application/vnd.microsoft.portable-executable

- name: Upload Release Asset Windows
uses: actions/upload-release-asset@v1
env:
Expand All @@ -379,7 +423,6 @@ jobs:
asset_path: artifacts/wasmer-windows-amd64/wasmer.tar.gz
asset_name: wasmer-windows-amd64.tar.gz
asset_content_type: application/gzip

- name: Upload Release Asset Linux amd64
uses: actions/upload-release-asset@v1
env:
Expand All @@ -389,7 +432,16 @@ jobs:
asset_path: artifacts/wasmer-linux-amd64/wasmer.tar.gz
asset_name: wasmer-linux-amd64.tar.gz
asset_content_type: application/gzip

- name: Upload Release Asset Linux amd64 (musl)
id: upload-release-asset-linux-musl-amd64
uses: actions/upload-release-asset@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
upload_url: ${{ steps.create_release.outputs.upload_url }}
asset_path: artifacts/wasmer-linux-musl-amd64/wasmer.tar.gz
asset_name: wasmer-linux-musl-amd64.tar.gz
asset_content_type: application/gzip
jubianchi marked this conversation as resolved.
Show resolved Hide resolved
- name: Upload Release Asset Mac amd64
uses: actions/upload-release-asset@v1
env:
Expand All @@ -399,7 +451,6 @@ jobs:
asset_path: artifacts/wasmer-darwin-amd64/wasmer.tar.gz
asset_name: wasmer-darwin-amd64.tar.gz
asset_content_type: application/gzip

- name: Upload Release Asset Mac arm64
uses: actions/upload-release-asset@v1
env:
Expand All @@ -409,7 +460,6 @@ jobs:
asset_path: artifacts/wasmer-darwin-arm64/wasmer.tar.gz
asset_name: wasmer-darwin-arm64.tar.gz
asset_content_type: application/gzip

- name: Upload Release Asset Linux aarch64
uses: actions/upload-release-asset@v1
env:
Expand Down
67 changes: 41 additions & 26 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,21 +295,34 @@ capi_compiler_features := --features $(subst $(space),$(comma),$(filter-out llvm
ifneq (, $(filter 1, $(IS_DARWIN) $(IS_LINUX)))
bold := $(shell tput bold 2>/dev/null || echo -n '')
green := $(shell tput setaf 2 2>/dev/null || echo -n '')
yellow := $(shell tput setaf 3 2>/dev/null || echo -n '')
reset := $(shell tput sgr0 2>/dev/null || echo -n '')
endif

HOST_TARGET=$(shell rustup show | grep 'Default host: ' | cut -d':' -f2 | tr -d ' ')

TARGET_DIR := target/release

ifneq (, $(TARGET))
TARGET_DIR := target/$(TARGET)/release
endif

$(info -----------)
$(info $(bold)$(green)INFORMATION$(reset))
$(info -----------)
$(info )
$(info Host Target: `$(bold)$(green)$(HOST_TARGET)$(reset)`.)
ifneq (, $(TARGET))
# We use spaces instead of tabs to indent `$(info)`
# otherwise it's considered as a command outside a
# target and it will fail.
$(info Build Target: $(bold)$(green)$(TARGET)$(reset) $(yellow)($(TARGET_DIR))$(reset))
endif
ifneq (, $(LIBC))
# We use spaces instead of tabs to indent `$(info)`
# otherwise it's considered as a command outside a
# target and it will fail.
$(info C standard library: $(bold)$(green)$(LIBC)$(reset))
# We use spaces instead of tabs to indent `$(info)`
# otherwise it's considered as a command outside a
# target and it will fail.
$(info C standard library: $(bold)$(green)$(LIBC)$(reset))
endif
$(info Enabled Compilers: $(bold)$(green)$(subst $(space),$(reset)$(comma)$(space)$(bold)$(green),$(compilers))$(reset).)
$(info Compilers + engines pairs (for testing): $(bold)$(green)${compilers_engines}$(reset))
Expand All @@ -325,7 +338,6 @@ $(info --------------)
$(info )
$(info )


############
# Building #
############
Expand Down Expand Up @@ -579,13 +591,13 @@ package-wapm:
mkdir -p "package/bin"
ifneq (, $(filter 1, $(IS_DARWIN) $(IS_LINUX)))
if [ -d "wapm-cli" ]; then \
cp wapm-cli/target/release/wapm package/bin/ ;\
cp wapm-cli/$(TARGET_DIR)/wapm package/bin/ ;\
echo "#!/bin/bash\nwapm execute \"\$$@\"" > package/bin/wax ;\
chmod +x package/bin/wax ;\
fi
else
if [ -d "wapm-cli" ]; then \
cp wapm-cli/target/release/wapm package/bin/ ;\
cp wapm-cli/$(TARGET_DIR)/wapm package/bin/ ;\
fi
ifeq ($(IS_DARWIN), 1)
codesign -s - package/bin/wapm
Expand All @@ -606,9 +618,9 @@ endif
package-wasmer:
mkdir -p "package/bin"
ifeq ($(IS_WINDOWS), 1)
cp target/release/wasmer.exe package/bin/
cp $(TARGET_DIR)/wasmer.exe package/bin/
else
cp target/release/wasmer package/bin/
cp $(TARGET_DIR)/wasmer package/bin/
ifeq ($(IS_DARWIN), 1)
codesign -s - package/bin/wasmer
endif
Expand All @@ -621,25 +633,28 @@ package-capi:
cp lib/c-api/wasmer_wasm.h* package/include
cp lib/c-api/wasm.h* package/include
cp lib/c-api/README.md package/include/README.md
ifeq ($(IS_WINDOWS), 1)
cp target/release/wasmer_c_api.dll package/lib/wasmer.dll
cp target/release/wasmer_c_api.lib package/lib/wasmer.lib
else
ifeq ($(IS_DARWIN), 1)

# Windows
if [ -f $(TARGET_DIR)/wasmer_c_api.dll ]; then \
cp $(TARGET_DIR)/wasmer_c_api.dll package/lib/wasmer.dll ;\
fi
if [ -f $(TARGET_DIR)/wasmer_c_api.lib ]; then \
cp $(TARGET_DIR)/wasmer_c_api.lib package/lib/wasmer.lib ;\
fi

# For some reason in macOS arm64 there are issues if we copy constantly in the install_name_tool util
rm -f package/lib/libwasmer.dylib
cp target/release/libwasmer_c_api.dylib package/lib/libwasmer.dylib
cp target/release/libwasmer_c_api.a package/lib/libwasmer.a
# Fix the rpath for the dylib
install_name_tool -id "@rpath/libwasmer.dylib" package/lib/libwasmer.dylib
else
# In some cases the .so may not be available, for example when building against musl (static linking)
if [ -f target/release/libwasmer_c_api.so ]; then \
cp target/release/libwasmer_c_api.so package/lib/libwasmer.so ;\
fi;
cp target/release/libwasmer_c_api.a package/lib/libwasmer.a
endif
endif
if [ -f $(TARGET_DIR)/libwasmer_c_api.dylib ]; then \
cp $(TARGET_DIR)/libwasmer_c_api.dylib package/lib/libwasmer.dylib ;\
install_name_tool -id "@rpath/libwasmer.dylib" package/lib/libwasmer.dylib ;\
fi

if [ -f $(TARGET_DIR)/libwasmer_c_api.so ]; then \
cp $(TARGET_DIR)/libwasmer_c_api.so package/lib/libwasmer.so ;\
fi
if [ -f $(TARGET_DIR)/libwasmer_c_api.a ]; then \
cp $(TARGET_DIR)/libwasmer_c_api.a package/lib/libwasmer.a ;\
fi
Comment on lines -624 to +657
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this new approach. It's more “error-proof”.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually just about to comment that I think it's more error prone as we'll silently fail if we're missing any of these files and will just have to wait until a user reports or we notice that we didn't release one of the files we intended to.

Ideally our packaging logic would be tested and not live in our CI file. We've lost a ton of things done in CI over time from all the CI migrations we've done -- I wouldn't put anything too critical there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think the above code is better though, but we do need tests for our packaging logic and the change above makes the need for tests greater.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkMcCaskey the problem with our previous strategy here was we relied on the host OS to define the files to copy.

This is not always the right thing to do, for example when we cross-compile this is why I changed that. Also, our strategy was not adaptative: if a user decides to change his build configuration (for example move from cdylib to staticlib) he'll have to change the Makefile. With this change, everything will automatically work.

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense, my one concern is just that it's less specific. Rather than encoding an expected state of the world "given condition X, we must have files Y, Z, ..." we just take whatever is there. So any bug that affects the files we produce will now go unnoticed. Put another way, the new code is less brittle and picky, but it's so non-brittle and picky that it won't complain if things are broken, where the old code would.

I think the real fix is to just have actual testing for our release process but I think that's out of scope for this PR.

Copy link
Contributor

@Hywan Hywan Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's accept the present patch, and then immediately open a new PR to test the package/ directory? We also have various scheduled tests that run basedwasmer-nightly (like in wasmer-go, wasmer-php etc.).


package-docs: build-docs build-docs-capi
mkdir -p "package/docs"
Expand Down
1 change: 1 addition & 0 deletions bors.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ status = [
"Audit",
"Code lint",
"Test on linux-x64",
"Test on linux-musl-x64",
"Test on linux-aarch64",
"Test on macos-x64",
"Test on windows-x64",
Expand Down
17 changes: 15 additions & 2 deletions lib/c-api/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
//! * setting `inline-c` up.

use cbindgen::{Builder, Language};
use std::{env, fs, path::PathBuf};
use std::{
env, fs,
path::{Path, PathBuf},
};

const PRE_HEADER: &'static str = r#"
// Define the `ARCH_X86_X64` constant.
Expand Down Expand Up @@ -569,7 +572,17 @@ fn build_inline_c_env_vars() {
} else if cfg!(target_os = "macos") {
"libwasmer_c_api.dylib".to_string()
} else {
"libwasmer_c_api.so".to_string()
let path = format!(
"{shared_object_dir}/{lib}",
shared_object_dir = shared_object_dir,
lib = "libwasmer_c_api.so"
);

if Path::new(path.as_str()).exists() {
"libwasmer_c_api.so".to_string()
} else {
"libwasmer_c_api.a".to_string()
}
}
);
}
5 changes: 5 additions & 0 deletions lib/c-api/tests/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# ignore wasm-c-api binaries
wasm-c-api-*
test-*
wasm-c-api/example/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related to musl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope.

It can be extracted to a dedicated PR if needed.


# Unignore files ending with `.c` (i.e. `wasm-c-api-wasi.c`)
!*.c
!wasm-c-api/example/*.c
!wasm-c-api/example/*.cc
!wasm-c-api/example/*.wasm
!wasm-c-api/example/*.wat