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

Detect implicit cabal cradle in the absence of cabal.project #221

Merged
merged 2 commits into from
Jul 7, 2020

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Jul 3, 2020

A cabal cradle doesn't need a cabal.project file and isn't guaranteed to have one either. This improves the implicit cradle search for cabal by falling back to finding .cabal files if hie-bios couldn't find any cabal.project files.
Because it falls back, if hie-bios tries to query a multi-package cabal project with a nested cabal package, it will still find use the "larger" cradle with the upper cabal.project file. See the implicit-cabal-deep-project test project for an example of this.

@mpickering
Copy link
Collaborator

When would the implicit stack cradle be selected with this logic?

@lukel97
Copy link
Contributor Author

lukel97 commented Jul 4, 2020

Always selected first, if possible. Cabal cradles are the “last resort” in the implicit lookup and this only extends the Cabal part

@lukel97 lukel97 force-pushed the implicit-cabal-no-project branch 3 times, most recently from d6ba01e to 82b39d5 Compare July 6, 2020 15:11
tests/BiosTests.hs Outdated Show resolved Hide resolved
@lukel97 lukel97 force-pushed the implicit-cabal-no-project branch from 82b39d5 to bef1ffc Compare July 6, 2020 18:05
@lukel97 lukel97 requested a review from fendor July 6, 2020 20:13
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Since the .hie-bios.stop file doesnt do anything, it should be removed

tests/BiosTests.hs Outdated Show resolved Hide resolved
tests/BiosTests.hs Outdated Show resolved Hide resolved
@lukel97 lukel97 requested a review from fendor July 7, 2020 12:09
@lukel97 lukel97 force-pushed the implicit-cabal-no-project branch from 666de6a to 77e8132 Compare July 7, 2020 12:13
A cabal cradle doesn't need a cabal.project file and isn't guaranteed to
have one either. This improves the implicit cradle search for cabal by
falling back to finding .cabal files if hie-bios couldn't find any
cabal.project files.
Because it falls back, if hie-bios tries to query a multi-package cabal
project with a nested cabal package, it will still find use the "larger"
cradle with the upper cabal.project file. See the
implicit-cabal-deep-project test project for an example of this.
@lukel97 lukel97 force-pushed the implicit-cabal-no-project branch from 77e8132 to 051ec49 Compare July 7, 2020 12:21
@@ -32,7 +32,7 @@ jobs:
key: ${{ runner.OS }}-${{ matrix.ghc }}-cabal-0

- name: Run ShellCheck
uses: ludeeus/action-shellcheck@master
uses: ludeeus/action-shellcheck@2f2aa0d
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workaround for this

Comment on lines +205 to +207
-- don't spin up a ghc session, just run the opts program manually since
-- we're not guaranteed to be able to get the ghc libdir if the cradle is
-- failing
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is interesting and not documented: https://github.com/mpickering/hie-bios/blob/master/src/HIE/Bios/Types.hs#L60
Or what do you mean with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that hie-bios no longer uses ghc-paths it has to call cabal exec ghc for the failing-cabal test case to set up the ghc session. But cabal exec ghc fails because it can’t resolve the constraints in the first place! So we aren’t able to get a lib dir. Prior to this it was just working by fluke, since there’s no cabal.project in this directory cabal exec ghc was using the hie-bios project to resolve it fine.
We can still test loading the flags etc and it will still fail, we just need to do it without a ghc session

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should document it better that we require runCradle to succeed and how the results for libdir may be non-sensical if it fails?

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

There are still the .hie-bios.stop files in the PR, not necessary right?

@lukel97
Copy link
Contributor Author

lukel97 commented Jul 7, 2020

@fendor let's remove them when we revert the stop files commit

This prevents the cabal.project/hie.yaml of hie-bios's source tree from
interfering with the cradle stuff
@lukel97 lukel97 force-pushed the implicit-cabal-no-project branch from 051ec49 to df4941b Compare July 7, 2020 13:19
@fendor fendor merged commit f1dbfc1 into haskell:master Jul 7, 2020
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

Successfully merging this pull request may close these issues.

None yet

3 participants