Extend ghc environment file features
ClosedPublic

Authored by duncan on Dec 19 2015, 7:47 PM.

Details

Summary

A set of changes to enable local ghc env files to be useful for tools
like cabal. Ultimately it will allow cabal to maintain a ghc env file so
that users can simple run ghc or ghci in a project directory and get the
expected environment of the project.

Change the name of .ghc.environment files to include the platform and
ghc version, e.g. .ghc.environment.x86_64-linux-7.6.3, since their
content is version specific. Strictly speaking this is not backwards
compatible, but we think this feature is not widely used yet.

"Look up" for a local env file, like the behaviour of git/darcs etc. So
you can be anywhere within a project and get the expected environment.

Don't look for local env files when -hide-all-packages is given.

Extend the syntax of env files to allow specifying package dbs too.

Test Plan

Currently completely untested. Compiles, that is all.
Sorry, have to disappear for the hols.

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.
duncan updated this revision to Diff 5862.Dec 19 2015, 7:47 PM
duncan retitled this revision from to Extend ghc environment file features.
duncan updated this object.
duncan edited the test plan for this revision. (Show Details)
duncan added reviewers: ezyang, bgamari, austin.
ezyang edited edge metadata.Dec 19 2015, 9:58 PM

Channeling SPJ, if we're going to allow these flags, why can't you just put whatever flags you want in an environment file?

compiler/main/DynFlags.hs
3864

extended

3964

It'll be tricky to do this cross-platform compatibly, but this probing needs to stop at a mount point (that's how Git works too.) Probably worth making a proper helper function to do.

ezyang requested changes to this revision.Dec 19 2015, 10:00 PM
ezyang edited edge metadata.
This revision now requires changes to proceed.Dec 19 2015, 10:00 PM
hvr added a reviewer: hvr.Dec 20 2015, 4:08 AM
hvr updated the Trac tickets for this revision.Dec 20 2015, 4:12 AM
hvr edited edge metadata.Dec 20 2015, 4:15 AM

@ezyang I'm afraid we'll have to finish up this patch ourselves for RC1 (in order to get as much field-testing as possible in case this breaks something, adding this in RC2 would be rather late)

hvr added inline comments.Dec 20 2015, 4:25 AM
compiler/main/DynFlags.hs
3964

This sounds like something we'd want to add to directory maybe? It's a common functionality ppl tend to reinvent...

bgamari edited edge metadata.Dec 20 2015, 4:55 AM

Let's make sure this gets properly documented in the release notes and users guide.

OK. Do we have someone who will see this patch through to RC1?

bgamari commandeered this revision.Dec 22 2015, 4:26 AM
bgamari edited reviewers, added: duncan; removed: bgamari.

I can pick it up.

duncan edited edge metadata.Jan 3 2016, 6:25 PM

I'm preparing a couple changes:

  • making package-db directives in the env files be relative to the location of the env file (when the db is given as a relative path).
  • update user guide entry on package environments
  • spelling error noted by @ezyang
  • fixing lint complaints
compiler/main/DynFlags.hs
3964

I agree. I didn't try to make it do that here since it depends on functionality we don't yet have. If it becomes available I agree we should use it here.

duncan commandeered this revision.Jan 4 2016, 2:38 PM
duncan edited reviewers, added: bgamari; removed: duncan.

Have to commandeer to add more patches...

duncan updated this revision to Diff 6067.Jan 4 2016, 2:40 PM
duncan edited edge metadata.
  • Make paths in package env files be relative to the env file location
  • Update the user guide section on package environments
  • Fix long line lint grumble
duncan added a comment.Jan 4 2016, 2:48 PM

@ezyang so I've not addressed your two main complaints:

  1. "Channeling SPJ, if we're going to allow these flags, why can't you just put whatever flags you want in an environment file?"
  2. "this probing needs to stop at a mount point (that's how Git works too.) Probably worth making a proper helper function to do."

For 1, I don't see that as the purpose here. We want to make a specific package environment. There's a few things we need for that, control of which package dbs and which specific package ids. So apart from "we don't need it to be that general", a specific disadvantage of only having something that general is that then users will want to use it manually which makes it hard for tools to use it automatically since the tools will stomp on the changes made by users. If there is demand for a general "automatically use these flags" file, then I think it ought to be a different file.

For 2, I agree this should be done. If/when we get such functionality then we should use it. Doing the work to make that available is rather time consuming however. The current logic is ok I think. The mount point would sometimes let us bail out earlier so it's a useful optimisation, but it is only an optimisation.

ezyang added a comment.Jan 4 2016, 5:04 PM

Re (1), I will just say my piece here, and if you're still unconvinced, go ahead. :) I am wondering about this because if this is a completely separate codepath, it will need to be updated in parallel whenever "relevant" flags are added to GHC, that people might want to have in the package environment. For example, we very recently got a new -plugin-package flag. Should these be supported by the package environment? Probably! So yes, I agree in the design principle of not prematurely overgeneralizing, but it seems like the generalized version will be easier to maintain. If we really want to avoid users from putting arbitrary things in the package environments and gunking up tooling, then it should be by explicitly subsetting what flags are acceptable, similar to how OPTIONS_GHC is implemented. (I don't find the "we don't want to make tooling's job harder", because package environment files are a shared resource and cabal-install (or is this logic living in Cabal?) doesn't have any particular claim over the ability to write package files. So if you want to support cabal-install and Stack they have to agree how to play nice with one another.

Re (2), it's not just a performance optimization. The relevant situation that induced the Git folks to stop at mount points is automounter badness. The relevant situation goes something like: ghc is run in a directory /smb/ezyang/code (where /smb/ezyang/ is a mountpoint), and it probes /smb/ezyang/code/.ghc-environment, then /smb/ezyang/.ghc-environment. Then, if it doesn't stop at a mountpoint, it will probe /smb/.ghc-environment, which will cause the automounter to attempt to mount a remote file system .ghc-environment. There are many automounters which are poorly implemented and will hang for a nontrivial amount of time at this point, before deciding that the mount will not work. Granted, Git existed for a while with this bug, so it is an unusual configuration, but it is one that exists.

ezyang accepted this revision.Jan 4 2016, 6:41 PM
ezyang edited edge metadata.

BTW, just to clarify my position (based on a conversation with @hvr), I'm not saying that whenever cabal-install adds a new feature, it needs to make sure it works with Stack; however, I do think we should try not to make design decisions that will make life hard for Stack, and this includes taking sole ownership of environment files when regular users and Stack may also very reasonably want to use this functionality.

In any case, I'm not going to argue against this patch going in as is, but it would be good to get a meeting of minds on the issue.

This revision is now accepted and ready to land.Jan 4 2016, 6:41 PM
duncan added a comment.Jan 5 2016, 6:57 AM

I don't think there's any problem here for stack. stack can use this feature or not as it likes. I don't think there's any conflict.

Re automounters, in that setup isn't it typically the user home dirs? We stop at the home dir already. So I suspect we can live with it for a bit 'til we get mount point functionality available.

As for maintenance. I'm quite open to the idea to reuse the main args parser and just use a whitelist of flags. I can't currently see how to handle flags that take file args, since we must interpret those file locations as relative to the env file, not relative to ghc's cwd.

My preference would be to get this feature into ghc rc1 and try and improve the maintainability thereafter if we can find ways to do that. It's not clear to me just yet how to do that and I'm worried about missing the whole release cycle.

duncan updated this revision to Diff 6073.Jan 5 2016, 6:58 AM
duncan edited edge metadata.
  • Ignore blank lines in package env files
bgamari accepted this revision.Jan 5 2016, 3:12 PM
bgamari edited edge metadata.

Thanks @duncan!

This revision was automatically updated to reflect the committed changes.

Regardless of whether we accept any flag in the environment file or a subset, I think we need a few more.

In particular I would like to see support for -i and -X since those are commonly specified in a .cabal file (via hs-source-dirs and default-extensions). More generally, I think we should support any flags that translate from a field in a .cabal file (I'm not sure if this should include ghc-options).

I've long wanted to be able to run

$ cabal configure

once and go about with my work, having ghci, hdevtools, liquidhaskell -- anything that uses the GHC API -- just work. But I don't think we're quite there with just package-related flags.

I definitely support @gridaphobe's comment.

having ghci, hdevtools, liquidhaskell -- anything that uses the GHC API -- just work. But I don't think we're quite there with just package-related flags.

Unfortunately even adding that would not be enough. There's a whole lot of other things that are needed to be able to run ghc to get the exact environment for a particular component of a particular package. And given that it is different for different components in the same package, it's hard to do this sanely with one file. And the other biggie is pre-processors etc. I appreciate the tooling problem but I think the right interface point is slightly different.

What this package environment feature gives us is enough to correspond to a local cabal/stack sandbox, so at least we can run ghc/ghci in the sandbox (or moral equiv) and have it just work.