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

Add wasm32-wasi to recognized arch/os #8096

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Add wasm32-wasi to recognized arch/os #8096

merged 3 commits into from
Apr 21, 2022

Conversation

TerrorJack
Copy link
Collaborator

This patch adds wasm32 to known archs, and wasi to known oss. It's a part of the work of adding webassembly support to ghc at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7632.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 15, 2022

Thank you for the PR. Please kindly compare with the very recent #8065 and ask in either of the PRs if you need any help.

@TerrorJack
Copy link
Collaborator Author

@Mikolaj Thanks for reminding; would you take another look now?

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM. CI is borked independently (cache?).

@TerrorJack
Copy link
Collaborator Author

I also suspect a stale cache issue here; would you accept a patch that bumps cache name from ${{ runner.os }}-${{ matrix.ghc }}-${{ github.sha }} to ${{ runner.os }}-${{ matrix.ghc }}-0-${{ github.sha }}? (And people can bump from 0 in the future in similar cases)

@Mikolaj
Copy link
Member

Mikolaj commented Apr 15, 2022

I also suspect a stale cache issue here; would you accept a patch that bumps cache name from ${{ runner.os }}-${{ matrix.ghc }}-${{ github.sha }} to ${{ runner.os }}-${{ matrix.ghc }}-0-${{ github.sha }}? (And people can bump from 0 in the future in similar cases)

@fgaz: is this similar to what you proposed on #hackage?

@fgaz
Copy link
Member

fgaz commented Apr 15, 2022

I proposed to add a workflow to fill (=> flush) the cache so we don't have to increase that number every time, but it doesn't seem to be working yet (#8097). Maybe pr and branch caches are separate?

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Is it right to add the architecture to cabal before adding it to ghc? Shouldn't it be the opposite?

@fgaz
Copy link
Member

fgaz commented Apr 15, 2022

#8097 didn't work, @TerrorJack could you open a pr with the workaround you mentioned?

@jneira
Copy link
Member

jneira commented Apr 15, 2022

I would use a date (v20220416?) instead version number to make more evident the last time we had to bump it (as we will need to invalidate it regularly even without corruption issues until we redesign it)

(about why I think we will need invalidate it regularly here: #7952 (comment))

@TerrorJack
Copy link
Collaborator Author

could you open a pr with the workaround you mentioned?

I've added a commit in place to see whether the workaround works; if you think it is better to open a separate PR and merge that first, I'll also be fine to do that.

Is it right to add the architecture to cabal before adding it to ghc? Shouldn't it be the opposite?

I'll open GHC MR to add the arch/os pair soon. But still, I'd like the Cabal PR to be opened and reviewed earlier, since it blocks other pending PRs of ghc boot libs, due to cabal check failure. It'll take time for this change to land into a cabal release, better sooner than later.

@TerrorJack
Copy link
Collaborator Author

CI is now green 👀

@gbaz gbaz added the squash+merge me Tell Mergify Bot to squash-merge label Apr 21, 2022
@mergify mergify bot merged commit 709799a into haskell:master Apr 21, 2022
@TerrorJack TerrorJack deleted the wasm32-wasi-pr branch April 22, 2022 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants