chore: Build Wasmer on musl #2003

jubianchi · 2021-01-11T14:49:50Z

This patch adds a specific build for musl. Currently, it will only support JIT engine.

I also changes the workflow definition a bit so we don't depend on the OS name but rather on the environment ID which is our convention.

Closes #1482
Closes #1766

Description

Review

Add a short description of the the change to the CHANGELOG.md file

jubianchi · 2021-01-11T14:49:59Z

bors try

bors · 2021-01-11T14:51:02Z

try

Build failed:

Audit

jubianchi · 2021-01-11T15:38:13Z

bors try

bors · 2021-01-11T15:39:24Z

try

Build failed:

Audit

jubianchi · 2021-01-11T20:14:20Z

bors try

bors · 2021-01-11T23:01:03Z

try

Timed out.

jubianchi · 2021-01-12T08:53:20Z

bors try

jubianchi · 2021-01-12T09:53:08Z

bors try

bors · 2021-01-12T09:53:09Z

try

Already running a review

jubianchi · 2021-01-12T10:27:52Z

bors try-

jubianchi · 2021-01-12T10:30:03Z

bors try

bors · 2021-01-12T10:49:59Z

try

Build failed:

Test on linux-x64

jubianchi · 2021-01-12T10:54:29Z

bors try-

jubianchi · 2021-01-12T10:54:59Z

bors try

bors · 2021-01-12T11:15:43Z

try

Build failed:

Test on linux-x64

jubianchi · 2021-01-12T17:42:04Z

bors try

bors · 2021-02-13T17:25:08Z

try

Build succeeded:

jubianchi · 2021-02-13T17:57:18Z

bors try

bors · 2021-02-13T18:57:50Z

try

Build succeeded:

.cargo/config.toml

.github/workflows/main.yaml

Makefile

nlewycky · 2021-02-19T21:27:33Z

lib/c-api/tests/.gitignore

@@ -1,6 +1,11 @@
 # ignore wasm-c-api binaries
 wasm-c-api-*
 test-*
+wasm-c-api/example/*


Is this change related to musl?

nope.

It can be extracted to a dedicated PR if needed.

jubianchi · 2021-02-19T22:58:07Z

bors try

bors · 2021-02-20T00:07:00Z

try

Build succeeded:

2133: fix: Use space-indent when calling functions outside of rules r=Hywan a=jubianchi This should fix failures in: * https://github.com/wasmerio/wasmer-nightly/pull/14/checks?check_run_id=1938612785 * https://github.com/wasmerio/wasmer-nightly/pull/13/checks?check_run_id=1938633577 This will unblock #2003 Co-authored-by: jubianchi <[email protected]>

.github/workflows/main.yaml

Hywan

The asset upload duplication must be fixed, but apart from that, I'm approving this PR. Good job!

.github/workflows/main.yaml

Hywan · 2021-02-22T08:20:20Z

Makefile

-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


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

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

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.

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

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.

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.).

.github/workflows/main.yaml

MarkMcCaskey · 2021-02-22T14:09:39Z

.github/workflows/main.yaml

@@ -173,7 +172,7 @@ jobs:
      - name: Test
        run: |
          make test
-        if: matrix.target != 'aarch64-apple-darwin'
+        if: matrix.build != 'macos-arm64'


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

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)

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

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

.github/workflows/main.yaml

MarkMcCaskey · 2021-02-22T14:17:51Z

Makefile

-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


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

MarkMcCaskey · 2021-02-22T14:19:00Z

Makefile

-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


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.

chore: Update workflow to reflect what we have in wasmerio/wasmer#2003

Closes #1482 Closes #1766

jubianchi · 2021-03-05T20:24:31Z

bors r+

bors · 2021-03-05T21:36:23Z

Build succeeded:

jubianchi requested review from Hywan and MarkMcCaskey as code owners January 11, 2021 14:49

bors bot added a commit that referenced this pull request Jan 11, 2021

Try #2003:

055522d

jubianchi force-pushed the musl-build branch from d3a5185 to 9dad483 Compare January 11, 2021 15:37

bors bot added a commit that referenced this pull request Jan 11, 2021

Try #2003:

350bde7

jubianchi force-pushed the musl-build branch from 9dad483 to 585005e Compare January 11, 2021 20:14

bors bot added a commit that referenced this pull request Jan 11, 2021

Try #2003:

a06b3d4

bors bot added a commit that referenced this pull request Jan 12, 2021

Try #2003:

fe940dc

jubianchi force-pushed the musl-build branch from 585005e to a575b59 Compare January 12, 2021 09:52

bors bot added a commit that referenced this pull request Jan 12, 2021

Try #2003:

2b8d998

jubianchi force-pushed the musl-build branch from a575b59 to 362f136 Compare January 12, 2021 10:54

bors bot added a commit that referenced this pull request Jan 12, 2021

Try #2003:

4657cfe

jubianchi force-pushed the musl-build branch from 362f136 to e2b4661 Compare January 12, 2021 17:20

bors bot added a commit that referenced this pull request Jan 12, 2021

Try #2003:

44b4f22

jubianchi force-pushed the musl-build branch 2 times, most recently from dea45a5 to ce154a2 Compare February 13, 2021 17:56

bors bot added a commit that referenced this pull request Feb 13, 2021

Try #2003:

5b952bc

jubianchi mentioned this pull request Feb 15, 2021

feat(makefile) Simplify the prelude + build-capi includes all available compilers #2123

Merged

1 task

jubianchi force-pushed the musl-build branch from ce154a2 to ec0d85d Compare February 19, 2021 21:07

jubianchi added a commit to wasmerio/wasmer-nightly that referenced this pull request Feb 19, 2021

chore: Update workflow to reflect what we have in wasmerio/wasmer#2003

ac50070

jubianchi force-pushed the musl-build branch from ec0d85d to ecf1683 Compare February 19, 2021 21:14

nlewycky reviewed Feb 19, 2021

View reviewed changes

jubianchi added a commit to wasmerio/wasmer-nightly that referenced this pull request Feb 19, 2021

chore: Update workflow to reflect what we have in wasmerio/wasmer#2003

9fb16ab

jubianchi mentioned this pull request Feb 19, 2021

fix: Use space-indent when calling functions outside of rules #2133

Merged

jubianchi force-pushed the musl-build branch from ecf1683 to bbfb99b Compare February 19, 2021 22:56

bors bot added a commit that referenced this pull request Feb 19, 2021

Try #2003:

df5e466

Hywan reviewed Feb 22, 2021

View reviewed changes

.github/workflows/main.yaml Show resolved Hide resolved

Hywan approved these changes Feb 22, 2021

View reviewed changes

MarkMcCaskey reviewed Feb 22, 2021

View reviewed changes

jubianchi added a commit to wasmerio/wasmer-nightly that referenced this pull request Feb 22, 2021

chore: Update workflow to reflect what we have in wasmerio/wasmer#2003

d319608

jubianchi added a commit to wasmerio/wasmer-nightly that referenced this pull request Feb 22, 2021

chore: Update workflow to reflect what we have in wasmerio/wasmer#2003

bd6cbee

jubianchi added a commit to wasmerio/wasmer-nightly that referenced this pull request Feb 22, 2021

Merge pull request #13 from wasmerio/build-musl

0932d88

chore: Update workflow to reflect what we have in wasmerio/wasmer#2003

chore: Build Wasmer on musl

5290185

Closes #1482 Closes #1766

jubianchi force-pushed the musl-build branch from bbfb99b to 5290185 Compare March 3, 2021 16:08

bors bot merged commit ea4ea47 into master Mar 5, 2021

bors bot deleted the musl-build branch March 5, 2021 21:36

chore: Build Wasmer on musl #2003

chore: Build Wasmer on musl #2003

Conversation

jubianchi commented Jan 11, 2021

Description

Review

jubianchi commented Jan 11, 2021

bors bot commented Jan 11, 2021

try

jubianchi commented Jan 11, 2021

bors bot commented Jan 11, 2021

try

jubianchi commented Jan 11, 2021

bors bot commented Jan 11, 2021

try

jubianchi commented Jan 12, 2021

jubianchi commented Jan 12, 2021

bors bot commented Jan 12, 2021

try

jubianchi commented Jan 12, 2021

jubianchi commented Jan 12, 2021

bors bot commented Jan 12, 2021

try

jubianchi commented Jan 12, 2021

jubianchi commented Jan 12, 2021

bors bot commented Jan 12, 2021

try

jubianchi commented Jan 12, 2021

bors bot commented Feb 13, 2021

try

jubianchi commented Feb 13, 2021

bors bot commented Feb 13, 2021

try

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubianchi commented Feb 19, 2021

bors bot commented Feb 20, 2021

try

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkMcCaskey Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Hywan Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubianchi Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubianchi commented Mar 5, 2021

bors bot commented Mar 5, 2021

MarkMcCaskey Feb 23, 2021 •

edited

Loading

Hywan Mar 4, 2021 •

edited

Loading

jubianchi Mar 3, 2021 •

edited

Loading