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.

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

Only install handlers when HAVE_SIGNAL_H #365

Merged

Conversation

amesgen
Copy link
Contributor

@amesgen amesgen commented Apr 2, 2023

This allows to run test suites on platforms that do not support signals, such as the new WASM backend in GHC 9.6.

@amesgen amesgen force-pushed the further-guard-install-handler branch from 50fa81a to 0a228dc Compare April 2, 2023 18:50
Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Is there a way to test this change?

@Bodigrim
Copy link
Collaborator

Bodigrim commented Apr 2, 2023

@amesgen would it be possible to add something like https://github.com/haskell/unix/blob/master/.github/workflows/ci-wasm32-wasi.yml ?

@amesgen
Copy link
Contributor Author

amesgen commented Apr 2, 2023

@amesgen would it be possible to add something like https://github.com/haskell/unix/blob/master/.github/workflows/ci-wasm32-wasi.yml ?

Yeah, I will try to add something here 👍

@amesgen
Copy link
Contributor Author

amesgen commented Apr 3, 2023

I pushed a commit running all tests except the resource-release-test as it depends on signals (which WASM/WASI does not have ATM).

Depends on haskellari/splitmix#73 as splitmix is a transitive test dependency.

@TerrorJack if you want to take a look: Given your blog post, is there a nicer way to treat WASM files as executables in PATH than in this PR?

@TerrorJack
Copy link

@amesgen The proot trick described in the blog post is really a huge hack that was only meant to be used before the ghc testsuite didn't support cross emulators, and I think it's better to not use that in this PR.

There is one little potential improvement: instead of your own wrapper script that calls wasmtime, you are recommended to use ~/.ghc-wasm/wasm-run/bin/wasmtime.sh foo.wasm --command_line_args instead. That wrapper script is what I use for the ghc testsuite; it automatically adds wasmtime flags to set up the filesystem, current directory and such.

@amesgen
Copy link
Contributor Author

amesgen commented Apr 3, 2023

There is one little potential improvement: instead of your own wrapper script that calls wasmtime, you are recommended to use ~/.ghc-wasm/wasm-run/bin/wasmtime.sh foo.wasm --command_line_args instead.

This works great locally, but on GHA, exit-status-tests.sh fails:

+ timeout 3 exit-status-test --result=False --slow=3 --fast=0 --num-threads=4 --quiet
+ [ 124 -eq 1 ]

I guess that this is due to one of the additional flags in wasmtime.sh. So I will leave it as is for now.

Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks, great work!

Some suggestion on the cache key for the new CI job.

path: |
~/.ghc-wasm/.cabal/store
dist-newstyle
key: ${{ runner.os }}-${{ matrix.ghc }}-${{ github.sha }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

matrix.ghc is undefined here, as there is no matrix strategy for this job.

I recommend actionlint to find such problems statically.

Also, the key should have component pertaining to the job, e.g.:

key: build-wasi-${{ runner.os }}-ghc-${{ env.FLAVOUR }}-sha-${{ github.sha }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks for the pointer to actionlint! Should now be fixed; I also pinned ghc-wasm-meta and included the rev in the cache key 👍

@amesgen amesgen force-pushed the further-guard-install-handler branch from 6c285e5 to 72b9d41 Compare April 4, 2023 13:14
Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes to caching!

@Bodigrim Bodigrim merged commit 88a0258 into UnkindPartition:master Apr 4, 2023
@amesgen amesgen deleted the further-guard-install-handler branch April 4, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants