Skip to content

Add support for the GHC JavaScript backend (node.js) #292

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 2, 2023
Merged

Add support for the GHC JavaScript backend (node.js) #292

merged 5 commits into from
Aug 2, 2023

Conversation

luite
Copy link
Member

@luite luite commented Jun 20, 2023

This PR adds support to the process package to target the node.js JavaScript platform using the GHC JavaScript backend.

This allows among other things the GHC JavaScript backend to use custom Cabal Setup.hs scripts.

@luite
Add JavaScript support
e9a151e 
This adds support for the GHC JavaScript backend. The resulting code can
be run on node.js.
hsyl20
hsyl20 reviewed Jun 20, 2023
View reviewed changes
System/Process.hs Outdated
@@ -875,6 +897,26 @@ c_getProcessExitCode _ _ = ioError (ioeSetLocation unsupportedOperation "getProc
c_waitForProcess :: PHANDLE -> Ptr CInt -> IO CInt
c_waitForProcess _ _ = ioError (ioeSetLocation unsupportedOperation "waitForProcess")

#elif defined(javascript_HOST_ARCH)

-- XXX descriptive argument names
Copy link

Choose a reason for hiding this comment

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

Suggested change
-- XXX descriptive argument names

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops forgot to remove that comment when switching to short form imports

hsyl20
hsyl20 reviewed Jun 20, 2023
View reviewed changes
jsbits/process.js Outdated
@@ -0,0 +1,585 @@
//#OPTIONS: CPP
// XXX do we need this?
Copy link

Choose a reason for hiding this comment

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

Does it work if you remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Testsuite still passes if removed

hsyl20
hsyl20 approved these changes Jun 20, 2023
View reviewed changes
Copy link

@hsyl20 hsyl20 left a comment

Choose a reason for hiding this comment

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

LGTM!

@bgamari
Copy link
Contributor

bgamari commented Jun 21, 2023

What is the plan to ensure that this is tested once merged?

@luite
Copy link
Member Author

luite commented Jun 21, 2023

The tests for process in the GHC testsuite can be enabled again for JS (except the ones that rely on extra C source files), they're currently marked broken.

cabal test works after cabal configure "--with-compiler=javascript-unknown-ghcjs-ghc" "--with-hc-pkg=javascript-unknown-ghcjs-ghc-pkg" --enable-tests, but I'm not sure if we can get such a compiler in the GitHub CI matrix yet.

@bgamari
Copy link
Contributor

bgamari commented Jun 22, 2023

The tests for process in the GHC testsuite can be enabled again for JS (except the ones that rely on extra C source files), they're currently marked broken.

Yes, but in this case I'm a bit confused: At least some of these tests live in this repository and yet are still marked as broken. Are these really broken? Is there a list of tests which are fixed by this MR?

@doyougnu
Copy link

Ben is right, these tests should be enabled with this PR:

process003
process004
process006
process007
process008
process009
process010
process011
T1780
T4889
T8343
T4198

Ideally we would add a JS backend test runner to this repo so we can tests these tests. Unfortunately it looks like the CI for this repo uses matrix and stack to run the testsuite. So until we have stack support and an official release (I guess?) it'll be hard to test here unless we pulled in haskell.nix or set up a job using ghcup or something like that.

Perhaps we:

  1. @luite enable these tests for js for this PR
  2. open an MR on GHC that simply bumps the process and check the tests there

@luite
Copy link
Member Author

luite commented Jun 29, 2023

Sorry for late reply, I'm on holiday this week. I'll add a commit remove the js_broken for tests that are now passing.

@hsyl20
Copy link

hsyl20 commented Jul 3, 2023

@luite Is there a corresponding GHC MR that uses this code?

@luite
Copy link
Member Author

luite commented Jul 3, 2023

@luite Is there a corresponding GHC MR that uses this code?

Ah I suppose it would be useful to get a CI run there. I'll make an MR

@luite
Copy link
Member Author

luite commented Jul 3, 2023

GHC MR here: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10801

@luite
Copy link
Member Author

luite commented Jul 4, 2023

I missed a few expected results in the process package, and also some base and rts tests pass now, which I've adjusted in MR !10801. After this has been merged, the process submodule bump should include these changes.

@hsyl20
Copy link

hsyl20 commented Jul 18, 2023

LGTM!

@hsyl20
Copy link

hsyl20 commented Aug 1, 2023

@bgamari Could you merge this?

@bgamari bgamari merged commit 5ba847a into haskell:master Aug 2, 2023
@thomasjm thomasjm mentioned this pull request Feb 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsyl20 hsyl20 hsyl20 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luite @bgamari @doyougnu @hsyl20