Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 26 commits into from
Nov 21, 2022

Conversation

runeksvendsen
Copy link
Contributor

@runeksvendsen runeksvendsen commented Jun 13, 2022

Depends on #2945

@runeksvendsen
Copy link
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
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
CODEOWNERS Outdated Show resolved Hide resolved
@fendor fendor requested a review from kokobd July 14, 2022 10:38
@kokobd
Copy link
Collaborator

kokobd commented Jul 14, 2022

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
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
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
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 :)

plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal.hs Outdated Show resolved Hide resolved
plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal.hs Outdated Show resolved Hide resolved
@pepeiborra
Copy link
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
Copy link
Collaborator

fendor commented Aug 13, 2022

@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
Copy link
Collaborator

@michaelpj michaelpj left a comment

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