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

Specify default exe extension on wasm32 to be .wasm #8633

Merged
merged 1 commit into from
Dec 17, 2022

Conversation

TerrorJack
Copy link
Collaborator

@TerrorJack TerrorJack commented Dec 13, 2022

This PR changes default exe extension on wasm32 to be .wasm, following convention of other WebAssembly toolchains. This is a part of the effort tracked in https://gitlab.haskell.org/ghc/ghc/-/issues/22594.


Please include the following checklist in your PR:

@TerrorJack TerrorJack marked this pull request as ready for review December 13, 2022 11:25
@ulysses4ever
Copy link
Collaborator

@TerrorJack thanks for the patch and congrats with getting the necessary approvals! The procedure now is that you apply one of the two merge-me labels to hand it over to the bot.

In my opinion, this patch could be improved with a test. But I don't want to hold it hostage to a test.

@TerrorJack
Copy link
Collaborator Author

@ulysses4ever Thanks for the reminder!

Re label: it seems I'm not a contributor to this repo, so I don't see the option to edit labels on this PR. If I can be granted contributor right or someone adds a merge-me label here, that'll be appreciated :)

Re test: another MR in GHC (!9533) needs to land, and then it may be possible to test it meaningfully.

@ulysses4ever
Copy link
Collaborator

Ah, good to know!

@Mikolaj will see to the label/permissions business, I hope

@ulysses4ever ulysses4ever added the merge me Tell Mergify Bot to merge label Dec 13, 2022
@ulysses4ever
Copy link
Collaborator

In the meantime, I added the merge-me label.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 13, 2022

@TerrorJack: please kindly receive the Triage authority for our humble repo.

@TerrorJack
Copy link
Collaborator Author

The honor is mine :)

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Dec 15, 2022
Specify default exe extension on wasm32 to be .wasm, following the convention in other WebAssembly toolchains.
@mergify mergify bot merged commit cb2b639 into haskell:master Dec 17, 2022
@TerrorJack TerrorJack deleted the wasm-ext-wasm branch December 17, 2022 01:13
@jneira
Copy link
Member

jneira commented Jan 3, 2023

This could have a test? It seems to me that Cabal integration with wasm should be tested somehow

@TerrorJack
Copy link
Collaborator Author

@jneira I can think of a simple E2E test that invokes Setup.hs for a cabal exe component. But that needs to install a separate ghc in CI that targets wasm, and IIUIC all current cabal tests only rely on a single host GHC?

By any chance I'd be happy to add such a test if the idea sounds good to you; I'm working on unblocking wasm CI for GHC 9.6 release and after that I can revisit this issue.

@jneira
Copy link
Member

jneira commented Jan 4, 2023

It sounds great. I guess some e2e test for wasm would be needed sooner or later and well, maybe this pr is not the best place.
I only would ask to create an issue about such test, linking the already merged prs.

Cabal tests in ci uses the default ghc, yeah, but i think there are guards to run tests conditionally on ghc version and these tests will need it.

@jneira jneira mentioned this pull request Jan 4, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-test merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants