Looking at the make build system, the UseLibFFIForAdjustors variable is only used to toggle cLibFFI in Config.hs files, and also in compiler/ghc.mk to set the USE_LIBFFI_FOR_ADJUSTORS flag when compiling the rts:

@bgamari, last we spoke it was unclear why this would work to limit linking agains ffi as per my previous comment:

This depends on this Haddock PR: https://github.com/haskell/haddock/pull/984

• Included all changes.

Rebase, resolve conflicts, and fix build.

@mpickering Ok, I'll do the rebase now.

Hello, sorry for the delay here. I can't seem to build this patch and am seeing merge conflicts. Can you please rebase on master, and resolve any merge conflicts.

Keep in mind that the dynamic ghc build has been disabled in D5430 due to the failing hadrian build.

The build failure in question is failing to find libffi when building the default flavour on x86_64 with no system wide libffi installed. The command that fails is when linking the ghc binary. The make build system links the ghc binary with -lffi. On that grounds I would say that D5427 will not solve this issue that this patch does.

There seems to be some logic using -l$$(LIBFFI_NAME) in rts/ghc.mk. configure.ac also seems to set the default value of the ffi flag by doing AC_SUBST([CabalHaveLibffi],[False]) or AC_SUBST([CabalHaveLibffi],[True]).

Im confused: setting the ffi flag *enables* linking with ffi. So if I understand correctly, this can only serve to *increase* the cases where we link with ffi. I'm trying to find the relevant logic in the make build system now.

Funny enough, I just came accross this in the make build system rts/ghc.mk:127:

I've tested locally using a nix environment where the Hadrian default build fails on master. With this patch I successfully built and ran tests for flavours: default, perf, quick, and quickest.

After speaking with Alp, we've decided it is safer to simply revert the dynamic linking changes: D5430. This should fix the Hadrian build while a hack free solution can be found.

Rebase

You're absolutely right @bgamari, this situation is not ideal, but I do fully intend to continue work on this. The Hadrian build is broken and the correct solution seems non-trivial, hence I saw this hacky solution as somewhat justified.

This fixes the build on CircleCI: https://gitlab.staging.haskell.org/DavidEichmann/ghc/-/jobs/904. Using Alp's nix environment I was able to recreate the build failure by building the default flavour. In the same environment with this patch build succeeds and I have run the tests in the perf and default flavours. I still see the drop in test failures the we expect from dynamically linking ghc.

There is a similar issue on linux where we link the system ffi instead of the local one. This fails e.g. on CircleCI as ld cannot find the ffi library. I don't know if this is related to the OSX failure, but I'll continue investigating this on linux.

Included too many changes in the last diff.

Converting check_output() result to bool is confusing and in this case is wrong. Comparing to byte string literal instead.

Hello, thanks for fixing that, great work! I see you also added a test which is always appreciated. Before I accept:

• I still think throwing an exception is more valid. I.e. replace loadSrcInterface_maybe with loadSrcInterface (this change would also make the code a bit simpler). If you're not sure why, do ask. If you're convinced otherwise, I'm not gonna fight you on it.
• The changes in .gitmodules should be reverted (as a rule of thumb, I'll only accept if I think it's ready to merge as is).
  • You can easily fix this without breaking your build by, first pushing your changes git push, then re-cloning ghc using git clone git://git.haskell.org/ghc --recurse-submodules (if you need help with this feel free to contact me on IRC at freenode #haskell my handle is DavidEichmann).

This was merged as part of D5312.

Build ghc with -fPIC -dynamic if RTS has a dynamic way.
Don't link executables with -shared -dynload deploy.
These changes are all in hadrian/src/Settings/Builders/Ghc.hs

• Use git status instead of rev-parse

• Fix logic to call Perf.append_perf_stat() only if canGitStatus.
• Improve error message.

@osa1 We use git notes to get the baseline performance numbers and to save the new numbers. Here we only skip performance tests if we both cannot get a baseline (and hence can't tell if tests pass or fail) nor have a way to save the results.

• Replace is_git_repo with can_git_status.

Validate now passes locally for me.

• Fix T4437 by removing DerivingVia from expectedGhcOnlyExtensions. Update comment in GHC.Magic.

• Bump Cabal, bytestring, containers, and text.
• Fix unused matches error.
• Remove unused imprt after rebase.
• Fix GHC.Magic validation errors.

• Correct assignment of hasMetricsFile.

There seems to be some errors when I validate locally. I'm working on this now.

• Fix unused matches error.

• Bump Cabal, bytestring, containers, and text.

Hi @alanasp, Have you managed to have a look at the issue since we last spoke? It would be great to see this to completion. I appreciate that a significant amount of effort has already gone into this patch. If you're short on time/resources, please let me know and I will investigate this further.

We'll likely need to bump Cabal again soon, but this at least gets ghc to build with Cabal 2.5.0.0.

Fix comment.

More commit message fiddling (I'm not sure how arcanist handles this).

Update commit message explaining why ieLWrappedName change was reverted.

In D4953#145970, @alanasp wrote:

@DavidEichmann The case that you pointed out indeed works as intended. I've made sure that if there is a non-deprecating way a symbol can be brought into context then that way will be used.

Looks good to me.

@DavidEichmann Ok, it's actually self-export rather than self import. The example is here /libraries/base/GHC/Num.hs

@alanasp Ah, thank you! That makes a lot more sense now.

@alanasp I wasn't aware that ghc supported self imports. The concept sounds a bit absurd to me (why would a module ever import itself?). I know GHC 8.4.3 would complain about circular imports, which is what I expect. I would expect a similar error to be raised by the code in this patch, but perhaps I'm missing something. Can you give a concrete example? How did you run into this use case?

Minor cleanup.

Regarding loadSrcInterface vs loadSrcInterface_maybe , it is hard to see what the correct solution is here. What exactly do you mean by "self import"? something like module A where; import module A?

Added CircleCI push git notes script. Removed temporary changes.

Please note there are still temporary changes that need to be removed from this patch.

• Use average if previous commit has multiple recordings of the same metric (in testlib._collect_stats).
• Remove TODO comments.
• Update README.md
  • Remove 'Important Types' section. This is now self documenting.

Squash commits

• Output suggested text for the git comment message that will allow all the detected changes in performance tests.
  • • removed optimisation to short circuit test if any metric fails (needed to give a complete suggestion)
    • added allow_changes_string()
    • check_stats_change takes now returns the change direction string.
    • use of TestRun.metrics changed (see comment above "self.metrics = []")
  • Use namedtuple PerfStat
    • check_stats_change takes PerfStat instead of individual arguments

• Use a namedtuple for performance stat objects.
• Print a suggest git commit msg to make all performance tests pass.