Skip to content
This repository was archived by the owner on Jan 2, 2021. It is now read-only.

GitHub test action #903

Merged
merged 12 commits into from
Nov 15, 2020
Merged

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Nov 11, 2020

The GitHub test action uses Cabal rather than Stack to build and run tests in a wider array of platforms and versions than we have now.

Keeping both the Github action and the Azure pipeline will increase the maintenance overhead, so we should decide for one or the other.

One important advantage of the GitHub Actions is that they work on forks, whereas the current Azure pipeline doesn't.

If we end up dropping the Azure one and by extension the stack tests, my intent is to also drop the Stack descriptors and the stack cradle.

Just found that there are some features missing in the GitHub action:

  • hlint
  • wError

@pepeiborra pepeiborra changed the title GitHub actions test GitHub test action Nov 11, 2020
@wz1000
Copy link
Collaborator

wz1000 commented Nov 11, 2020

Dropping stack works for me.

One important advantage of the GitHub Actions is that they work on forks, whereas the current Azure pipeline doesn't.

I find the email spam this generates to be a considerable disadvantage, and I could only find one toggle to disable all emails from Github actions, nothing finer than that.

@jneira
Copy link
Member

jneira commented Nov 11, 2020

I am for switch to github actions but not sure about stack. I think we should try to be build tool agnostic and make it easy for stack users start hacking ghcide/hls (or continue to do it! i think @ndmitchell uses stack for example).
You can use stack in github haskell actions too and i would keep a compile job using it (and test only with cabal).

@jneira
Copy link
Member

jneira commented Nov 12, 2020

The compilation is failing for azure due to -Werror

ghcide> [5 of 5] Compiling Main
ghcide> 
ghcide> /home/vsts/work/1/s/test/exe/Main.hs:2274:3: error: [-Wunused-local-binds, -Werror=unused-local-binds]
ghcide>     Defined but not used: ‘cccL17’
ghcide>      |
ghcide> 2274 |   cccL17 = Position 17 16  ;  docLink = [ExpectHoverText ["[Documentation](file:///"]]
ghcide>      |   ^^^^^^
ghcide> 
ghcide> /home/vsts/work/1/s/test/exe/Main.hs:2274:31: error: [-Wunused-local-binds, -Werror=unused-local-binds]
ghcide>     Defined but not used: ‘docLink’
ghcide>      |
ghcide> 2274 |   cccL17 = Position 17 16  ;  docLink = [ExpectHoverText ["[Documentation](file:///"]]

I would keep -Werror, helps a lot to keep clean the codebase

@pepeiborra
Copy link
Collaborator Author

Dropping stack works for me.

One important advantage of the GitHub Actions is that they work on forks, whereas the current Azure pipeline doesn't.

I find the email spam this generates to be a considerable disadvantage, and I could only find one toggle to disable all emails from Github actions, nothing finer than that.

This is complaining for the sake of it. I disabled Github emails years ago. When I want to get a Github update, I open the notifications page.

@pepeiborra
Copy link
Collaborator Author

The compilation is failing for azure due to -Werror

ghcide> [5 of 5] Compiling Main
ghcide> 
ghcide> /home/vsts/work/1/s/test/exe/Main.hs:2274:3: error: [-Wunused-local-binds, -Werror=unused-local-binds]
ghcide>     Defined but not used: ‘cccL17’
ghcide>      |
ghcide> 2274 |   cccL17 = Position 17 16  ;  docLink = [ExpectHoverText ["[Documentation](file:///"]]
ghcide>      |   ^^^^^^
ghcide> 
ghcide> /home/vsts/work/1/s/test/exe/Main.hs:2274:31: error: [-Wunused-local-binds, -Werror=unused-local-binds]
ghcide>     Defined but not used: ‘docLink’
ghcide>      |
ghcide> 2274 |   cccL17 = Position 17 16  ;  docLink = [ExpectHoverText ["[Documentation](file:///"]]

I would keep -Werror, helps a lot to keep clean the codebase

Of course I intend to keep -Werror and hlint. The only thing I am not intending to keep is stack

@jneira
Copy link
Member

jneira commented Nov 12, 2020

Of course I intend to keep -Werror and hlint

Nice, i thought you were in doubt for -Wall for the question mark after its entry in the description.

@pepeiborra pepeiborra force-pushed the github-actions-test branch 6 times, most recently from f6e8b78 to 459347c Compare November 13, 2020 20:48
@pepeiborra
Copy link
Collaborator Author

This is looking good. I propose that we keep the Azure pipeline for now but reduce it to one stack.yaml file:

  1. (this PR) Delete stack.yaml and stack88.yaml
  2. (this PR) Promote stack810.yaml to stack.yaml
  3. (this PR) Update the stack Azure pipeline
  4. (future work) Move the stack pipeline to a Github Action

@pepeiborra pepeiborra force-pushed the github-actions-test branch 8 times, most recently from 5083678 to 049302b Compare November 14, 2020 12:14
@pepeiborra pepeiborra force-pushed the github-actions-test branch 3 times, most recently from 48b651e to 2c7ca3f Compare November 14, 2020 21:05
@jneira
Copy link
Member

jneira commented Nov 14, 2020

My proposal is:

  • keep all actual stack.yaml's (for ghc 8.6, 8.8 and 8.10)
  • build them without running tests in ci

Reasons:

  • we need a stack.yaml for ghc-8.6 and windows
    • but it would be not suited for linux and macos, cause ghc-8.10 is reliable for them and quite users have switched to
  • stack users needs compile ghcide with the same ghc they are using for their projects like everyone else
    • and the project should be friendly for stack users 😄
  • it is possible that all cabal buils and the unique stack one are succesful but failed for other stack resolvers
    • ant it is not improbable due to the amount of ghc related code under cpp conditions

@pepeiborra
Copy link
Collaborator Author

My proposal is:

  • keep all actual stack.yaml's (for ghc 8.6, 8.8 and 8.10)
  • build them without running tests in ci

Reasons:

  • we need a stack.yaml for ghc-8.6 and windows

    • but it would be not suited for linux and macos, cause ghc-8.10 is reliable for them and quite users have switched to
  • stack users needs compile ghcide with the same ghc they are using for their projects like everyone else

    • and the project should be friendly for stack users 😄
  • it is possible that all cabal buils and the unique stack one are succesful but failed for other stack resolvers

    • ant it is not improbable due to the amount of ghc related code under cpp conditions

Your proposal seems reasonable, but it doesn't help with the original motivation for this PR, which is to move away from the Azure CI. And I have no plans to write a Github action for stack.

Moreover, keeping a family of stack descriptors and the associated CI does little for us and has considerable overhead. It won't catch additional build errors over the Cabal CI, in fact, it hides them. cabal build hie-compat -f ghc-lib fails to build without additional constraints and no one noticed.

A more economical solution would be to update the Contributing section of the README to point Windows users towards Chocolatey and/or a specific version of GHC.

@pepeiborra pepeiborra force-pushed the github-actions-test branch 2 times, most recently from cafd4f2 to 77197e7 Compare November 15, 2020 10:47
@ndmitchell
Copy link
Collaborator

A more economical solution would be to update the Contributing section of the README to point Windows users towards Chocolatey and/or a specific version of GHC.

To split things out appropriately, there are:

  1. The things we support end users of. For Windows that has to be Cabal/Stack on many versions of GHC. We can't demand Chocolatey or a specific version.
  2. The things we test with, in order to gain confidence that we are supporting what we think. For all my projects, I test with Linux/Cabal on multiple GHC versions, and Windows/Stack on a single GHC version. That gives me confidence in Linux/Stack/other-versions and Windows/Cabal/other-versions without having tested them. I've yet to find gaps in my coverage due to that significant simplification. For projects that are more OS centric I also do a single Mac/Cabal build.
  3. What we support for developers. The wider we can go, the better, but requiring chocolatey, GHC 8.10.2 and a locale setting of Klingon for Windows isn't fatal.

I note that the Haskell community just started a huge foundation to make Windows stuff, including IDE, easier. If we need to see help, we could do so.

@pepeiborra
Copy link
Collaborator Author

  1. ghcide is not an end user product anymore - https://neilmitchell.blogspot.com/2020/09/dont-use-ghcide-anymore-directly.html

  2. Roughly what this PR does. cabal test on multiple versions and OS, stack test on Linux nightly. If we need to test stack on Windows, then we'll just keep 2 stack descriptors around.

  3. If we keep a stack descriptor for Windows, then there's no need to adjust the Contributing guidelines

@pepeiborra pepeiborra force-pushed the github-actions-test branch 3 times, most recently from a9edc00 to a32478b Compare November 15, 2020 12:27
We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI
@pepeiborra
Copy link
Collaborator Author

  • Updated to keep two stack descriptors, one for nightly and one for Windows
  • Updated the Linux and Windows stack CI scripts to drop testing
  • Updated the test suite to drop all the stack specific workarounds
  • Updated the README to point users to the Haskell extension and the HLS precompiled binaries
  • Updated the Contributing section to mention that stack test is unsupported

I'm happy with this and hopefully it meets everyone else's bar too. If there are any other concerns, I'd rather hand this PR over.

@ndmitchell
Copy link
Collaborator

ghcide is not an end user product anymore

Agreed, I don't think we necessarily need to ship each of a stack-86/88/810.yaml, since the existence of a Cabal build implies there probably is a stack.yaml. I think once Windows ends its hilariously bad run of GHC releases, having a single Stack.yaml should be plenty, and one day maybe even we don't need that as stack init will build a working one.

I'm happy with the end state you've reached, what say others?

@jneira
Copy link
Member

jneira commented Nov 15, 2020

if @ndmitchell, as one of the stack users of the project is fine with actual changes, me too, of course.
@pepeiborra thanks for try hard to reach a compromise

@pepeiborra pepeiborra merged commit 03e89d9 into haskell:master Nov 15, 2020
@jneira
Copy link
Member

jneira commented Nov 16, 2020

Just in case someone could still need them, i will keep in sync a branch with stack.yaml's for ghc-8.6 and ghc-8.8 for a while: https://github.com/jneira/ghcide/tree/stack-support
If nobody asks for them for some time i will delete it.

pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Add github test action

* Disable unreliable test

Does not work reliably on all platforms.

Reenable when haskell/ghcide#861 lands

* Add hlint and -Werror

* Explicit timeout

6h is the default and also the maximum:

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration

* Experiment tests to use Cabal instead of Stack

* Fix an unreliable test

* Trim down matrix

* Add ghc-lib to the test matrix

* Address broken hie-compat ghc-lib build

* Drop stack descriptor family

We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI

* Update README to point end users to HLS

* Drop support for `stack test`
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Add github test action

* Disable unreliable test

Does not work reliably on all platforms.

Reenable when haskell/ghcide#861 lands

* Add hlint and -Werror

* Explicit timeout

6h is the default and also the maximum:

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration

* Experiment tests to use Cabal instead of Stack

* Fix an unreliable test

* Trim down matrix

* Add ghc-lib to the test matrix

* Address broken hie-compat ghc-lib build

* Drop stack descriptor family

We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI

* Update README to point end users to HLS

* Drop support for `stack test`
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
* Add github test action

* Disable unreliable test

Does not work reliably on all platforms.

Reenable when haskell/ghcide#861 lands

* Add hlint and -Werror

* Explicit timeout

6h is the default and also the maximum:

https://docs.github.com/en/free-pro-team@latest/actions/reference/usage-limits-billing-and-administration

* Experiment tests to use Cabal instead of Stack

* Fix an unreliable test

* Trim down matrix

* Add ghc-lib to the test matrix

* Address broken hie-compat ghc-lib build

* Drop stack descriptor family

We keep two stack descriptors:

- One for Nightly
- One for Windows (stuck in GHC 8.10.1)

To ensure that `stack test` doesn't break, we keep running the stack tests in CI

* Update README to point end users to HLS

* Drop support for `stack test`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants