Bugfix: don't mangle GHC options that begin with "-i" (e.g. "-instantiated-with")

[acerempel]

The 'fixImportDirs' function inadvertently catches GHC options that begin with "-i", such as "-instantiated-with" and "-ignore-package", and changes them to e.g. "-i/Users/foo/stantiated-with". This patch fixes that.

This bit me when I was trying to use ghcide with a Cabal package that has two module signatures: BaseMonad.hsig and List.hsig. I would get this error:

target ‘BaseMonad=<BaseMonad>,List=<List>’ is not a module name or a source file

This error occurred because Cabal was trying to pass instantiated-with BaseMonad=<BaseMonad>,List=<List> to GHC, but fixImportDirs had turned that into -i/Users/parsonyorick/…/nstantiated-with BaseMonad=<BaseMonad>,List=<List>, which GHC parsed as two commandline arguments.

[fendor]

Thank you for catching this!
Is there a reason why you are matching by prefix instead of elem?

[acerempel]

I use isPrefixOf in order to also catch options of the form -instantiated-with=… (in addition to -instantiated-with followed by an argument). Does that make sense?

That deserves a comment, so I'll add one.

[fendor]

Do you think there is a more general solution? This requires that we update hie-bios for every ghc version that adds a new flag beginning with -i.

[acerempel]

Sure, I suppose so! I guess I imagined that ghc's flags don't change that often.

One way to solve this more generally would be to do this fixing of import paths after GHC parses its command line arguments—i.e. modifying the DynFlags—rather than operating on the raw list of strings.

Another way would be to check, for each argument that looks like -idir, whether dir is actually a directory, and modify the argument only in that case. (This works as long as no one has an hs-source-dirs: nstantiated-with or similar, which would be very strange!)

Upon further thought, I like the first option better. GHC already knows how to parse its own command line arguments – no need for us to try to do it ourselves. It looks like we modify the DynFlags in initSession anyway, so moving the modification of the import dirs over there shouldn't be hard.

If that sounds good to you, I'm happy to implement it.

[fendor]

The problem with the first approach is that previously, it was possible to pass around the raw compilation flags and they could be easily used. We would lose this property if the paths to -i are relative.
Moreover, I fear that think that we can not easily know the root of the project, in initSession. I would have to verify, but I think that the flags are relative to the root of a cradle and cradles can be nested, thus getCurrentDirectory would be wrong, it would require that we pass in the Cradle to initSession.

I thought about the second approach as well, but I feel it is a bit too brittle. Unless the tools guarantee that the paths exist relative to the working directory.

@mpickering Do you have a preference or idea how to properly handle this?

[acerempel]

Ah, I see. Relating to your second point, when I was playing around with it the other day, I thought of adding a field to ComponentOptions to pass the directory to initSession. But that’s immaterial if it’s desirable to be able to pass around the raw compilation flags.

…

Am 02.03.2020 um 07:01 schrieb fendor ***@***.***>: The problem with the first approach is that previously, it was possible to pass around the raw compilation flags and they could be easily used. Moreover, I fear that think that we can not easily know the root of the project, in initSession. I would have to verify, but I think that the flags are relative to the root of a cradle and cradles can be nested, thus getCurrentDirectory would be wrong, it would require that we pass in the Cradle to initSession. I thought about the second approach as well, but I feel it is a bit too brittle. Unless the tools guarantee that the paths exist relative to the working directory. @mpickering Do you have a preference or idea how to properly handle this? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[mpickering]

The way to fix this probably is to allow each Cradle to specify the directory the flags are relative to and then make the source directories can be made relative to this after the flags have been parsed. At least the cabal cradle also has issues with Template Haskell splices using relative paths which this approach would fix.

[fendor]

@parsonyorick Sorry for not returning earlier to this issue. I have a prototype of the suggested implementation at #166 which ought to solve the problem by normalising the filepaths after GHC flag parsing

[acerempel]

Great, the approach at #166 makes complete sense to me—that will do the trick.

[fendor]

Closed in favour of #166