Skip to content

Integrate Cabal-Helper #4

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

Closed
fendor opened this issue May 28, 2020 · 17 comments
Closed

Integrate Cabal-Helper #4

fendor opened this issue May 28, 2020 · 17 comments

Comments

@fendor
Copy link
Collaborator

fendor commented May 28, 2020

Since haskell/haskell-language-server#138 would render the Code in https://github.com/haskell/haskell-language-server/blob/master/src/Ide/Cradle.hs vastly obsolete, I would like to propose that this repo integrates Cabal-Helper, especially the CabalHelper Cradle.

A possible usage of Cabal-Helper in gen-hie would be to have two modes of generating a hie.yaml file:

  • heuristic (parsing .cabal file, etc...)
  • exact (using cabal-helper)

If you do not want to impose the dependencies on all consumers of this library, I think it would make sense to enable/disable the usage of cabal-helper via a cabal flag.

What this would bring:

  • Move vast parts of Cradle.hs to this repo, making it a possible implicit cradle discovery mechanism.
  • This repo becoming the only source of implicit cradle discovery.
  • Add a flag to gen-hie to use an exact heuristic for creating hie.yaml files.

I think this may bring us the best of both worlds: a more error resilient implicit cradle discovery in HLS, and the more exact generation of hie.yaml with cabal-helper.

The PR haskell/haskell-language-server#138 is unaffected from this issue in general.
cc @jneira

@Avi-D-coder
Copy link
Owner

I'm certainly open to this, but I don't have time to implement this (starting a fellowship on Monday).

My one minor concern is that the current cabal-helper cradle can load private libs under stack. If we converged on using cabal-helper to generate hie.yaml configs we would lose that advantage.

@jneira
Copy link
Collaborator

jneira commented May 29, 2020

Agree with @Avi-D-coder, if there are cases where even the hie-bios implicit cradle fails we could use c-h to get a correct cradle.
A problem with the fallback is the cradle could return a succesfull cradle but with emty or wrong ghc options and you cant say it for sure until you start the ghc session. That fallbackk should be implemened upstream (in hls).

@fendor
Copy link
Collaborator Author

fendor commented May 29, 2020

I think it is more important to be comprehesnible than fixing every use-case. In this sense, nesting (or sequencing) implicit cradle discovery brings us a can of worms, e.g. when one cradle mechanism works while the other doesnt.
So, how do we tell which is used by looking at logs? How do we present the error? How might we tell users how to fix it?
Even worse, if it doesnt work in both modes, do we show the error of both discovery mechanisms (bad idea, might show contradicting and unhelpful error messages)? Only of one of the modes (will make it harder to debug these modes)? Either way, the solution is imperfect anyways.

In the end, it all leads to the one and only solution: it needs to be fixed in the tools themselves and they need to be patched to support our workflow (cabal show-build-info, obelisk, stack needs to add a similar command). Everything else is just adding a bandaid.
That is why I think it would be better to have only one cradle discovery mechanism in hls (at least).
Due to the cradle abstraction, it doesnt even matter that much since changing the behaviour is a single line change. Sequencing is probably like three line changes.

As a last work-around, it is possible to allow the user to specify explicitly in a hie.yaml file to use the cabal-helper cradle. E.g.

cradle:
  other:
    cabal-helper:
      cabal:

Granted, it would break the debug binary hie-bios, because something like hie-bios debug ... would not work anymore, but we could have a similar binary in gen-hie that is aware of the cabal-helper cradle.

@Avi-D-coder
Copy link
Owner

I agree that one mechanism is better than two, even if we inherit the limitations of cabal and stack's repls.

Hie-bios's no prefixes matched error is very easy to understand, unlike cabal-helper's silent errors, linker errors, CPP errors, and whatever else I have not yet seen.

@Avi-D-coder
Copy link
Owner

I think the use cabal-helper option should be a LSP config not in hie.yaml.

@Avi-D-coder
Copy link
Owner

I still suspect integrating cabal-helper will require more code then just implementing the last two missing features.

Common stanzas just require a extra field and a pass over components.

Conditionals have to be parsed and the expressions we care about (GHC, OS) have to be evaluated.

@fendor
Copy link
Collaborator Author

fendor commented May 30, 2020

The cabal-helper cradle already exists though and I think it lends itself perfectly for the gen-hie binary.
I will prepare a PR, maybe this weekend or early next week, then we can talk about how to continue.

Conditionals have to be parsed and the expressions we care about (GHC, OS) have to be evaluated.

Requires a lot of parsing and monkey patching, imo. If gen-hie depends on Cabal, you could use the Cabal parser anyways.

@jneira
Copy link
Collaborator

jneira commented May 30, 2020

I think it is more important to be comprehesnible than fixing every use-case. In this sense, nesting (or sequencing) implicit cradle discovery brings us a can of worms, e.g. when one cradle mechanism works while the other doesnt.

Fair enough, given there will be more generic solutions from both cabal and stack.

@Avi-D-coder
Copy link
Owner

The cabal-helper cradle stuff looks good, thanks for porting it.

I'm not interested in using the Cabal as a library to parse, I'd like to keep the dependency behind a flag, since it's so big. Extracting info from cabal files is pretty straight forward since we don't care about most things. If we need a more complete parser cabal-fmt's looks like the way to go.

@fendor
Copy link
Collaborator Author

fendor commented May 30, 2020

If we use cabal-helper for generating hie.yaml, I get
this for gen-hie:

cradle:
  cabal:
    - path: "src/"
      component: "lib:implicit-hie"

    - path: "app/"
      component: "implicit-hie:exe:gen-hie"

    - path: "test/"
      component: "implicit-hie:test:implicit-hie-test"

    - path: "test/unit/"
      component: "implicit-hie:test:unit-tests"

    - path: "test/utils/"
      component: "implicit-hie:test:unit-tests"

this for haskell-ide-engine:

cradle:
  cabal:
    - path: "src/"
      component: "lib:haskell-ide-engine"

    - path: "test/dispatcher/"
      component: "haskell-ide-engine:test:dispatcher-test"

    - path: "test/functional/"
      component: "haskell-ide-engine:test:func-test"

    - path: "app/"
      component: "haskell-ide-engine:exe:hie"

    - path: "test/utils/"
      component: "haskell-ide-engine:lib:hie-test-utils"

    - path: "app/"         # <----------- this needs to be more accurate
      component: "haskell-ide-engine:exe:hie-wrapper"

    - path: "test/plugin-dispatcher/"
      component: "haskell-ide-engine:test:plugin-dispatcher-test"

    - path: "test/unit/"
      component: "haskell-ide-engine:test:unit-test"

    - path: "test/wrapper/"
      component: "haskell-ide-engine:test:wrapper-test"

    - path: "hie-plugin-api/"
      component: "lib:hie-plugin-api"

this for cabal-extras:

cradle:
  cabal:
    - path: "cabal-bundler/cli/"
      component: "cabal-bundler:exe:cabal-bundler"

    - path: "cabal-bundler/src/"
      component: "cabal-bundler:lib:cabal-bundler-internal"

    - path: "cabal-deps/cli/"
      component: "cabal-deps:exe:cabal-deps"

    - path: "cabal-deps/src/"
      component: "cabal-deps:lib:cabal-deps-internal"

    - path: "cabal-diff/cli/"
      component: "cabal-diff:exe:cabal-diff"

    - path: "cabal-diff/test/"
      component: "cabal-diff:test:cabal-diff-golden"

    - path: "cabal-diff/src/"
      component: "cabal-diff:lib:cabal-diff-internal"

    - path: "cabal-env/cli/"
      component: "cabal-env:exe:cabal-env"

    - path: "cabal-env/src/"
      component: "cabal-env:lib:cabal-env-internal"

    - path: "cabal-store-check/cli/"
      component: "cabal-store-check:exe:cabal-store-check"

    - path: "cabal-store-check/src/"
      component: "cabal-store-check:lib:cabal-store-check-internal"

    - path: "cabal-store-gc/cli/"
      component: "cabal-store-gc:exe:cabal-store-gc"

    - path: "cabal-store-gc/src/"
      component: "cabal-store-gc:lib:cabal-store-gc-internal"

    - path: "peura/src/"
      component: "lib:peura"

this for cabal

cradle:
  cabal:
    - path: "Cabal/"
      component: "lib:Cabal"

    - path: "Cabal/tests/"
      component: "Cabal:test:check-tests"

    - path: "Cabal/tests/custom-setup/"
      component: "Cabal:test:custom-setup-tests"

    - path: "Cabal/tests/"
      component: "Cabal:test:hackage-tests"

    - path: "Cabal/tests/"
      component: "Cabal:test:parser-tests"

    - path: "Cabal/tests/"
      component: "Cabal:test:rpmvercmp"

    - path: "Cabal/tests/"
      component: "Cabal:test:unit-tests"

    - path: "Cabal/Cabal-quickcheck/src/"
      component: "Cabal:test:unit-tests"

    - path: "Cabal/Cabal-quickcheck/src/"
      component: "lib:Cabal-quickcheck"

    - path: "cabal-benchmarks/bench/"
      component: "cabal-benchmarks:test:cabal-benchmarks"

    - path: "cabal-install/main/"
      component: "cabal-install:exe:cabal"

    - path: "cabal-install/solver-dsl/"
      component: "cabal-install:lib:cabal-install-solver-dsl"

    - path: "cabal-install/"
      component: "cabal-install:lib:cabal-lib-client"

    - path: "cabal-install/tests/"
      component: "cabal-install:test:integration-tests2"

    - path: "cabal-install/tests/"
      component: "cabal-install:test:memory-usage-tests"

    - path: "cabal-install/tests/"
      component: "cabal-install:test:solver-quickcheck"

    - path: "cabal-install/tests/"
      component: "cabal-install:test:unit-tests"

    - path: "cabal-testsuite/src/"
      component: "lib:cabal-testsuite"

    - path: "cabal-testsuite/main/"
      component: "cabal-testsuite:exe:cabal-tests"

    - path: "cabal-testsuite/"
      component: "cabal-testsuite:exe:setup"

    - path: "solver-benchmarks/"
      component: "lib:solver-benchmarks"

    - path: "solver-benchmarks/main/"
      component: "solver-benchmarks:exe:hackage-benchmark"

    - path: "solver-benchmarks/tests/"
      component: "solver-benchmarks:test:unit-tests"

Looks pretty alright to use already. Load time is horrible, since we are building every component in the package, but I imagine that it will be the "more correct" approach.

@Avi-D-coder
Copy link
Owner

This looks pretty good. We need to handle multiple executables and other-modules and nested packages.

The duplicate other-modules (./exe/Arguments.hs) files are not a issue since any competent that uses it must have the it's dependency (The first listing one is allays used by hls).

hls

cradle:
  cabal:
    - path: "./src"
      component: "lib:haskell-language-server"

    - path: "./exe/Main.hs"
      component: "haskell-language-server:exe:haskell-language-server"

    - path: "./exe/Arguments.hs"
      component: "haskell-language-server:exe:haskell-language-server"

    - path: "./exe/Wrapper.hs"
      component: "haskell-language-server:exe:haskell-language-server-wrapper"

    - path: "./exe/Arguments.hs"
      component: "haskell-language-server:exe:haskell-language-server-wrapper"

    - path: "./test/functional"
      component: "haskell-language-server:test:func-test"

    - path: "ghcide/src"
      component: "lib:ghcide"

    - path: "ghcide/src-ghc86"
      component: "lib:ghcide"

    - path: "ghcide/src-ghc88"
      component: "lib:ghcide"

    - path: "ghcide/src-ghc810"
      component: "lib:ghcide"

    - path: "ghcide/test/preprocessor/Main.hs"
      component: "ghcide:exe:ghcide-test-preprocessor"

    - path: "ghcide/exe/Main.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/exe/Utils.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/exe/Arguments.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/exe/Paths_ghcide.hs"
      component: "ghcide:exe:ghcide"

    - path: "ghcide/test/cabal"
      component: "ghcide:test:ghcide-tests"

    - path: "ghcide/test/exe"
      component: "ghcide:test:ghcide-tests"

    - path: "ghcide/test/src"
      component: "ghcide:test:ghcide-tests"

@jneira
Copy link
Collaborator

jneira commented Jun 4, 2020

I wonder if we could get that level of precision but faster only reading the (cabal.project and .cabal files) configuration using cabal-parsers instead cabal-helper.
As Cabal usually is pretty backwards compatible we could statically link the last Cabal version.
And for stack you have to parse yaml...

@fendor
Copy link
Collaborator Author

fendor commented Jun 4, 2020

@jneira I like that idea!

@Avi-D-coder
Copy link
Owner

Does cabal-parsers render common stanzas and conditionals or is that in higher in cabals stack?

@jneira
Copy link
Collaborator

jneira commented Jun 4, 2020

Afaik all those features are included in the parser.

@Avi-D-coder
Copy link
Owner

That sounds like a great way to go

@jneira
Copy link
Collaborator

jneira commented Jun 4, 2020

Well, to be precise the package is cabal-install-parser:

  • for .cabalfiles it returns the GenericPackageDescription. The conditionals are represented by data structures, so maybe it doesnt have the code to actually compute them. We would need full Cabal or copy the code.
  • for cabal.project it returns a Project type
    • including a utility to parse all packages of a project in one go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants