Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Fix the build for ghc-8.8.2. #335

Merged
merged 2 commits into from
Jan 21, 2020
Merged

Fix the build for ghc-8.8.2. #335

merged 2 commits into from
Jan 21, 2020

Conversation

robrix
Copy link
Contributor

@robrix robrix commented Jan 20, 2020

This PR fixes the build for ghc 8.8.2 by using the new LlvmVersion API (which no longer exports constructors).

Fixes #334.

@digitalasset-cla
Copy link

digitalasset-cla commented Jan 20, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor Author

@robrix robrix left a comment

Choose a reason for hiding this comment

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

Ready for review.

Comment on lines 144 to 147
#if MIN_GHC_API_VERSION(8,8,2)
Just v
| m:n:_ <- llvmVersionList v -> [ "-D__GLASGOW_HASKELL_LLVM__=" ++ format (m, n) ]
| n:_ <- llvmVersionList v -> [ "-D__GLASGOW_HASKELL_LLVM__=" ++ format (n, 0) ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t have any projects which use LLVM, so I’d appreciate it if someone could double-check that this is doing what they expect.

Note that despite llvmVersionList returning [Int], these branches are in fact exhaustive (under ghc 8.8.2), because LlvmVersion contains a NonEmpty list and converts it to a list otherwise unmodified. So we will always have at least a major version, and thus this will not silently fail (modulo future changes to ghc’s API).

@cocreature cocreature self-assigned this Jan 20, 2020
src/Development/IDE/GHC/CPP.hs Outdated Show resolved Hide resolved
src/Development/IDE/GHC/CPP.hs Outdated Show resolved Hide resolved
Just v
| m:n:_ <- llvmVersionList v -> [ "-D__GLASGOW_HASKELL_LLVM__=" ++ format (m, n) ]
| n:_ <- llvmVersionList v -> [ "-D__GLASGOW_HASKELL_LLVM__=" ++ format (n, 0) ]
#elif MIN_GHC_API_VERSION(8,8,0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This matches the way this is written in GHC and is a bit easier to read imho: https://gitlab.haskell.org/ghc/ghc/blob/ghc-8.8/compiler/main/DriverPipeline.hs#L2174

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, good call 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shame they didn’t export that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole doCpp is exported from GHC 8.10 and above, so the whole thing can disappear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndmitchell Do you mean that we should do that in this PR? Cos I’m not really set up to test against 8.10.* just now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No - we should leave it as it is for now. Once 8.10 is actually out we should do some conditional stuff - more just pointing out there is a long term plan for getting rid of it all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we need to support older versions so this can’t go away completely in particular not the code for 8.8.2. We can however, use the definition from GHC in 8.10 but let’s please keep 8.10 support for a separate PR. I expect that there are a bunch of other issues anyway.

Co-Authored-By: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
Copy link
Collaborator

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Nice, thanks a lot! We should switch our CI to 8.8.2 once stackage nightly switches over.

@cocreature cocreature merged commit 2f5d0d3 into haskell:master Jan 21, 2020
@robrix robrix deleted the ghc-8.8.2 branch January 21, 2020 13:37
pepeiborra pushed a commit to pepeiborra/ghcide that referenced this pull request Feb 1, 2020
* Fix the build for ghc-8.8.2.

* Match the single-element list first.

Co-Authored-By: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>

Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Fix the build for ghc-8.8.2.

* Match the single-element list first.

Co-Authored-By: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>

Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Fix the build for ghc-8.8.2.

* Match the single-element list first.

Co-Authored-By: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>

Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Fix the build for ghc-8.8.2.

* Match the single-element list first.

Co-Authored-By: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>

Co-authored-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
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.

Failed build with ghc 8.8.2
4 participants