Add a script for running a ghci that can load and run ghc
ClosedPublic

Authored by mgsloan on Jun 28 2018, 6:18 PM.

Details

Summary

Add scripts and .ghci files for loading GHC into GHCi

Major credit to Csongor Kiss who wrote nearly all of settings.ghci

Some small modifications to GHC are needed to make this work, and this diff depends on D4986 being merged.

Test Plan

Manual for now. I have some thoughts on how to run the entire testsuite against
GHC-in-GHCi.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mgsloan created this revision.Jun 28 2018, 6:18 PM
mgsloan edited the summary of this revision. (Show Details)Jun 28 2018, 6:20 PM
mgsloan updated this revision to Diff 17118.Jun 28 2018, 6:43 PM
  • Better name for the top_dir workaround + comment
  • Use "ghci-load" and "ghci-start" targets instead of "load-ghci" and "run-ghci"
mgsloan added a comment.EditedJun 28 2018, 6:51 PM

Note that there is some prior conversation about this change on github - https://github.com/ghc/ghc/pull/154 - but this differential supersedes that PR.

I'm particularly interested if anyone can think of a cleaner way to handle the issue where running :main gets the wrong top_dir since the -B present in inplace/bin/ghc-stage2 is not specified. One idea that's just occurred to me is to use :def to define a macro which runs main and provides the correct -B.

Feel free to suggest alternative choices about naming and file locations. Also, perhaps this should be put somewhere other than the Makefile, though to me it seems like a decent spot to put a top level development command.

mgsloan retitled this revision from Add "make run-ghci", which runs a ghci that can load compiler + ghci to Add "make ghci-start", which runs a ghci that can load compiler + ghci.Jun 28 2018, 6:57 PM

Very nice!

Note that there is some prior conversation about this change on github - https://github.com/ghc/ghc/pull/154 - but this differential supersedes that PR.

I'm particularly interested if anyone can think of a cleaner way to handle the issue where running :main gets the wrong top_dir since the -B present in inplace/bin/ghc-stage2 is not specified. One idea that's just occurred to me is to use :def to define a macro which runs main and provides the correct -B.

A simple (but perhaps not cleaner) solution would be to accept a _GHC_TOP_DIR environment variable. We would want -B to take precedence over the environment variable.

Feel free to suggest alternative choices about naming and file locations. Also, perhaps this should be put somewhere other than the Makefile, though to me it seems like a decent spot to put a top level development command.

mgsloan updated this revision to Diff 17152.Jul 2 2018, 5:01 PM
  • Have "make ghci-load" default args so that "main" runs an inner ghci
  • Add documentation in the Makefile and switch to make ARGS parameter

@bgamari Thanks for reviewing! I've updated it with some changes that I've found convenient. It now sets "args" such that running "main" will default to running a nested GHCi with a script that changes the prompt to indicate that it is the inner GHCi. Otherwise I found it easy to forget if you're in the inner or outer GHC.

Having a GHC_TOP_DIR environment variable makes sense. Would it be useful otherwise? Was the leading underscore intentional?

I think that approach is probably cleaner. It's probably fine in that case to also not emit any messages indicating that the env var is being used.

Having a GHC_TOP_DIR environment variable makes sense. Would it be useful otherwise? Was the leading underscore intentional?

I think that approach is probably cleaner. It's probably fine in that case to also not emit any messages indicating that the env var is being used.

The leading underscore was indeed intentional; I think we want to strongly discourage this variable from being used outside of development. There are few good reasons to change the top directory without rerunning the installation procedure and I don't think we want to support the ability to do so as a user-facing feature.

Without looking at the details, this looks great. When it's merged, could you send a followup note to ghc-devs, please? I'm keen to try this out!

Thanks!

Thanks @goldfire ! Yeah, I'm pretty stoked about it. As mentioned elsewhere, this makes development far more enjoyable. I'm often getting something like a 10 or 15 second iteration time between saving a file, reloading the compiler, and getting a nested GHCi with which to try out the new behavior.

@bgamari I am eager to get this merged! I think it can really help out ghc devs and newcomers. Also it is not very convenient that all of my dev branches are based on this branch.

Please let me know if anything should be modified here. I'm not really all that happy with the Makefile approach. I noticed there is some precedent for top level .sh files (build.nix.sh, install-sh, validate, configure) and python scripts (boot). Would a top level ghci script that starts with #!/bin/sh be preferred?

mgsloan updated this revision to Diff 17231.Jul 7 2018, 10:42 PM
  • _GHC_TOP_DIR to specify top dir + use scripts instead of makefile

    Using scripts instead of Makefile targets makes it more straightforward to set the env var. This also makes it possible to supply ghci args directly.

    The scripts also use exec, so that the program immediately under the user's shell is ghc-stage2. This looks nicer than make when using things like tmux which display the currently running process name.
mgsloan updated this revision to Diff 17232.Jul 7 2018, 10:49 PM
  • Add a comment in findTopDir about _GHC_TOP_DIR env var

@bgamari Please take a look. I've switched it over to using _GHC_TOP_DIR + top level shell scripts instead of Makefile targets. This seems better than how it was.

I've left in the change which makes just one call to findTopDir instead of 3, because I think that's an improvement even if it is an unrelated change. Please let me know if that should be moved to a separate diff.

mgsloan updated this revision to Diff 17233.Jul 7 2018, 10:52 PM
  • Remove the last remnants of Makefile changes

I think the point of the Makefile approach would be to generate the necessary settings.ghci file automatically from other dependency information. This will probably be easier once hadrian is merged.

Would we perhaps want the ghci scripts to live somewhere else than at the top level (say a scripts/ directory) ?

(I'm not saying I do, but it might make sense to try and keep things somewhat organized and to not make too big of a mess of the top level directory of the ghc source tree.)

I think the point of the Makefile approach would be to generate the necessary settings.ghci file automatically from other dependency information. This will probably be easier once hadrian is merged.

Ah, I hadn't considered that! The reason I initially chose the Makefile approach was to avoid cluttering the top directory, without needing to run a script from a sub-directory. If I understand correctly, this shouldn't be very difficult. I'm looking into adding this to hadrian.

Would we perhaps want the ghci scripts to live somewhere else than at the top level (say a scripts/ directory) ?

(I'm not saying I do, but it might make sense to try and keep things somewhat organized and to not make too big of a mess of the top level directory of the ghc source tree.)

Indeed, I also didn't want to add top level scripts, but there doesn't seem to be much precedent. I will be content to bury them in utils/ghc-in-ghci/ if the following also happens:

  • hadrian repl gets added (working on it!)
  • Addition of a .ghcid as described here

I have decided that I can do without the "ghci-start" variant, because you can just ctrl-c the load.

I'm a little surprised this hasn't gotten merged yet, but that's ok, partially my own fault I suppose, for getting real preoccuped and busy with other things. To make it less of a pain to use this change along with other GHC changes (before it's merged), I am going to split off a diff the modifications to GHC. Then this diff will just be the added files for GHCi support.

I have split off D4986 and D4987 from this diff so now all this does is add files. Only the former is a dependency of this change. I also removed the ghci-start script, since it wasn't really necessary, and renamed ghci-load to utils/ghc-in-ghci/run.sh.

I am going to experiment with adding a repl target / command to hadrian, based on this change.

Please let me know if I need to address anything in particular to get this merged. Thanks!

mgsloan updated this revision to Diff 17390.Jul 19 2018, 1:16 AM

Move top level scripts into utils subdir + documentation improvements + remove ghci-start

mgsloan edited the summary of this revision. (Show Details)Jul 19 2018, 1:17 AM
mgsloan edited the summary of this revision. (Show Details)
mgsloan edited the summary of this revision. (Show Details)
mgsloan retitled this revision from Add "make ghci-start", which runs a ghci that can load compiler + ghci to Add a script for running a ghci that can load and run ghc.
mgsloan updated this revision to Diff 17396.Jul 19 2018, 6:50 AM

Fix a --restart path typo

@bgamari @alpmestan Please let me know if there is anything I can do to help get this merged!

alpmestan accepted this revision.Jul 24 2018, 11:01 AM

This looks good to me. I'd love to have something that makes sure this is not broken (a test that just runs your script?), but I don't think we necessarily want to hold this patch on that.

This revision is now accepted and ready to land.Jul 24 2018, 11:01 AM

I can add such a test. Where should a test like that go?

testsuite/tests/ghci/ would be a good place I suppose.

(Let me link to the various components of one such existing test:

In your case, you will quite likely not need such a fancy set up, but it could be useful to know for future patches. Feel free to look around the all.T file to see the possibilities for writing a test that loads GHC itself in ghci. This trac page might be helpful for learning a little about the testsuite, in case you're not familiar with it already.)

Awesome, thanks a bunch for the guidance! I will work on this soon,
hopefully sometime tomorrow

mgsloan updated this revision to Diff 17455.Jul 26 2018, 5:28 AM
  • Remove -fobject-code as it is not always wanted
This revision was automatically updated to reflect the committed changes.
mgsloan added a comment.EditedJul 27 2018, 12:41 PM

Awesome, thanks for merging @bgamari ! Note that for this script to work properly, https://phabricator.haskell.org/D4986 needs to be merged as well. In the meantime one can remove -ighc and :load Main, and it should work at least for loading the compiler code, but you won't be able to run GHC inside of GHCi without the changes in 4986.