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.

Sign up for GitHub

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

Jump to bottom

Add optional component to stack cradle and multi-stack #97

Merged
merged 4 commits into from
Dec 13, 2019
Merged

Add optional component to stack cradle and multi-stack #97

merged 4 commits into from
Dec 13, 2019

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Dec 2, 2019 •
edited
Loading

Closes #96

  • Don't pass a FilePath to the cradle, rely on the magic all component logic
  • Add an option to specify a specific component, like the cabal cradle
  • Simple parsing of the ghci script to file :add and :load targets.

@fendor fendor force-pushed the stack-changes branch 3 times, most recently from 72df4fb to 4287277 Compare December 3, 2019 11:05
@fendor fendor changed the title WIP: Add optional component to stack cradle and multi-stack Add optional component to stack cradle and multi-stack Dec 3, 2019
@fendor fendor requested review from mpickering and jneira December 3, 2019 19:13
mpickering
mpickering reviewed Dec 4, 2019
View reviewed changes
src/HIE/Bios/Cradle.hs Outdated Show resolved Hide resolved
mpickering
mpickering reviewed Dec 4, 2019
View reviewed changes
src/HIE/Bios/Cradle.hs Outdated Show resolved Hide resolved
mpickering
mpickering reviewed Dec 4, 2019
View reviewed changes
wrappers/cabal Outdated Show resolved Hide resolved
@mpickering
Copy link
Collaborator

Great patch thanks. One structural comment about where we parse the ghci script file.

@jneira
Copy link
Member

jneira commented Dec 4, 2019 •
edited
Loading

This would make test components work as well (at least for stack cradle)?

I've run the test suite on windows and it passed (well with the same failing test -simple-bios- than master)

@fendor
Copy link
Collaborator Author

fendor commented Dec 4, 2019

Locally, the tests succeed for me as well, I just fail to make CI happy :/

@mpickering
Copy link
Collaborator

I was just thinking that instead of calling ghc directly for stack we should call stack exec ghc? That would make it also work for the docker integration I believe and avoid having to manually deal with the compiler path.

@fendor
Add environment variables for ghc.
de954c4 
* On windows, use either ghc or stack to compile the wrapper.
* Parse .ghci script to obtain all targets.
* Change tests to fit the new target selection.
@fendor fendor requested a review from mpickering December 13, 2019 20:41
@fendor
Copy link
Collaborator Author

fendor commented Dec 13, 2019

CI is happy again!
Turns out, there were actually mistakes in the hie.yaml files, they were actually using cabal and now we fix this by writing a stack.yaml based on the used ghc.

@fendor
Copy link
Collaborator Author

fendor commented Dec 13, 2019

I think, we do have a breaking change in hie.yaml:

cradle:
  stack:

is no longer a valid yaml configuration.
My suggestion would be to lift this restriction, for cabal and stack. If there is no object, then just assume Nothing. @mpickering, @jneira opinion?

@jneira
Copy link
Member

jneira commented Dec 13, 2019

Sounds good to me.
The test suite in windows continue stable: only one failing test (same than master)

@fendor fendor force-pushed the stack-changes branch 3 times, most recently from b075ad7 to 95bbaec Compare December 13, 2019 21:18
mpickering
mpickering reviewed Dec 13, 2019
View reviewed changes
Copy link
Collaborator

@mpickering mpickering left a comment

Choose a reason for hiding this comment

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

LGTM. Great stuff (as always).

tests/BiosTests.hs Outdated Show resolved Hide resolved
@mpickering
Copy link
Collaborator

I think this can be merged once CI passes?

@mpickering mpickering merged commit 27add56 into haskell:master Dec 13, 2019
@jneira jneira mentioned this pull request Dec 18, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpickering mpickering mpickering left review comments

@jneira jneira jneira approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Changes to the stack cradle
3 participants
@fendor @mpickering @jneira