Skip to content

Streamline CircleCI jobs #1152

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

Merged
merged 6 commits into from
Jan 4, 2021
Merged

Streamline CircleCI jobs #1152

merged 6 commits into from
Jan 4, 2021

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Jan 3, 2021

Now that we have mostly streamlined the GitHub Actions (Windows cache still not working) the CircleCI jobs have become the bottleneck.

Changes:

  • Drop Cabal job (redundant)
  • Drop ghc-default job (what is ghc-default?)
  • Drop tests (already covered)

I have also enabled "auto cancel redundant builds" in the Project settings, which should help immensely.

@pepeiborra pepeiborra requested a review from jneira January 3, 2021 09:48
@jneira
Copy link
Member

jneira commented Jan 3, 2021

Drop Cabal job (redundant)

agree

Drop ghc-default job (what is ghc-default?)

it is using the default stack.yaml, i would keep it, maybe with a better name, as it was broken in the past inadvertently

Drop tests (already covered)

not sure about that, there were specific errors testing with stack in the past, maybe they are not possible or improbable enough to drop them
what about keep them for the last minor version (8.6.5, 8.8.4 and 8.10.3)?

@jneira
Copy link
Member

jneira commented Jan 3, 2021

@pepeiborra

what about keep them for the last minor version (8.6.5, 8.8.4 and 8.10.3)?

nvm, we can drop them optimistically and enable if we hit concrete issues with stack

Tests are already covered by the Github Actions
@pepeiborra
Copy link
Collaborator Author

Tests: making the tests pass with stack is extra hard, so I'm keen on dropping the stack tests

ghc-default: solved by deleting the unlabelled stack.yaml which was a copy of stack-8.6.5.yaml

@jneira
Copy link
Member

jneira commented Jan 3, 2021

well, i wanted to keep the stack.yaml in the first place 😄: you have to create your own or add --stack-yaml to all commands.
Newcomers can have some frustation in its first aproximation to buils hls and the price is not so high imho.

@pepeiborra
Copy link
Collaborator Author

well, i wanted to keep the stack.yaml in the first place 😄: you have to create your own or add --stack-yaml to all commands.
Newcomers can have some frustation in its first aproximation to buils hls and the price is not so high imho.

I don't think so. It's much better to pick a desired version and do ln -s stack-8.10.3.yaml stack.yaml rather than having to overwrite the existing stack.yaml every time.

@jneira
Copy link
Member

jneira commented Jan 3, 2021

I don't think so. It's much better to pick a desired version and do ln -s stack-8.10.3.yaml stack.yaml rather than having to overwrite the existing stack.yaml every time.

mmm, the default stack yaml is a way to signal what is the default ghc for us. It is 8.6.5 cause is the unique reliable enough version for all os's (let's see how behaves 8.10.3 in windows)
You cant do ln in general in windows, so you have to overwrite each time copying the desired one.
And beginners (from real chats) barely knows what is the difference between stack.yaml, package.yaml and the rationale of having several stack.yaml's or how to handle it. They will have to know it sooner or later, but imho it is better if we dont force them to do it sooner, letting stack throw them unhelpful errors.
how many time save us remove it in exchange?

@pepeiborra
Copy link
Collaborator Author

To signal the preferred stack.yaml for Windows, you could add a note in the README or use a more descriptive name, e.g. stack-windows.yaml

As for the rationale to keep multiple stack descriptors, I'm with the beginners. It doesn't make sense, for the following reasons:

  • It's unmaintainable. No one can test all those descriptors locally, it has to be done by CI. This requires a lot of iterations, at least one per version, which given how slow CI is, can take a week or two.
  • It's useless. No one really needs to use an old version of GHC (other than windows). Everyone else should stick to nightly: http://neilmitchell.blogspot.com/2015/12/whats-point-of-stackage-lts.html
  • It's confusing! The removal of stack.yaml is not an efficiency matter, it's a deconfusing matter. How long have we had two CircleCI jobs running the same Stack descriptor, for instance? How many contributors have wasted time making the same modifications twice?

My suggestion is to be pragmatic and keep just one stack.yaml on nightly, and one stack-windows.yaml on 8.6.5.

@jneira
Copy link
Member

jneira commented Jan 3, 2021

My suggestion is to be pragmatic and keep just one stack.yaml on nightly, and one stack-windows.yaml on 8.6.5.

Agree with that, but stack-windows.yaml will continue being a copy of stack-8.6.5.yaml, not sure about nightly (now we are using last nightly for stack-8.10.3.yaml f.e.).
And both should be build in ci, no?

EDIT: oh, i think you are suggesting removing the rest of stack.yaml?

@jneira
Copy link
Member

jneira commented Jan 3, 2021

Just in case, take a look (with beginner glasses) to the stack error:

PS D:\hls> ren .\stack.yaml stack.yaml.old
PS D:\hls> stack build
Error parsing targets: The specified targets matched no packages.
Perhaps you need to run 'stack init'?

It doesn't say anything about the rest of stack.yaml's and the unique suggestion will cause even more problems (stack init fails loudly in the hls repo). So high changes of a possible contributor going away...

@pepeiborra
Copy link
Collaborator Author

I'm suggesting to remove all the stack descriptors and keep only stack.yaml on Nightly and stack-windows.yaml on 8.6.5
Or if 8.10.3 works well enough in Windows, then keep just stack.yaml.

@jneira
Copy link
Member

jneira commented Jan 3, 2021

I'm suggesting to remove all the stack descriptors and keep only stack.yaml on Nightly and stack-windows.yaml on 8.6.5
Or if 8.10.3 works well enough in Windows, then keep just stack.yaml.

That would be ideal but we have to build hls for each ghc version users are actually using and there is people out there using 8.6,x and 8.8.x. They would have to write down from zero a suitable stack.yaml 😟 .
In fact the build script (Install.hs) needs those stack.yaml's to build hls with stack and with cabal! (weird, i know). The script assumes that each supported ghc versions (for both tools) has a stack-${version}.yaml in the project root.

@pepeiborra
Copy link
Collaborator Author

Ok. Restored stack.yaml and removed the duplicate CI job.

@jneira
Copy link
Member

jneira commented Jan 3, 2021

so we are in the same situation (respect the ghc-default circleci job) as in my first comment, no?:

Drop ghc-default job (what is ghc-default?)

it is using the default stack.yaml, i would keep it, maybe with a better name, as it was broken in the past inadvertently

🙂

@pepeiborra
Copy link
Collaborator Author

so we are in the same situation (respect the ghc-default circleci job) as in my first comment, no?:

Drop ghc-default job (what is ghc-default?)

it is using the default stack.yaml, i would keep it, maybe with a better name, as it was broken in the past inadvertently

🙂

Well, stack.yaml is a duplicate of stack-8.6.5.yaml which already has its own entry in the matrix. Is there any particular reason you want to run two jobs for the same descriptor?

@jneira
Copy link
Member

jneira commented Jan 3, 2021

To not break it inadvertently for no keeping it in sync.

@pepeiborra
Copy link
Collaborator Author

pepeiborra commented Jan 3, 2021

Let's delete stack-8.6.5.yaml then

EDIT: I give up. Hopefully once we have a merge bot, all this will not matter. But as a former ghcide maintainer, I find the pace of contributions in this repo too slow, and I might find myself contributing less often for that reason

@jneira
Copy link
Member

jneira commented Jan 3, 2021

Mmm it will break Install.hs for the goal hls-8.6.5, not sure it we could fix it in a generic way

@jneira
Copy link
Member

jneira commented Jan 3, 2021

but I recognize the burden of maintain that extra file, could we at least changing readme to add instructions on how to build without stack.yaml?

@Ailrun
Copy link
Member

Ailrun commented Jan 3, 2021

Just FYI, as a stack user, I agree with @jneira on most about stack.yaml and stack-***.yaml.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Many thanks for your hard work here

@jneira
Copy link
Member

jneira commented Jan 4, 2021

EDIT: I give up. Hopefully once we have a merge bot, all this will not matter. But as a former ghcide maintainer, I find the pace of contributions in this repo too slow, and I might find myself contributing less often for that reason

@pepeiborra

Sorry to hear that. It is clear that to have ghcide in a separate repo had advantages besides the drawbacks, which were more ostensible and noted before actually merging.
But i think that thanks to merge bot, this pr and the next one reducing the full tests jobs, we will restore the commit rate prior to ghcide merge. However, i am afraid that it's hard that a bigger (more after merging ghcide) repo like this, that has lot of final users, mostly beginners (unlike ghcide), can reach the swiftness of ghcide standalone.

...hard but no impossible 😃

@jneira jneira merged commit 1454b7b into haskell:master Jan 4, 2021
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.

4 participants