Add s390x GHA workflow (cross-compile) #2380

SoniEx2 · 2024-02-01T02:11:01Z

This adds a cross-compile workflow for s390x. Heavily based on #1971 , but runs significantly faster.

sbc100

Change itself looks good!

If the step take too long we could run it post-commit I support.. or make it optional to wait for it to complete. Lets see how long it takes..

SoniEx2 · 2024-02-01T02:30:20Z

just a bit faster =^-^= (also yes, it is expected to fail for now.)

also, cc @alexreinking ?

alexreinking · 2024-02-02T18:07:36Z

.github/workflows/build.yml

+ - name: mkdir
+ run: mkdir -p out
+ - name: cmake
+ run: cmake -DCMAKE_TOOLCHAIN_FILE=../scripts/TC-${{matrix.arch}}.cmake .. -G Ninja -DWITH_WASI=ON -DWERROR=OFF -Werror=dev -Wno-deprecated
+ working-directory: out


This can be simplified a little:

Suggested change

- name: mkdir

run: mkdir -p out

- name: cmake

run: cmake -DCMAKE_TOOLCHAIN_FILE=../scripts/TC-${{matrix.arch}}.cmake .. -G Ninja -DWITH_WASI=ON -DWERROR=OFF -Werror=dev -Wno-deprecated

working-directory: out

- name: cmake

run: cmake -G Ninja -S . -B out -DCMAKE_TOOLCHAIN_FILE=./scripts/TC-${{matrix.arch}}.cmake -DWITH_WASI=ON -DWERROR=OFF -Werror=dev -Wno-deprecated

we would rather keep this consistent with the build job, aside from the toolchain file (and, currently, the -DWERROR=OFF).

This isn't consistent everywhere in this file, though... look at line 161 for instance. I'd sooner move the file towards having fewer unnecessary steps (which just add noise to logs)

hmm... @sbc100 thoughts?

Fine with me.

IIRC the support for -S + -B flags in cmake wasn't always supported but as a long as are running with recent enough cmake everywhere this seems fine to me.

Maybe make separate PR to convert to using -S + -B everywhere before we lang this change?

The -S and -B flags were introduced in CMake 3.13 while the minimum version for wabt is CMake 3.16 and has been for a while. No hazard with using them here.

hmm even the places that use -S + -B aren't consistent about order. would you prefer -S . -B out -G Ninja or -G Ninja -S . -B out?

No opinion there..

alexreinking · 2024-02-02T18:08:59Z

.github/workflows/build.yml

+ options: --health-cmd distccmon-text --health-interval 5s --health-start-period 5m debian:latest bash -c "apt-get update && apt-get install -y g++-s390x-linux-gnu distcc && distccd --daemon --no-detach"
+ ports:
+ - 3632:3632
+ steps:


Another simplification:

Suggested change

steps:

env:

CMAKE_GENERATOR: Ninja

QEMU_LD_PREFIX: /usr/${{matrix.arch}}-linux-gnu/

steps:

Then get rid of -G Ninja and the other QEMU_LD_PREFIX settings below.

We use -G Ninja elsewhere in this file though... so maybe leave that one as is for consistency?

@sbc100 - totally non-critical, just a suggestion! I think QEMU_LD_PREFIX does lower the risk for forgetting it if the workflow has to change.

that works...? well, let's find out! (github actions documentation is not exactly the easiest thing to read...)

(huh, so it does!)

alexreinking · 2024-02-02T18:12:36Z

Thanks for the great work @SoniEx2! Super happy to see this effort picked back up -- and super happy to see it improved! I commented a couple easy ways to simplify the workflow steps.

I no longer work at Google, though, so I'd appreciate it if you would change the co-authored-by email to [email protected].

sbc100 · 2024-02-02T19:49:52Z

lgtm in general, but lets see how long it takes (and make sure it passes :)

SoniEx2 · 2024-02-02T19:57:39Z

it'll pass after #2382 and #2340 .

SoniEx2 · 2024-02-05T23:19:43Z

the only blocker now is #2340 , it'd be really nice to get this merged...

SoniEx2 · 2024-02-07T04:15:45Z

there we go!

SoniEx2 · 2024-02-08T01:59:32Z

does this need anything else?

sbc100 · 2024-02-08T02:16:28Z

does this need anything else?

How long does it take compared the current slowest CI step?

keithw · 2024-02-08T02:24:10Z

The previous slowest step was the CIFuzz (which goes for exactly 10 minutes of fuzzing); this one takes about 17-18 minutes. Seems tolerable to me but this will be the new tentpole.

sbc100

lgtm % nits

sbc100 · 2024-02-08T02:27:17Z

.github/workflows/build.yml

+ if: ${{ matrix.arch != 's390x' }} # currently broken...
+ - name: c-api-tests
+ run: cmake --build out --target run-c-api-tests
+ if: ${{ matrix.arch != 's390x' }}


Give that s390x is the only arch these blocks are now dead code aren't they?

this is explicitly intended to make it easy to add more archs (arm64 anyone?) but you're right that there's a slight readability tradeoff here.

I guess I'd rather see that added later once we do actually want more than one cross arch tested here.

But I don't feel too strongly, since its not much code and its already written.

sbc100 · 2024-02-08T02:28:00Z

.github/workflows/build.yml

@@ -185,3 +185,55 @@ jobs:
 - run: make clang-debug
 - name: tests (wasm2c tests excluding memory64)
 run: ./test/run-tests.py wasm2c --exclude-dir memory64
+
+ build-arch:


How about calling this build-s390x? Maybe build-arch harks of Arch linux?

hmm, maybe build-cross?

SoniEx2 · 2024-02-09T00:09:40Z

btw now that we have #2386 we re-enabled run-unittests!

(that just leaves run-c-api-tests as dead code)

Co-Authored-By: Alex Reinking <[email protected]>

SoniEx2 · 2024-02-11T21:36:03Z

btw qemu does not emulate stack overflows correctly, would have to run the entire thing in machine emulation for that. maybe something to do as future work? (not really relevant when not using signal handler tho)

SoniEx2 · 2024-02-12T13:21:02Z

is there anything else missing for this? (we took out the c-api-tests since c-api is unsupported on big-endian... maybe component model will provide a better bridge between big-endian and wasm...)

sbc100 · 2024-02-12T17:52:11Z

Happy to go ahead and land this now.

Can you remind me for the record why you care about wabt support for little endian? I'm sorry if I've asked before and forgotten the answer? Is it because you are targeting s390 yourself in in production?

SoniEx2 · 2024-02-12T18:15:50Z

big endian*

we really care about old macs, amigas, the wii, the sega genesis. retro computing really. not so much the s390x, the s390x is a bit out of reach for us. maybe one day it'll become retro computing tho! that would be kinda cool.

sbc100 · 2024-02-12T18:18:19Z

big endian*

we really care about old macs, amigas, the wii, the sega genesis. retro computing really. not so much the s390x, the s390x is a bit out of reach for us. maybe one day it'll become retro computing tho! that would be kinda cool.

Very interesting. And you are wanting to run wasm on actual hardware using these old devices? Or are you wanting to run wasm inside emulators of these devices?

SoniEx2 · 2024-02-12T18:27:11Z

yep! it'd be cool to run it on real hardware =^-^=

sbc100 reviewed Feb 1, 2024

View reviewed changes

SoniEx2 force-pushed the s390x-rewrite-again-sigh branch 2 times, most recently from 699325f to 743a722 Compare February 1, 2024 16:03

alexreinking reviewed Feb 2, 2024

View reviewed changes

SoniEx2 force-pushed the s390x-rewrite-again-sigh branch 2 times, most recently from 7bc923d to c7b1a74 Compare February 2, 2024 19:15

SoniEx2 requested a review from sbc100 February 2, 2024 19:36

SoniEx2 force-pushed the s390x-rewrite-again-sigh branch from c7b1a74 to c4c5957 Compare February 2, 2024 20:40

SoniEx2 force-pushed the s390x-rewrite-again-sigh branch from c4c5957 to 709eaa2 Compare February 7, 2024 03:54

sbc100 approved these changes Feb 8, 2024

View reviewed changes

SoniEx2 force-pushed the s390x-rewrite-again-sigh branch 4 times, most recently from 1938a2b to e0842b0 Compare February 9, 2024 00:05

Add s390x GHA workflow

c11434c

Co-Authored-By: Alex Reinking <[email protected]>

SoniEx2 force-pushed the s390x-rewrite-again-sigh branch from e0842b0 to c11434c Compare February 9, 2024 19:53

sbc100 merged commit 0562fd5 into WebAssembly:main Feb 12, 2024
17 checks passed

SoniEx2 deleted the s390x-rewrite-again-sigh branch February 12, 2024 18:13

sbc100 mentioned this pull request Mar 28, 2024

Set up community build bot for Linux/Aarch64 builds emscripten-core/emsdk#547

Closed

- steps:
+ env:
+ CMAKE_GENERATOR: Ninja
+ QEMU_LD_PREFIX: /usr/${{matrix.arch}}-linux-gnu/
+ steps:

Add s390x GHA workflow (cross-compile) #2380

Add s390x GHA workflow (cross-compile) #2380

Conversation

SoniEx2 commented Feb 1, 2024

sbc100 left a comment

Choose a reason for hiding this comment

SoniEx2 commented Feb 1, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoniEx2 Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

alexreinking commented Feb 2, 2024 • edited Loading

sbc100 commented Feb 2, 2024

SoniEx2 commented Feb 2, 2024

SoniEx2 commented Feb 5, 2024

SoniEx2 commented Feb 7, 2024

SoniEx2 commented Feb 8, 2024

sbc100 commented Feb 8, 2024

keithw commented Feb 8, 2024 • edited Loading

sbc100 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoniEx2 commented Feb 9, 2024

SoniEx2 commented Feb 11, 2024

SoniEx2 commented Feb 12, 2024

sbc100 commented Feb 12, 2024

SoniEx2 commented Feb 12, 2024

sbc100 commented Feb 12, 2024

SoniEx2 commented Feb 12, 2024

SoniEx2 commented Feb 1, 2024 •

edited

Loading

SoniEx2 Feb 2, 2024 •

edited

Loading

alexreinking commented Feb 2, 2024 •

edited

Loading

keithw commented Feb 8, 2024 •

edited

Loading