- User Since
- Aug 1 2018, 9:55 AM (19 w, 6 d)
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.
Tue, Dec 11
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:
Mon, Dec 10
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.
Sun, Dec 9
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.
Fri, Dec 7
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.
Tue, Dec 4
Sun, Dec 2
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.
Fri, Nov 30
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.
Thu, Nov 29
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).
Cabal was already bumped in D5385.
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
Wed, Nov 28
Tue, Nov 27
- Use git status instead of rev-parse
- Fix logic to call Perf.append_perf_stat() only if canGitStatus.
- Improve error message.
Mon, Nov 26
@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.
Fri, Nov 23
Thu, Nov 22
Validate now passes locally for me.
- Fix T4437 by removing DerivingVia from expectedGhcOnlyExtensions. Update comment in GHC.Magic.
Wed, Nov 21
- Bump Cabal, bytestring, containers, and text.
- Fix unused matches error.
- Remove unused imprt after rebase.
- Fix GHC.Magic validation errors.
- Correct assignment of hasMetricsFile.
Tue, Nov 20
There seems to be some errors when I validate locally. I'm working on this now.
Mon, Nov 19
- Fix unused matches error.
- Bump Cabal, bytestring, containers, and text.
Nov 13 2018
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.
Nov 12 2018
We'll likely need to bump Cabal again soon, but this at least gets ghc to build with Cabal 126.96.36.199.
Nov 8 2018
More commit message fiddling (I'm not sure how arcanist handles this).
Update commit message explaining why ieLWrappedName change was reverted.
Nov 5 2018
Oct 31 2018
Looks good to me.
Oct 30 2018
@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?
Oct 29 2018
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?
Oct 24 2018
Added CircleCI push git notes script. Removed temporary changes.
Oct 17 2018
Please note there are still temporary changes that need to be removed from this patch.
Sep 25 2018
Sep 24 2018
Sep 21 2018
Aug 30 2018
Aug 27 2018
- 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.
Aug 24 2018
- 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.
Aug 23 2018
Parse the git commit message for allowed changes in test metrics.
Aug 14 2018
Try catch 'git notes append'.
I've incorporated the remaining changes from D3758. Some key differences
testsuite: Save performance metrics in git notes.
Aug 13 2018
- Added more test cases.
Correct limb length assertion for gcdExtInteger (fixes Trac #15350)
Aug 10 2018
Note that this is mostly copied from D3758.