honour the flavours advertised by our flavours.md document

[alpmestan]

In particular, this patches focuses on enabling back the dynamic-enabled ways for the runtime system, which required to fix a bug in libsuf.

[alpmestan]

If anyone could try to run hadrian/build.<something> --flavour=perf -j<something> (or even the test target) on their system (preferably Windows & OS X, that I don't have available to me right now), that would be lovely. @snowleopard You've got a Windows machine?

[alpmestan]

This fails to build in the quickest way (and maybe others) because it tries to copy the _debug variant of libHSrts which we do not produce/build. I'm looking into this.

[snowleopard]

Thanks @alpmestan! I've left some comments above. Please also make sure doc/flavours.md is updated.

@snowleopard You've got a Windows machine?

Yes, I have a Windows machine. I can give this a try when CI bots are green.

[alpmestan]

Thanks @alpmestan! I've left some comments above. Please also make sure doc/flavours.md is updated.

Why should it be updated? What this PR accomplishes it make hadrian actually honor what's decribed in flavours.md. Well, at least it tries to but doesn't quite succeed just yet.

Yes, I have a Windows machine. I can give this a try when CI bots are green.

OK, but they're testing just the quickest way while it would be very informative for me to have someone run a build that includes some dynamic-enabled ways like quick or perf. As you prefer though, any help is welcome. Worst case I'll get myself a Windows VM to get it all to work properly.

[snowleopard]

What this PR accomplishes it make hadrian actually honor what's decribed in flavours.md.

@alpmestan Ah, sorry, my bad! I think this is a consequence of having a rather convoluted logic in listing ways that get build. This also relates so some of my earlier comments. What about simply listing all ways for each flavour, just like in the flavours.md table? Yes, there will be some repetition, but at least it will be easy to understand and modify.

it would be very informative for me to have someone run a build that includes some dynamic-enabled ways like quick or perf

Sure, I've just started the quick build. Will report back!

[alpmestan]

Yes, I guess I'll just go back to using pure [vanilla] etc and just list everything explicitly everywhere.

[snowleopard]

@alpmestan Here is what I got on Windows:

Error when running Shake build system:
* _build/stage1/lib/bin/touchy.exe
* _build/stage1/rts/build/libHSrts-1.0-ghc8.7.20180917_l.dll
Error, file does not exist and no rule available:
  _build/stage1/rts/build/libHSrts-1.0-ghc8.7.20180917_l.dll
CallStack (from HasCallStack):
  error, called at src\Development\Shake\Internal\Rules\File.hs:180:58 in shake-0.16.1-6eodb56sJBtAOWmdpKWJIs:Development.Shake.Internal.Rules.File

And indeed -- I don't think we have any dll rules in Hadrian.

[alpmestan]

Oh dear, right, Rules.Library does not support .dlls... What do you want us to do here?

[snowleopard]

@alpmestan I guess we shouldn't build dynamic way on Windows until the missing rules are implemented.

[alpmestan]

Do you know what the make build system does there? I guess we can leave that for another PR, but discussing the plan might still belong to this one.

[snowleopard]

@alpmestan Here is the issue about dynamic way on Windows: #343. Let's leave this to a separate PR.

[alpmestan]

@snowleopard I was wondering a few minutes ago: it seems to me like there is some code in platformSupportsSharedLibs that should keep dynamic ways from being included. What am I missing?

[alpmestan]

My latest commit depends on (i.e requires) https://phabricator.haskell.org/D5166

This gives us finer-grained control over what flavours we want produced. quickest works again here, trying quick and perf now.

[snowleopard]

I was wondering a few minutes ago: it seems to me like there is some code in platformSupportsSharedLibs that should keep dynamic ways from being included. What am I missing?

dynamic ways are excluded on Windows, but your setting of dynamicGhcPrograms = True is currently unconditional, which I think is what triggers dll rules.

[alpmestan]

With this branch as it stands along with the aforementionned GHC patch (for rts.cabal.in), here's what we produce for the RTS, on my x86_64 NixOS system:

$ ls _perf/stage1/rts/build/*.a _perf/stage1/rts/build/*.so
_perf/stage1/rts/build/libCffi.a                             _perf/stage1/rts/build/libHSrts-1.0.a
_perf/stage1/rts/build/libCffi_debug.a                       _perf/stage1/rts/build/libHSrts-1.0_debug.a
_perf/stage1/rts/build/libCffi_debug_p.a                     _perf/stage1/rts/build/libHSrts-1.0_debug_p.a
_perf/stage1/rts/build/libCffi-ghc8.7.20180919_debug.so      _perf/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_debug.so
_perf/stage1/rts/build/libCffi-ghc8.7.20180919_l.so          _perf/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_l.so
_perf/stage1/rts/build/libCffi-ghc8.7.20180919.so            _perf/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919.so
_perf/stage1/rts/build/libCffi-ghc8.7.20180919_thr_debug.so  _perf/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_thr_debug.so
_perf/stage1/rts/build/libCffi-ghc8.7.20180919_thr_l.so      _perf/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_thr_l.so
_perf/stage1/rts/build/libCffi-ghc8.7.20180919_thr.so        _perf/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_thr.so
_perf/stage1/rts/build/libCffi_l.a                           _perf/stage1/rts/build/libHSrts-1.0_l.a
_perf/stage1/rts/build/libCffi_p.a                           _perf/stage1/rts/build/libHSrts-1.0_p.a
_perf/stage1/rts/build/libCffi_thr.a                         _perf/stage1/rts/build/libHSrts-1.0_thr.a
_perf/stage1/rts/build/libCffi_thr_debug.a                   _perf/stage1/rts/build/libHSrts-1.0_thr_debug.a
_perf/stage1/rts/build/libCffi_thr_debug_p.a                 _perf/stage1/rts/build/libHSrts-1.0_thr_debug_p.a
_perf/stage1/rts/build/libCffi_thr_l.a                       _perf/stage1/rts/build/libHSrts-1.0_thr_l.a
_perf/stage1/rts/build/libCffi_thr_p.a                       _perf/stage1/rts/build/libHSrts-1.0_thr_p.a

$ ls _quick/stage1/rts/build/*.a _quick/stage1/rts/build/*.so
_quick/stage1/rts/build/libCffi.a                             _quick/stage1/rts/build/libHSrts-1.0.a
_quick/stage1/rts/build/libCffi_debug.a                       _quick/stage1/rts/build/libHSrts-1.0_debug.a
_quick/stage1/rts/build/libCffi-ghc8.7.20180919_debug.so      _quick/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_debug.so
_quick/stage1/rts/build/libCffi-ghc8.7.20180919_l.so          _quick/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_l.so
_quick/stage1/rts/build/libCffi-ghc8.7.20180919.so            _quick/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919.so
_quick/stage1/rts/build/libCffi-ghc8.7.20180919_thr_debug.so  _quick/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_thr_debug.so
_quick/stage1/rts/build/libCffi-ghc8.7.20180919_thr_l.so      _quick/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_thr_l.so
_quick/stage1/rts/build/libCffi-ghc8.7.20180919_thr.so        _quick/stage1/rts/build/libHSrts-1.0-ghc8.7.20180919_thr.so
_quick/stage1/rts/build/libCffi_l.a                           _quick/stage1/rts/build/libHSrts-1.0_l.a
_quick/stage1/rts/build/libCffi_thr.a                         _quick/stage1/rts/build/libHSrts-1.0_thr.a
_quick/stage1/rts/build/libCffi_thr_debug.a                   _quick/stage1/rts/build/libHSrts-1.0_thr_debug.a
_quick/stage1/rts/build/libCffi_thr_l.a                       _quick/stage1/rts/build/libHSrts-1.0_thr_l.a

$ ls _quickest/stage1/rts/build/*.a
 _quickest/stage1/rts/build/libCffi.a       _quickest/stage1/rts/build/libHSrts-1.0.a
 _quickest/stage1/rts/build/libCffi_thr.a   _quickest/stage1/rts/build/libHSrts-1.0_thr.a
# there's no .so produced with quickest

[snowleopard]

@alpmestan Looks good! But now we have a new failure on all CI bots. For example, this is on Windows:

copyFile: does not exist (The system cannot find the file specified.)

Error when running Shake build system:
* _build/stage1/lib/package.conf.d/rts-1.0.conf
ExitFailure 1

And this is on Linux:

/home/travis/build/snowleopard/hadrian/ghc/_build/stage1/rts/build/libHSrts-1.0_debug.a: copyFile: does not exist (No such file or directory)

Error when running Shake build system:
* _build/stage1/lib/package.conf.d/rts-1.0.conf

[alpmestan]

@snowleopard Yes, I suspect that's because as I said above, this requires a GHC patch (https://phabricator.haskell.org/D5166) that we're not using in CI. Should I do like in #689 and just force them to use it (by checking out a fork of mine) just to get a little bit of green in the CI panel? =)

[snowleopard]

@alpmestan Aha, I see. Well, let's wait until D5166 lands, as otherwise Hadrian will essentially be broken for everyone unless they apply your patch locally.

[alpmestan]

@snowleopard Note that I'm not sure I'm done with this PR just yet, so there's no immediate rush anyway. The directory listing above was mainly there to show that the title of the PR is appropriate :grin:

[alpmestan]

I think I've now implemented everything I wanted for this patch. Feel free to review while we wait for the GHC patch to be processed. (or wait for later, when the patch lands and we can get useful results from CI)

[snowleopard]

@alpmestan There seem to be some duplication:

-- In src/Oracles/Flag.hs
    win <- windowsHost
    return $ not (win || badPlatform || solaris && solarisBroken)

-- In src/Settings/Default.hs
defaultDynamicGhcPrograms :: Action Bool
defaultDynamicGhcPrograms = do
  win <- windowsHost
  supportsShared <- platformSupportsSharedLibs
  return (not win && supportsShared)

Should the change in platformSupportsSharedLibs be reverted? If I understand correctly, the current implementation (i.e. without looking up windowsHost) mirrors the one in the Make build system.

[alpmestan]

I guess so. To be honest, I'm still a little confused that your Windows build tried to generate .dlls, given platformSupportsSharedLibs' definition. The check in there should have prevented it no? Windows should be a badPlatform.

[snowleopard]

@alpmestan I think before this PR platformSupportsSharedLibs did not determine build targets, but dynamicGhcPrograms did. Now, with your recent changes, dynamicGhcPrograms is defined taking platformSupportsSharedLibs into account, as it should.

[alpmestan]

OK, is there any chance you could confirm there's no problem anymore on Windows with those dyn libs? (if you can: remember you also need to apply the patch from https://phabricator.haskell.org/D5166)

[snowleopard]

@alpmestan Sure, I'll give this a try this week. And I think we don't need win <- windowsHost in this PR, since platformSupportsSharedLibs already accounts for Windows (unless I'm mistaken).

[alpmestan]

Removed the redundant check in my latest commit.

[alpmestan]

My GHC patch has been merged: https://github.com/ghc/ghc/commit/e3355b7c5955df8daab0f3fc75fe021f42d21dbb

I rebased this branch, let's see what CI says.

[alpmestan]

Wow, everything green so far, except the travis OS X build which timed out.

[snowleopard]

@alpmestan Awesome, thanks! Sorry I couldn't try it on my machine yet. I suggest we merge it as is.

One question: shouldn't we set dynamicGhcPrograms = pure False in the quickest flavour?

[alpmestan]

I don't think so, a quick grep shows it's only disabled in the *-cross flavours:

$ git grep -n DYNAMIC_GHC_PROGRAMS | grep "mk/flavours"
mk/flavours/bench-cross-ncg.mk:17:DYNAMIC_GHC_PROGRAMS = NO
mk/flavours/bench-cross.mk:17:DYNAMIC_GHC_PROGRAMS = NO
mk/flavours/perf-cross-ncg.mk:16:DYNAMIC_GHC_PROGRAMS = NO
mk/flavours/perf-cross.mk:16:DYNAMIC_GHC_PROGRAMS = NO
mk/flavours/quick-cross-ncg.mk:17:DYNAMIC_GHC_PROGRAMS = NO
mk/flavours/quick-cross.mk:17:DYNAMIC_GHC_PROGRAMS = NO

Merging as-is sounds good to me.

[snowleopard]

@alpmestan Thank you, merged!