Add support for js-sources #8636

hsyl20 · 2022-12-14T16:21:06Z

This PR adds support for js-sources with GHC, not only GHCJS.

Caveat: for now, similarly to asm-sources and cmm-sources, js-sources only works for library components, not executable components. See #8639

Not sure how to add tests for this as it requires GHC >= 9.6 built for the JS target. I've only tested it locally.

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.

hsyl20 · 2022-12-20T09:02:18Z

I've opened #8639 to track the issue with executable components. It has a larger scope because asm-sources and cmm-sources are concerned too.

hsyl20 · 2022-12-20T09:10:48Z

@Mikolaj Could you or someone else take this PR over to ensure that it gets included in GHC 9.6 (and in GHC HEAD as soon as possible)? Users that try the JS backend will quickly face this issue (and #8639).

Mikolaj · 2022-12-20T09:32:49Z

Local tests are fine in this case. A tiny changelog file would be very welcome though.

@hsyl20: I presume this is ready for review? Based on this assumption, I've set the label and asked @bgamari to have a look (or delegate to somebody in GHC team that works on related issues).

Re including in GHC 9.6, we plan to release cabal 3.10 in step with GHC 9.6, so if this PR gets merged in 2022 or shortly thereafter, it's going to be included in GHC 9.6 and in cabal 3.10. I will bug people to make sure it doesn't just get forgotten.

hsyl20 · 2022-12-20T09:45:56Z

I've added a changelog. Now it's ready for review. :)

Mikolaj · 2022-12-20T10:14:49Z

Cabal/src/Distribution/Simple/GHC.hs

@@ -561,6 +561,7 @@ buildOrReplLib mReplFlags verbosity numJobs pkg_descr lbi lib clbi = do
                      , toNubListR (cxxSources libBi)
                      , toNubListR (cmmSources libBi)
                      , toNubListR (asmSources libBi)
+                      , toNubListR (jsSources  libBi)


This is adding JS to cLikeSources. Is "C-like" a misnomer or was the original intention to exclude JS? Change of plans? I don't see any "non-C-like sources" list in the codebase that we could add JS source to, instead, so this must be the right place.

Perhaps the C-like vs non-C-like distinction is a left over from some special cases for GHCJS? Is JS C-like enough when considered by GHC JS backend, but not C-like when fed to GHCJS?

Yes for the JS backend we've reworked the way JS sources are treated. They are now C-like because we've added support for a "compilation" from .js to .o files (basically a file renaming with a header added). It makes them less special for tools like Hadrian and cabal.

(in the GHCJS code path (Distribution.Simple.GHCJS) these files were directly passed as link options)

Maybe this deserves a NOTE JS Sources are C-Like or similar.

@Profpatsch: GHC note or cabal note? Or one of those and hyperlink from the other place?

I've added a comment explaining that JS sources are C-like in the JS backend.

Mikolaj

I've verified that this PR changes all the bits of code that are likely to need changing. However, I don't know this functionality enough to understand whether these changes make sense, so I have to trust the results of local testing by the PR author. This is unlikely to break other stuff, though.

jneira · 2023-01-03T17:01:24Z

hi, this looks great but it would be nice to know the plans to add or modify existing tests

jneira · 2023-01-03T20:42:20Z

Not sure how to add tests for this as it requires GHC >= 9.6 built for the JS target. I've only tested it locally.

A possible way would be write tests and run them conditionally on ghc version. They will be eventually executed everywhere.

jneira · 2023-01-04T21:52:20Z

This pr seems to be related with #8633 and i guess tests also would be related

hsyl20 · 2023-01-06T15:37:06Z

A possible way would be write tests and run them conditionally on ghc version. They will be eventually executed everywhere.

I'll try to do this.

This pr seems to be related with #8633 and i guess tests also would be related

The two PRs aren't related. This other one is for the wasm backend, this one is for the JS backend.

Mikolaj · 2023-01-12T18:58:51Z

We'd like to include this commit in the 3.10 branch that is to be cut RSN so, in the worst case, perhaps we could add the test afterwards. However, we need a second review, an in-depth one, ideally prompting some more documentation to be added, so I'm kindly asking @ben to help with that.

Mikolaj · 2023-01-14T15:50:15Z

BTW, if this

#8675

is done in finite time and we have 9.6.1-alpha1 in CI, the test would be able to run. Probably not on Windows, though, because our CI is quite thoroughly broken with GHC >= 9.4 on Windows.

hsyl20 · 2023-01-16T09:43:49Z

@Mikolaj You also will need a build of GHC cross-compiling for the JS target. Not sure if it is planned?

hsyl20 · 2023-01-16T10:16:39Z

I've added a test but I couldn't run it with cabal-tests. It doesn't find the correct ghc-pkg and there is no --with-ghc-pkg flag to use.

hsyl20 · 2023-01-16T10:34:32Z

Oops I forgot to push some changes, now done. Run example:

/home/hsyl20/projects/ghc/javascript-backend/libraries/Cabal/dist-newstyle/build/x86_64-linux/ghc-9.2.4/cabal-install-3.9.0.0/x/cabal/build/cabal/cabal --with-ghc='/home/hsyl20/projects/ghc/javascript-backend/_build/stage1/bin/js-unknown-ghcjs-ghc' --with-ghc-pkg='/home/hsyl20/projects/ghc/javascript-backend/_build/stage1/bin/js-unknown-ghcjs-ghc-pkg' run demo
Warning: this is a debug build of cabal-install with assertions enabled.
Warning: Unknown/unsupported 'ghc' version detected (Cabal 3.9.0.0 supports
'ghc' version < 9.6):
/home/hsyl20/projects/ghc/javascript-backend/_build/stage1/bin/js-unknown-ghcjs-ghc
is version 9.7.20230112
Resolving dependencies...
Build profile: -w ghc-9.7.20230112 -O1
In order, the following will be built (use -v for more details):
 - jssources-0 (lib) (first run)
 - jssources-0 (exe:demo) (first run)
Warning: this is a debug build of cabal-install with assertions enabled.
Configuring library for jssources-0..
Warning: this is a debug build of cabal-install with assertions enabled.
Preprocessing library for jssources-0..
Building library for jssources-0..
[1 of 1] Compiling Lib              ( src/Lib.hs, /home/hsyl20/projects/ghc/javascript-backend/libraries/Cabal/cabal-testsuite/PackageTests/JS/JsSources/dist-newstyle/build/js-ghcjs/ghc-9.7.20230112/jssources-0/
Warning: this is a debug build of cabal-install with assertions enabled.
Warning: this is a debug build of cabal-install with assertions enabled.
Configuring executable 'demo' for jssources-0..
Warning: this is a debug build of cabal-install with assertions enabled.
Preprocessing executable 'demo' for jssources-0..
Building executable 'demo' for jssources-0..
[1 of 1] Compiling Main             ( demo/Main.hs, /home/hsyl20/projects/ghc/javascript-backend/libraries/Cabal/cabal-testsuite/PackageTests/JS/JsSources/dist-newstyle/build/js-ghcjs/ghc-9.7.20230112/jssources-0/x/demo/build/demo/demo-tmp/Main.o )
[2 of 2] Linking /home/hsyl20/projects/ghc/javascript-backend/libraries/Cabal/cabal-testsuite/PackageTests/JS/JsSources/dist-newstyle/build/js-ghcjs/ghc-9.7.20230112/jssources-0/x/demo/build/demo/demo.jsexe
Hello JS!

Mikolaj

Thanks a lot for the test. I think it's fine as the first test of any functionality in any way related to JS.

cabal-testsuite/PackageTests/JS/JsSources/cabal.test.hs

hsyl20 · 2023-01-16T11:35:31Z

I've also fixed skipIfGhcVersion in passing. It probably shouldn't belong to this MR but time is short.

Mikolaj

I've also fixed skipIfGhcVersion in passing. It probably shouldn't belong to this MR but time is short.

Thanks a lot. It is. :)

Mikolaj · 2023-01-16T14:52:52Z

Splendid. That's descent behaviour and these are not blockers for this PR, IMHO.

Mikolaj · 2023-01-19T15:02:56Z

@bgamari: thank you.

@hsyl20: Success! When you are ready, please kindly set the label (squash+merge_me, I suppose?) and then it's going to be merged in 2 days, unless further changes are made.

hsyl20 · 2023-01-19T15:30:14Z

@Mikolaj Done! Thanks :)

Mikolaj · 2023-01-21T22:22:29Z

@mergify refresh

mergify · 2023-01-21T22:24:54Z

refresh

✅ Pull request refreshed

Mikolaj · 2023-01-21T22:34:20Z

Mergify is fussy today, so let me prod it manually.

Mikolaj · 2023-01-21T22:34:32Z

@mergify rebase

mergify · 2023-01-21T22:35:29Z

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

mergify · 2023-01-21T22:35:30Z

rebase

✅ Branch has been successfully rebased

jneira

late to the party but thanks for working on this (and add the test!)

hsyl20 mentioned this pull request Dec 20, 2022

Support for asm-sources, cmm-sources, and js-sources in executable components #8639

Closed

hsyl20 changed the title ~~Draft: add support for js-sources~~ Add support for js-sources Dec 20, 2022

Mikolaj requested a review from bgamari December 20, 2022 09:23

Mikolaj added attention: needs-review compiler: ghcjs re: ghc 9.6 labels Dec 20, 2022

Mikolaj added the js backend label Dec 20, 2022

Mikolaj added this to the Considered for 3.10 milestone Dec 20, 2022

Mikolaj reviewed Dec 20, 2022

View reviewed changes

Mikolaj approved these changes Dec 20, 2022

View reviewed changes

Mikolaj removed the request for review from bgamari January 2, 2023 15:23

jneira added the attention: needs-test label Jan 3, 2023

Mikolaj requested a review from bgamari January 12, 2023 18:54

Mikolaj requested changes Jan 16, 2023

View reviewed changes

cabal-testsuite/PackageTests/JS/JsSources/cabal.test.hs Outdated Show resolved Hide resolved

Mikolaj requested a review from mpickering January 16, 2023 11:10

Mikolaj approved these changes Jan 16, 2023

View reviewed changes

hsyl20 mentioned this pull request Jan 18, 2023

GHC JS browser introduction blog post/tutorial input-output-hk/engineering#24

Merged

bgamari approved these changes Jan 19, 2023

View reviewed changes

Mikolaj removed the request for review from mpickering January 19, 2023 15:01

Mikolaj removed attention: needs-test attention: needs-review labels Jan 19, 2023

hsyl20 added the squash+merge me Tell Mergify Bot to squash-merge label Jan 19, 2023

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 21, 2023

haskell deleted a comment from mergify bot Jan 21, 2023

hsyl20 added 7 commits January 21, 2023 22:35

Add support for js-sources

8b8f810

Add changelog

8da42fb

Add comment about C-like sources

9d379dc

Add tests

e0e8bd7

Fix test

bda945c

Fix skipUnless and factorize

a1e7529

Fix skipIfGhcVersion

b55d479

cocreature force-pushed the hsyl20/support-js-sources branch from 9f4ea9d to b55d479 Compare January 21, 2023 22:35

mergify bot merged commit 605a3c6 into haskell:master Jan 22, 2023

jneira reviewed Jan 23, 2023

View reviewed changes

Mikolaj mentioned this pull request Feb 8, 2023

Regression: cannot build streamly-0.8.3 #8741

Closed

Mikolaj mentioned this pull request Feb 18, 2023

CI: add GHC pre-release #8754

Merged

ffaf1 mentioned this pull request Mar 7, 2023

Add 3.10.1.0 changelogs and remove the used up tiny changlog files #8831

Merged

Add support for js-sources #8636

Add support for js-sources #8636

Uh oh!

Conversation

hsyl20 commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsyl20 commented Dec 20, 2022

Uh oh!

hsyl20 commented Dec 20, 2022

Uh oh!

Mikolaj commented Dec 20, 2022

Uh oh!

hsyl20 commented Dec 20, 2022

Uh oh!

Mikolaj Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

hsyl20 Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

Profpatsch Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

Mikolaj Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

hsyl20 Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Mikolaj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jneira commented Jan 3, 2023

Uh oh!

jneira commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jneira commented Jan 4, 2023

Uh oh!

hsyl20 commented Jan 6, 2023

Uh oh!

Mikolaj commented Jan 12, 2023

Uh oh!

Mikolaj commented Jan 14, 2023

Uh oh!

hsyl20 commented Jan 16, 2023

Uh oh!

hsyl20 commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsyl20 commented Jan 16, 2023

Uh oh!

Mikolaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hsyl20 commented Jan 16, 2023

Uh oh!

Mikolaj left a comment

Choose a reason for hiding this comment

Uh oh!

Mikolaj commented Jan 16, 2023

Uh oh!

Mikolaj commented Jan 19, 2023

Uh oh!

hsyl20 commented Jan 19, 2023

Uh oh!

Mikolaj commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Jan 21, 2023

✅ Pull request refreshed

Uh oh!

Mikolaj commented Jan 21, 2023

Uh oh!

Mikolaj commented Jan 21, 2023

Uh oh!

mergify bot commented Jan 21, 2023

hsyl20 commented Dec 14, 2022 •

edited

Loading

Mikolaj left a comment •

edited

Loading

jneira commented Jan 3, 2023 •

edited

Loading

hsyl20 commented Jan 16, 2023 •

edited

Loading

Mikolaj commented Jan 21, 2023 •

edited

Loading