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

Support asm/cmm/js sources in executable components (#8639) #9061

Merged
merged 9 commits into from
Jul 16, 2023

Conversation

JoshMeredith
Copy link
Collaborator

@JoshMeredith JoshMeredith commented Jun 23, 2023

Implementation for #8639

The code for building each of c/cpp/asm/cmm/js is factorised into a local function buildExtraSources. The local function is separate per library/executable building - i.e. no factorisation between these two stages is attempted.

Added tests:

  • cabal-testsuite/PackageTests/CmmSourcesExe: based on the sibling CmmSources test, with the source directories and .cmm
    file moved into the executable, and the library removed
  • cabal-testsuite/PackageTests/JS/JsSourcesExe: as above for the .js source.

Please include the following checklist in your PR:

Bonus points for added automated tests!

@ulysses4ever ulysses4ever marked this pull request as draft June 23, 2023 18:02
@JoshMeredith JoshMeredith changed the title WIP support asm/cmm/js sources in executable components (#8639) Support asm/cmm/js sources in executable components (#8639) Jun 28, 2023
@JoshMeredith JoshMeredith marked this pull request as ready for review June 28, 2023 15:15
Cabal/src/Distribution/Simple/GHC.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/GHC.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/GHC.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/GHC.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/Simple/GHC.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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

@hsyl20
Copy link
Collaborator

hsyl20 commented Jul 13, 2023

Could someone review this PR, please? It would be nice to get it backported to 3.10 to avoid this surprising bug for users trying to use the JS backend in 9.8. Thanks!

@Kleidukos
Copy link
Member

@Mergifyio backport 3.10

@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2023

backport 3.10

✅ Backports have been created

@Kleidukos Kleidukos added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Jul 13, 2023
@hsyl20
Copy link
Collaborator

hsyl20 commented Jul 13, 2023

Thanks @Kleidukos !

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jul 15, 2023
@ulysses4ever
Copy link
Collaborator

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Jul 16, 2023

rebase

✅ Branch has been successfully rebased

@ulysses4ever
Copy link
Collaborator

Fourmolu is failing for an unrelated reason. A fix is in flight.

@mergify mergify bot merged commit 3350a6c into haskell:master Jul 16, 2023
mergify bot pushed a commit that referenced this pull request Jul 16, 2023
* WIP support asm/cmm/js sources in executable components (#8639)

* Factorise extra src code for lib/exe and add extra exe src tests

* Add extra sources to linking step

* lint

* lint

* Don't build js sources for executables on non-js hosts

* Fix cabal.out for CmmSourcesExe test and lint

* Update changelog

* Slight changes

(cherry picked from commit 3350a6c)

# Conflicts:
#	Cabal/src/Distribution/Simple/GHC.hs
@Mikolaj
Copy link
Member

Mikolaj commented Sep 4, 2023

@Kleidukos, @hsyl20, @JoshMeredith: It turns out nobody resolved the conflicts in the backport #9130 and so this is not slated for release in cabal 3.10.2.0, but only in cabal 3.12.1.0 (which we optimistically expect to get released just after 3.10.2.0). Is that a problem? Is there a volunteer for an emergency conflict resolution to squeeze this in into 3.10 one minute before midnight?

@hsyl20
Copy link
Collaborator

hsyl20 commented Sep 5, 2023

It shouldn't be a problem, we'll just recommend 3.12 for the JS backend.

@alt-romes
Copy link
Collaborator

@JoshMeredith

In this change, why does dynLinkerOpts not have jsObjs? I suppose we cannot dynamically link jsObjects?

@@ -1794,14 +1676,14 @@ gbuild verbosity numJobs pkg_descr lbi bm clbi = do
                 PD.extraFrameworkDirs bnfo
           , ghcOptInputFiles =
               toNubListR
-                [tmpDir </> x | x <- cLikeObjs ++ cxxObjs]
+                [tmpDir </> x | x <- cLikeObjs ++ cxxObjs ++ jsObjs ++ cmmObjs ++ asmObjs]
           }
       dynLinkerOpts =
         mempty
           { ghcOptRPaths = rpaths
           , ghcOptInputFiles =
               toNubListR
-                [tmpDir </> x | x <- cLikeObjs ++ cxxObjs]
+                [tmpDir </> x | x <- cLikeObjs ++ cxxObjs ++ cmmObjs ++ asmObjs]
           }
       replOpts =
         baseOpts

@Mikolaj
Copy link
Member

Mikolaj commented Dec 6, 2023

@JoshMeredith: a humble ping?

@hsyl20: do you perhaps know the answer off the top of you head?

@JoshMeredith
Copy link
Collaborator Author

Hi, sorry I saw this and forgot to send the reply.

Yes, it's intentional that we don't currently dynamically link JavaScript. Our team hasn't discussed if it's possible, maybe @hsyl20 or @luite have ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants