Skip to content

Parse .cabal files; show error and warning diagnostics#2954

Merged
mergify[bot] merged 26 commits into
haskell:masterfrom
runeksvendsen:rune/hls-cabal-plugin
Nov 21, 2022
Merged

Parse .cabal files; show error and warning diagnostics#2954
mergify[bot] merged 26 commits into
haskell:masterfrom
runeksvendsen:rune/hls-cabal-plugin

Conversation

@runeksvendsen

@runeksvendsen runeksvendsen commented Jun 13, 2022

Copy link
Copy Markdown
Contributor

Depends on #2945

@runeksvendsen

Copy link
Copy Markdown
Contributor Author

Wait... do all the plugins share the same dependency version? This plugin uses the parser from Cabal==3.6.2.0, and by looking at the CI logs it fails because it tries to use the same Cabal version for everything? :-/

@michaelpj

Copy link
Copy Markdown
Collaborator

All of the Haskell packages are compiled together into a single build plan by Cabal, yes. But I don't think any other plugins impose version constraints on Cabal. Possibly there's something to do with the Cabal library being bundled with GHC? It wouldn't shock me if we were only able to enable this on newer GHCs which give us a newer cabal library.

@fendor
fendor force-pushed the rune/hls-cabal-plugin branch 4 times, most recently from 0818d3c to 5c6eeaa Compare June 23, 2022 16:09
@fendor
fendor force-pushed the rune/hls-cabal-plugin branch 5 times, most recently from d3e68ba to 062b62a Compare July 11, 2022 06:47
@fendor
fendor marked this pull request as ready for review July 11, 2022 07:20
@fendor fendor changed the title WIP: parse .cabal files; show error and warning diagnostics Parse .cabal files; show error and warning diagnostics Jul 11, 2022
@fendor
fendor force-pushed the rune/hls-cabal-plugin branch from 062b62a to a0b7ff8 Compare July 11, 2022 07:21
Comment thread CODEOWNERS Outdated
@fendor
fendor requested a review from kokobd July 14, 2022 10:38
@kokobd

kokobd commented Jul 14, 2022

Copy link
Copy Markdown
Collaborator

Mention it in the docs? For example section "Features", "Supported GHC Versions", etc.

build-depends:
, base >=4.12 && <5
, bytestring
, Cabal ^>=3.2 || ^>=3.4 || ^>=3.6

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So, this is the Cabal we use to build HLS. What will happen when this version mismatches the user's cabal-install version? For GHC, we require the versions to equal. Maybe mention this information somewhere in docs.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

iirc, it would not be a problem to use the same cabal version for all GHC versions here, since Cabal 3.6 still compiles with GHC 8.8. However, stack makes it difficult, as we would have to either allow newer Cabal versions for all dependencies, or make the old resolver use the newer version for virtually all dependencies (e.g. packages that needed only version revisions). Don't know what do exactly do here, however, pre-built executables aren't build with stack so it should always use Cabal 3.6, afaict.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this generally isn't too much of a problem: this plugin mostly operates on cabal files, and different versions of cabal usually treat cabal files the same; differences in behaviour are controlled with the cabal-version field. So I think we're okay. BUT worth a comment :)

Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal.hs Outdated
Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal.hs Outdated
Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Diagnostics.hs
Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Parse.hs Outdated
@pepeiborra

Copy link
Copy Markdown
Collaborator

@runeksvendsen are you able to wrap up this PR?

@pepeiborra pepeiborra added the status: unfinished Status for PRs that have been semi-abandoned label Aug 13, 2022
@fendor

fendor commented Aug 13, 2022

Copy link
Copy Markdown
Collaborator

@pepeiborra I am making sure this PR will be pushed over the finish line. At the moment, we struggle with indeterministic test failure. Also, I want to add more tests, before merging.

@fendor
fendor force-pushed the rune/hls-cabal-plugin branch 3 times, most recently from 23066ed to 0ebea4d Compare August 14, 2022 14:54
@fendor
fendor force-pushed the rune/hls-cabal-plugin branch from 3cce877 to 996d6a1 Compare November 19, 2022 14:03
@fendor
fendor requested a review from berberman as a code owner November 19, 2022 14:37
@fendor
fendor requested a review from michaelpj November 19, 2022 15:31
@fendor
fendor force-pushed the rune/hls-cabal-plugin branch from 24f9f9b to 701915c Compare November 19, 2022 16:31

@michaelpj michaelpj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@fendor fendor added the merge me Label to trigger pull request merge label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge me Label to trigger pull request merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants