Change 'stack ghc' to provide package context

[afcady]

This patch is necessary for me to use flycheck (which calls stack ghc internally) on a project that uses cryptonite with a package database that contains cryptohash.

Without it, I get flycheck errors like this:

    Ambiguous module name ‘Crypto.Hash’:
      it was found in multiple packages:
      cryptonite-0.10@crypt_9z0j8QI27Av2VIWw0mEkTO cryptohash-0.11.6@crypt_3Cwvwq9ssRY1dmVI1qI6C2

Related: flycheck/flycheck-haskell#52

[mgsloan]

Thanks for working on this. One issue is that since it uses the default build options, info from all the executable and library components will be used (and not from test / benchmarks). Sometimes the info from two packages don't work well together, for example, if one imports monads-tf and the other uses mtl, then we'll get a lot of ambiguous module name errors. So, the user needs to be able to specify which packages to use.

I actually don't think this is necessary, it should already be possible with stack ghci --with-ghc flycheck --ghci-options my-extra-opt-1 --ghci-options my-extra-opt-2, if flycheck accepts all the same args as ghc.

[mgsloan]

Hrm, actually defaultBuildOptions has boptsTargets = []. I'm rather confused as to why this would work, as if there are no targets then ghciSetup won't do much cleverness, and we won't get any -package-id flags.

[afcady]

Oh, I see. Didn't read that yet when I replied. I guess it would fail on certain projects.

As far as --with-ghc, I didn't know you could do that -- but flycheck isn't a command line app, it's elisp inside emacs which launches an external process via stack ghc on the file that you're editing in emacs (or, actually, a copy). Besides the filename, it just adds ghc options to redirect output.

So I think what has to happen to fix what you're talking about is to go from a x.hs filename (the info available to flycheck) to a cabal target.

---

I'm rather confused as to why this would work, as if there are no targets then ghciSetup won't do much cleverness, and we won't get any -package-id flags.

Well, it produces the same package-id flags that would be produced with a simple stack ghci with no other args. (Which is not an empty set, as I verified with ps.) Actually stack ghci --with-ghc echo gets you those flags too :)

[afcady]

I did a git pull --rebase and git push -f (editing git history) and I guess that caused github to hide a conversation which had previously been visible here:

afcady@7897a9d#commitcomment-15877233

[mgsloan]

So is stack ghci --with-ghc echo sufficient? I'm trying to avoid adding stuff when unnecessary. One issue is that I don't like the ghcCmd name. No other command is camel case. We already have stack ghc as an alias for stack exec ghc.

How about adding something like this as a subcommand of query - stack query ghc-args? I'd want it to take all the arguments that stack ghci takes, but with the following conceptual defaults: --no-load --no-build --no-interpreter. I say conceptual, because probably the code paths would be different beyond ghciSetup.

Sorry to push back on code you've worked on, but we want things to fit together nicely into a cohesive whole.

[afcady]

So is stack ghci --with-ghc echo sufficient?

No it isn't. For two reasons:

1. The issue you raised, that the default cabal target is not going to work if different targets have conflicting packages. So ultimately the package arguments are not even correct.

2. ghci isn't ghc, so the arguments need to be edited: --interactive needs to be stripped off, -threaded ideally needs to be _re-_added (if it was removed), -optP/tmp/ghci29597/cabal_macros.hdoesn't look like it should be there, etc.

   Of course, this could be done with some kind of external program instead of echo, that gets the -package args out of the command line and then constructs a new command line (which must be kept in sync with the version of stack that is calling it) and then calls it. Ultimately such a program would have to solve problem #(1) too. Since it does need to be written, I'd say the logical place for it is therefore within the stack program itself. Right?

---

One issue is that I don't like the ghcCmd name.

There's a miscommunication going on here -- you're looking at the wrong patch! Look at a diff between my master and the master of commercialhaskell -- there is no such name anymore. I removed it before even submitting the pull request, although I didn't amend my git history. (I guess that was a mistake, I should have submitted a single commit.)

Here is the pull request with all four commits combined:

https://github.com/commercialhaskell/stack/pull/1737/files

What I did in the final version of the pull request is modify the default behavior for stack ghc and the reason I did it that way is that, on my computer where I'm writing code, this fixes flycheck error checking, thus restoring my workflow to what it was (very pleasant!) before I added a dependency on cryptonite -- without me having to modify/reconfigure flycheck or otherwise write any elisp or anything like that.

But it does seem to me that modifying the default behavior of stack ghc is a good idea here, even beyond fixing flycheck my own box. It seems to me that stack ghc's behavior is simply not correct, unless it receives the package-id args (and the correct ones).

Sorry to push back on code you've worked on, but we want things to fit together nicely into a cohesive whole.

That's OK.

[mgsloan]

Ah, I see! More food for thought on this: #1208 #1388

-optP/tmp/ghci29597/cabal_macros.hdoesn't look like it should be there, etc.

You actually probably do want that, if you want cabal CPP macros to work.

I removed it before even submitting the pull request, although I didn't amend my git history. (I guess that was a mistake, I should have submitted a single commit.)

Yeah, it's better to leave out the previous attempt, to avoid confusion.

But it does seem to me that modifying the default behavior of stack ghc is a good idea here, even beyond fixing flycheck my own box. It seems to me that stack ghc's behavior is simply not correct, unless it receives the package-id args (and the correct ones).

stack ghc simply runs ghc in stack's environment. These args come from knowing which package we're trying to build, which requires targets.

The issue you raised, that the default cabal target is not going to work if different targets have conflicting packages. So ultimately the package arguments are not even correct.

Yeah, that's why we need to know which targets the user cares about. This requires specifying or inferring which packages / components to use as targets. #1361 is relevant, we should be able to determine the target the user cares about, based on which file they are compiling.

[afcady]

-optP/tmp/ghci29597/cabal_macros.h

You actually probably do want that, if you want cabal CPP macros to work.

Ha! So it should. Turns out that's another bug in flycheck+stack! The cabal CPP macros indeed do not work in flycheck.

(Of course, just adding the argument isn't enough to fix it, because the file also has to be generated (and, ideally, removed) -- another problem for a stack ghci --with-ghc=echo kind of approach.)

#1361

Seems about right. One kink though is that flycheck does not run stack ghc on the actual file, but instead saves from the editor buffer into a temporary file and runs stack ghc on that.

Probably it's too much for stack to guess the original filename -- flycheck should supply more info (although this breaks compatibility with existing flycheck installs). But the functionality of mapping filename -> cabal target still seems to fall into the purview of stack.

Seems to me like a possible plan would be something like this:

1. Add a flag --no-interactive to stack ghci which disables adding --interactive and also disables anything else that is done solely for ghci (like removing -threaded -- and maybe that's it?)

2. Add a parameter --filename (or whatever) to stack ghci that derives the target from a filename (but does not add the filename to the ghc command line).

   (Alternatively: add another syntax to the target parser. Maybe source-file: to go along with exe:, test:, and bench:.)

3. Modify flycheck to call stack ghci --no-interactive --filename <original filename in project> <temp file> instead of stack ghc <temp file> (iff stack version is high enough)

(That creates a weirdness where stack ghci --no-interactive != stack ghc but the latter could be made an alias of the former if desired.)

---

Thoughts?

[afcady]

So I got cabal_macros.h included, fixing cabal CPP in flycheck, but I had to change some things around to do so. Specifically, that ghci29597 in optP/tmp/ghci29597/cabal_macros.h changes on every compile, forcing a recompilation of everything. I changed the filename to exist under .stack-work/ and to use the hash of the file content as the basename.

https://github.com/afcady/stack/compare/ghci-no-interactive~2...afcady:ghci-no-interactive?diff=unified&name=ghci-no-interactive

[mgsloan]

Hey, sorry for letting this languish! Things got busy. I hope you're still keen on doing some work on this?

But the functionality of mapping filename -> cabal target still seems to fall into the purview of stack.

True! How about "stack query targets-for FILE" and "stack query default-target-for FILE`. It doesn't really fit into query's current tree-of-data schema, but it's experimental, so we can change how it works (ideally maintaining compatibility with the existing queries). As already mentioned, the implementation can be shared with #1361

Add a parameter --filename (or whatever) to stack ghci that derives the target from a filename (but does not add the filename to the ghc command line).

(Alternatively: add another syntax to the target parser. Maybe source-file: to go along with exe:, test:, and bench:.)

The targets parser is already pretty overloaded. However, none of our existing target syntax allows ".hs" or ".lhs" (well except dirs, and in that case it's unambiguous what the user means), so I think it'd be fine to just take haskell files. It'll be quite great to be able to do stack ghci Foo.hs and load up just Foo and its dependencies, with all the right options :D

(That creates a weirdness where stack ghci --no-interactive != stack ghc but the latter could be made an alias of the former if desired.)

I think that's fine. Retrospectively, using repl, like cabal, may have been a better choice. Then, stack ghci could just be an alias of stack exec ghci (which is a rather useful thing independently of stack ghci!).

Modify flycheck to call stack ghci --no-interactive --filename <original filename in project> <temp file> instead of stack ghc <temp file> (iff stack version is high enough)

Seems reasonable! You could also just demand that the user updates their stack version since this will work quite a bit better.

[mgsloan]

Going to go ahead and close this, thanks for the discussion! I opened #2053 to track further work here