Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

GHC 8.8 support #1482

Merged
merged 23 commits into from
Jan 19, 2020
Merged

GHC 8.8 support #1482

merged 23 commits into from
Jan 19, 2020

Conversation

Avi-D-coder
Copy link
Collaborator

I'm not particularly happy with per file pragmas, demoting unused imports to a globally warning is cleaner.

Once apply-refact is updated we can remove the submodule. Preferably before merging.

Copy link
Collaborator

@alanz alanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a stack-8.8.1.yaml file

.gitmodules Outdated Show resolved Hide resolved
app/MainHie.hs Outdated Show resolved Hide resolved
hie-plugin-api/Haskell/Ide/Engine/Ghc.hs Outdated Show resolved Hide resolved
install/shake.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@lukel97 lukel97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than those options LGTM

app/MainHie.hs Outdated Show resolved Hide resolved
hie-plugin-api/Haskell/Ide/Engine/Ghc.hs Outdated Show resolved Hide resolved
hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs Outdated Show resolved Hide resolved
src/Haskell/Ide/Engine/LSP/Completions.hs Outdated Show resolved Hide resolved
src/Haskell/Ide/Engine/Plugin/ApplyRefact.hs Outdated Show resolved Hide resolved
src/Haskell/Ide/Engine/Plugin/Package.hs Outdated Show resolved Hide resolved
src/Haskell/Ide/Engine/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
src/Haskell/Ide/Engine/Scheduler.hs Outdated Show resolved Hide resolved
src/Haskell/Ide/Engine/Support/HieExtras.hs Outdated Show resolved Hide resolved
src/Haskell/Ide/Engine/Transport/LspStdio.hs Outdated Show resolved Hide resolved
@fendor
Copy link
Collaborator

fendor commented Dec 20, 2019

I suggested to use -fno-warn-unused-imports instead of disabling the warning globally.
The intention was to not litter multiple source files with extensive CPP instructions.

@alanz
Copy link
Collaborator

alanz commented Dec 20, 2019

@fendor much as I hate CPP, I would rather be clear about what we are actually using.

@Avi-D-coder
Copy link
Collaborator Author

@alanz the advantage of the original approach (-Werror -Wwarn=unused-imports in the cabal file) is that it always warns you when your using a new GHC, but does not require CPP for redundant imports since it does not Error out.

Currently every new GHC/base produces a lot of churn adding new CPP all over the place. If we warn instead, in most cases we will only have to remove imports when they show up as warning on the oldest GHC.

@Avi-D-coder
Copy link
Collaborator Author

Strange error without a hie.yaml cradle: {stack: {}}, hie-8.8.1 produces the following on the stack template project.

ghcDispatcher:Got error for a request: IdeError {ideCode = OtherError, ideMessage = "\"cannot satisfy -package-id base-4.13.0.0\\n    (use -v for more information)\"", ideInfo = Null} with mid: Nothing

@mpickering
Copy link
Collaborator

Any GHC related CPP should go into the GhcCompat module? so it's easy to remove when old versions stop being supported.

@Avi-D-coder
Copy link
Collaborator Author

@mpickering While uses of CPP conditional imports could conceivable be refactored to GhcCompat, that does not fix the churn issue. Every new GHC release or change in a library's exports between stack snapshots can break Hie's build until someone adds the CPP somewhere. The advantage of demoting unused imports to a warning rather than a hard error is a reduction in churn.

This the last argument I'll make against unneeded CPP, unless told otherwise I'll rework the PR to use CPP tomorrow.

@alanz
Copy link
Collaborator

alanz commented Dec 21, 2019

@Avi-D-coder to me this is a balancing of two bad things

  • CPP
  • code routinely having diagnostics reported

For me I prefer to suffer CPP for having clean builds, so I know that anything that shows up needs investigation.

@jneira jneira mentioned this pull request Dec 21, 2019
@jneira
Copy link
Member

jneira commented Dec 21, 2019

This closes #1353

@Avi-D-coder
Copy link
Collaborator Author

Hie-bios thinks Mismatching GHC versions: Multi Component project is 8.6.5, HIE is 8.8.1.
I have to use hie-8.8.1 explicitly not the wrapper.
The stack.yaml is using resolver: nightly-2019-12-18

hie.yaml

cradle:
  stack:
    - path: "./test/dispatcher/"
      component: "haskell-ide-engine:test:dispatcher-test"
    - path: "./test/functional/"
      component: "haskell-ide-engine:test:func-test"
    - path: "./test/unit/"
      component: "haskell-ide-engine:test:unit-test"
    - path: "./hie-plugin-api/"
      component: "hie-plugin-api:lib"
    - path: "./app/MainHie.hs"
      component: "haskell-ide-engine:exe:hie"
    - path: "./app/HieWrapper.hs"
      component: "haskell-ide-engine:exe:hie-wrapper"
    - path: "./"
      component: "haskell-ide-engine:lib"

@fendor
Copy link
Collaborator

fendor commented Dec 21, 2019

We cant set the used stack.yaml to use via hie.yaml, yet. Missing feature upstream in hie-bios.

@Avi-D-coder
Copy link
Collaborator Author

@fendor is bios using stack-8.6.5.yaml? stack.yaml is on a 8.8.1 resolver.

@fendor
Copy link
Collaborator

fendor commented Dec 21, 2019

Oh right, no, that is the other problem. In a multi-cradle, we dont know we are actually a stack cradle, so the wrapper asks ghc for the version. Needs also fixing in hie-bios though :/
I will create appropriate issues.

@fendor
Copy link
Collaborator

fendor commented Jan 17, 2020

@jneira Since all issues and feedback have been adressed, afaict, I root for a merge! It would be nice so we have some time to dogfood changes before the next release!

@jneira
Copy link
Member

jneira commented Jan 17, 2020

Well, at least in linux and macos, i'll have to wait to ghc-8.8.2 for windows.
Re: azure ci errors in windows: one it is due a interleaved diagnostics responses and i have a fix that i will pull soon, the other is an intermittent one (i have to investigate)

@Avi-D-coder
Copy link
Collaborator Author

Bumping the stack nightly resolver, removes an extra dep.

@Avi-D-coder
Copy link
Collaborator Author

@jneira Any idea why azure is failing?

haskell-ide-engine> Failures:
haskell-ide-engine> 
haskell-ide-engine>   test/unit/CabalHelperSpec.hs:38:5: 
haskell-ide-engine>   1) CabalHelper, cabal-helper spec, cradle discovery, dummy filepath, finds none-cradle, implicit exe, dummy filepath
haskell-ide-engine>        uncaught exception: IOException of type OtherError
haskell-ide-engine>        callProcessStderr: cabal v2-build --with-ghc=/home/vsts/.stack/programs/x86_64-linux/ghc-8.8.1/bin/ghc --with-ghc-pkg=/home/vsts/.stack/programs/x86_64-linux/ghc-8.8.1/bin/ghc-pkg --with-haddock=/home/vsts/.stack/programs/x86_64-linux/ghc-8.8.1/bin/haddock --project-file=/home/vsts/work/1/s/test/testdata/cabal-helper/implicit-exe/cabal.project --builddir=/home/vsts/work/1/s/test/testdata/cabal-helper/implicit-exe/dist-newstyle --dry-run all (exit 1): failed
haskell-ide-engine> 
haskell-ide-engine>   To rerun use: --match "/CabalHelper/cabal-helper spec/cradle discovery/dummy filepath, finds none-cradle/implicit exe, dummy filepath/"
haskell-ide-engine> 
haskell-ide-engine>   test/unit/CabalHelperSpec.hs:56:5: 
haskell-ide-engine>   2) CabalHelper, cabal-helper spec, cradle discovery, Existing projects, implicit exe
haskell-ide-engine>        uncaught exception: IOException of type OtherError
haskell-ide-engine>        callProcessStderr: cabal v2-build --with-ghc=/home/vsts/.stack/programs/x86_64-linux/ghc-8.8.1/bin/ghc --with-ghc-pkg=/home/vsts/.stack/programs/x86_64-linux/ghc-8.8.1/bin/ghc-pkg --with-haddock=/home/vsts/.stack/programs/x86_64-linux/ghc-8.8.1/bin/haddock --project-file=/home/vsts/work/1/s/test/testdata/cabal-helper/implicit-exe/cabal.project --builddir=/home/vsts/work/1/s/test/testdata/cabal-helper/implicit-exe/dist-newstyle --dry-run all (exit 1): failed

@jneira
Copy link
Member

jneira commented Jan 18, 2020

Mmm, they are failing when the CabalHelper tests are calling cabal v2-build using ghc-8.8.1. I assume that using cabal directly would work, but just in case, and for completeness, we should add ghc-8.8.1 to the Linux-Cabal job already in master. I suppose we'll need to do a rebase/merge with master to make it available in your branch.

lukel97 added a commit that referenced this pull request Jan 18, 2020
@lukel97
Copy link
Collaborator

lukel97 commented Jan 18, 2020

@Avi-D-coder @jneira needed to bump the base in the implicit-exe test project, just fixed that in 9220fda, try rebasing again now

@alanz
Copy link
Collaborator

alanz commented Jan 18, 2020

When you guys have rebased, got the tests passing, I am happy for it to go in.

@Avi-D-coder
Copy link
Collaborator Author

All the circleci tests pass. I have no idea why those particular azure tests are failing. they seem unrelated to each other and this pr?

@Avi-D-coder
Copy link
Collaborator Author

So windows 8.4.4 is broken the other two are already failing on master. Can a windows user take a look at it, I'm without a way of debugging it. 8.4 is also pretty old at this point, so we could just disable it's test on windows.

@jneira
Copy link
Member

jneira commented Jan 19, 2020

Mmm, i've reviewed the different azure ci results:

  • jobs have not deterministic errors, windows with more frequence (but i think it doesn't depend on version)
  • in any case i think this pr is not related so i think it could be merged

@alanz alanz merged commit f4600b7 into haskell:master Jan 19, 2020
@alanz
Copy link
Collaborator

alanz commented Jan 19, 2020

@Avi-D-coder Thanks for the contributions.

@alanz
Copy link
Collaborator

alanz commented Jan 19, 2020

I am getting a stack install fail, we did not add an ormolu dep. See https://gist.github.com/alanz/c0069393a899a8625283396999d12059

@alanz
Copy link
Collaborator

alanz commented Jan 19, 2020 via email

@Avi-D-coder
Copy link
Collaborator Author

@alanz Thanks for the collaborator invite.
The PR didn't conflict since stack-8.8.1.yaml did yet not exist.

@alanz
Copy link
Collaborator

alanz commented Jan 19, 2020

For the record, #1481, ping @DavSanchez

@jneira
Copy link
Member

jneira commented Jan 20, 2020

cabal build -w ghc-8.8.2 --allow-newer="ormolu:optparse-applicative" works for me so it seems ormolu only would need relax an upper bound to build with ghc-8.8.1

@Avi-D-coder
Copy link
Collaborator Author

Running ormolu test with relaxed upper bounds as we speak. I'll send a PR in a minute.

@DavSanchez
Copy link
Contributor

Hi! It seems like the solution for the build issues is already underway? If I can be of any help with this please let me know.

@Avi-D-coder Avi-D-coder mentioned this pull request Jan 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants