search

snowleopard / hadrian

Hadrian: a new build system for the Glasgow Haskell Compiler. Now merged into the GHC tree!
https://gitlab.haskell.org/ghc/ghc/tree/master/hadrian
MIT License
208 stars 37 forks source link

pass 'threaded' flag for the GHC executable, to link against the threaded RTS #689

Closed alpmestan closed 6 years ago

alpmestan commented 6 years ago

This goes hand in hand with https://phabricator.haskell.org/D5146 I'm trying to get the findPtr fix landed in master so that we can switch CI back to whatever GHC HEAD is. This way, we could even wait for D5146 to be merged before re-running CI for this patch and making sure that nothing is broken, if we so desire.

We could have a shorter term hack of just passing -threaded ourself explicitly in the right place, but it's a lot hackier and I suspect the diff will not take months to be merged.

This should in theory fix the failure of T8242 (when testing a GHC built by hadrian of course):

$ "/home/alp/WT/ghc-hadrian/_tmp/stage1/bin/ghc" T8242.hs -dcore-lint -dcmm-lint -no-user-package-db -rtsopts -fno-warn-missed-specialisations -fshow-warning-groups -fdiagnostics-color=never -fno-diagnostics-show-caret -dno-debug-output  --interactive -v0 -ignore-dot-ghci -fno-ghci-history +RTS -I0.1 -RTS < T8242.genscript
T8242: setNumCapabilities: not supported in the non-threaded RTS

I launched a build with both this patch and the GHC one locally, it'll run T8242 at the end, I'll report back with the result.

alpmestan commented 6 years ago

The test now passes:

=====> T8242(ghci) 1 of 1 [0, 0, 0]
cd "testsuite/tests/rts/T8242.run" && "/home/alp/WT/ghc-hadrian/_tmp/stage1/bin/ghc" T8242.hs -dcore-lint -dcmm-lint -no-user-package-db -rtsopts -fno-warn-missed-specialisations -fshow-warning-groups -fdiagnostics-color=never -fno-diagnostics-show-caret -dno-debug-output  --interactive -v0 -ignore-dot-ghci -fno-ghci-history +RTS -I0.1 -RTS < T8242.genscript

SUMMARY for test run started at Thu Sep 13 00:58:03 2018 CEST
 0:00:01 spent to go through
       1 total tests, which gave rise to
       8 test cases, of which
       7 were skipped

       0 had missing libraries
       1 expected passes
       0 expected failures

(with both the GHC and the hadrian patches)

alpmestan commented 6 years ago

To be precise, this fixes exactly 4 tests failures.

snowleopard commented 6 years ago

@alpmestan Thank you! So, shall we merge this or do you prefer to wait for D5146 to land which makes this patch unnecessary?

alpmestan commented 6 years ago

@snowleopard Well, it's not really that this patch is not necessary. It's certainly useless without D5146 as without that diff there isn't a threaded flag to begin with. This patch just documents that we're explicitly requiring the ghc executables to be linked against the appropriate threaded runtime, it basically acknowledges the existence of that flag in hadrian by enabling it explicitly, even though it's "redundant" since the default value for that flag is True.

But yes, I'd recommend keeping this PR around until the diff lands, at which point we can rerun CI and merge if everything goes well. I've got some other failing tests to keep me busy in the meantime :-)

snowleopard commented 6 years ago

@alpmestan Thanks, sounds good! Let's wait.

alpmestan commented 6 years ago

The GHC patch has been merged: https://github.com/ghc/ghc/commit/99eb4595910f20c41734aa07a2da4db1f25512ae

I rebased this branch against the tip of master to trigger another CI build, to make sure everything's still fine.

snowleopard commented 6 years ago

@alpmestan Many thanks! Some CI bots timeout, but I think we can merge this now.