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

Upgrade to resolver 20.11 #1158

Merged
merged 10 commits into from
May 2, 2023
Merged

Upgrade to resolver 20.11 #1158

merged 10 commits into from
May 2, 2023

Conversation

mx-ws
Copy link
Contributor

@mx-ws mx-ws commented Mar 11, 2023

Hi, this is my first pull request, so please point me to the relevant resources if I did something wrong :D
I wanted to run the tests locally and ran into this ghc on m1 issue: https://gitlab.haskell.org/ghc/ghc/-/issues/20592
and then into this one #1058

So what I did is basically one-liners similar to what is described in latter issue:

grep -rl 'resolver' | xargs sed -i '' -e 's/19.27/20.13/g'
grep -rl 'configFastFail' | xargs sed -i '' -e 's/configFastFail/configFailFast/g'

I ran stack test in some of the exercise projects and it seems to be fixed for me now.

Also one question that I have: Does the configFastFail change count as a change to the tests as described in the README, so that I should increment the serial number of the version in each exercise?

and configFastFail to configFailFast due to hspec 2.9.0
@github-actions
Copy link

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Mar 11, 2023
@MatthijsBlom
Copy link
Contributor

The version of GHC currently recommended by GHCup is GHC 9.2.5.

[I] ran into this ghc on m1 issue: https://gitlab.haskell.org/ghc/ghc/-/issues/20592

In what sense / how did you work around this / would this still be an issue for others?

I ran stack test in some of the exercise projects and it seems to be fixed for me now.

The checks were cancelled because this PR was closed, but normally they would automatically check all exercises, I guess using this code.

@mx-ws
Copy link
Contributor Author

mx-ws commented Mar 14, 2023

The version of GHC currently recommended by GHCup is GHC 9.2.5.

Ok. stack --resolver lts build uses resolver: lts-20.14. While stack --resolver its-19 uses ghc-9.0.2, which does not have the fix I was trying to incorporate. What would you suggest here?

[I] ran into this ghc on m1 issue: https://gitlab.haskell.org/ghc/ghc/-/issues/20592

In what sense / how did you work around this / would this still be an issue for others?

I also had the error message about 'ffitarget_arm64.h' file not being found. The issue I linked considers this a bug and states this bug has been fixed in ghc-9.2.3. I then tried to understand stack as far as to make it use ghc >=9.2.3. Sorry, but I forgot how I came up with resolver: 20.13, but as I stated above, resolver: 20.14 is the latest lts.

Furthermore, my understanding is that this would be an issue for everyone who uses a computer with an m1 chip and tries to build an exercise locally.

I ran stack test in some of the exercise projects and it seems to be fixed for me now.

The checks were cancelled because this PR was closed, but normally they would automatically check all exercises, I guess using this code.

@MatthijsBlom
Copy link
Contributor

Do I understand correctly that

  • you are using M1,
  • GHC 9.0.2 does not work for you (because M1), and
  • GHC 9.2 does work?

What would you suggest here?

I gravitate towards lts-20.11, i.e. GHC 9.2.5, the current recommendation of GHCup (though I couldn't find out why).

@mx-ws
Copy link
Contributor Author

mx-ws commented Mar 14, 2023

Do I understand correctly that

  • you are using M1,

Yes.

  • GHC 9.0.2 does not work for you (because M1), and

Yes.

  • GHC 9.2 does work?

To be precise, the bug is supposed to be fixed with 9.2.3 and I can confirm for 9.2.5 and 9.2.7. But yes.

@MatthijsBlom
Copy link
Contributor

@ErikSchierboom @petertseng This PR should be reopened. It affects M1 users for the better. As a bonus, GHC 9.2 comes with GHC2021 enabled by default.

@MatthijsBlom
Copy link
Contributor

@mx-ws You missed docs/TESTS.md.

@mx-ws
Copy link
Contributor Author

mx-ws commented Mar 14, 2023

Thanks, I changed it there too. Also, I set all the resolver versions to 20.11 for now.

@ErikSchierboom
Copy link
Member

@mx-ws Thanks! Could you fix the merge conflict please?

@mx-ws
Copy link
Contributor Author

mx-ws commented Mar 15, 2023

@mx-ws Thanks! Could you fix the merge conflict please?

Done.

@ErikSchierboom
Copy link
Member

@mx-ws Did you push the change? I'm still seeing the conflict

@mx-ws
Copy link
Contributor Author

mx-ws commented Mar 15, 2023

Oops, I thought I did this. You should see the merge now.

@ErikSchierboom
Copy link
Member

There are two things left to do:

  1. CI is failing. See https://github.com/exercism/haskell/actions/runs/4429565971/jobs/7780928191
  2. The Haskell test runner needs to be updated to support the listed resolver version https://github.com/exercism/haskell-test-runner/

@mx-ws
Copy link
Contributor Author

mx-ws commented Mar 17, 2023

Hm, it is a 'Pattern match(es) are non-exhaustive' error. A list was matched as a 2-element list in a place where the list has already been checked to be of size 2.

I rewrote the relevant place to remove the error in the first way I could think of. I don't understand how this error was caused by the update though.

@mx-ws
Copy link
Contributor Author

mx-ws commented Mar 18, 2023

  1. The Haskell test runner needs to be updated to support the listed resolver version https://github.com/exercism/haskell-test-runner/

As you see, I created a pull request over there. Should I create a forum thread this time (as the automatically sent mail suggests) or just leave it like that?

@ErikSchierboom ErikSchierboom changed the title Upgrade to resolver 20.13 Upgrade to resolver 20.11 Mar 21, 2023
@hesselink
Copy link

Any progress on this, or something I can do to help get this merged? I ran into the same issue on an M1 mac, and while I'm perfectly capable of updating the stack.yaml for each exercise, I'm worried this could be a bigger hurdle for less experienced people.

@mx-ws
Copy link
Contributor Author

mx-ws commented Apr 23, 2023

Any progress on this, or something I can do to help get this merged?

I was just working on the corresponding pull request for the haskell-test-runner. I'm battling the expected-results right now. My next step would be to fix the expected-results in this repository. But it will probably take me another week or two to get to this. So this would be something you could do =)

@denismaier
Copy link

Would be great if this could be merged soon. I've just ran into the error in the online editor.

@MatthijsBlom
Copy link
Contributor

MatthijsBlom commented May 2, 2023

@ErikSchierboom the merging of exercism/haskell-test-runner#72 has (temporarily) broken all exercises. Merging this PR should fix it.

Alternatively the workaround suggested by #1058 (comment) might work, but that is less certain and takes more research, i.e. more hours.

@ErikSchierboom
Copy link
Member

Once CI is green I can merge this

@ErikSchierboom
Copy link
Member

CI is failing. Could someone take a look?

@MatthijsBlom
Copy link
Contributor

I'm already looking into it.

@MatthijsBlom
Copy link
Contributor

@ErikSchierboom At minimum, in exercises/practice/food-chain/.meta/examples/success-standard/src/FoodChain.hs apply the diff

-  where showLower a = let (h:t) = show a
-                      in (toLower h : t)
+  where showLower = map toLower . show

I'll go look for more failures now.

@ErikSchierboom
Copy link
Member

@MatthijsBlom @mx-ws I can't push to the branch, as it seems to be created without the ability to allow pushes.

@MatthijsBlom
Copy link
Contributor

@ErikSchierboom Are you able to rebase this PR?

@mx-ws
Copy link
Contributor Author

mx-ws commented May 2, 2023

@MatthijsBlom @mx-ws I can't push to the branch, as it seems to be created without the ability to allow pushes.

I don't have time to look into any issues until later this day, but I added you as a contributor to my repository, @ErikSchierboom .

@MatthijsBlom
Copy link
Contributor

@mx-ws There should be a tickbox like this one:

image

@mx-ws
Copy link
Contributor Author

mx-ws commented May 2, 2023

image
There is and it was already checked. I now unchecked and checked it again. Now there is this second checkmark on the right hand side. Can you push now? :D

@MatthijsBlom
Copy link
Contributor

MatthijsBlom commented May 2, 2023

@mx-ws Could you please add me as a contributor as well? Running the tests on my machine is taking very long; I'd rather run them here. I'm not a maintainer, so I cannot edit this PR.

@ErikSchierboom
Copy link
Member

I'll let @MatthijsBlom do the fixing, I don't know Haskell that well

@MatthijsBlom
Copy link
Contributor

MatthijsBlom commented May 2, 2023

Alright, checks are finally green.

  • 43eccdd replaces a non-exhaustive pattern match with map. Not exactly the same thing, but arguably more correct.
  • 8c30a3d I'm not entirely sure about, but at least the replacement function produces the same output for the given inputs.
  • 2b20375 replaces a non-exhaustive pattern match with head. The behavior is the same; the runtime error is different (but does not occur and should not matter anyway).
  • ddb41a4 replaces a non-exhaustive pattern match on a list of known length with a match on a tuple.

@ErikSchierboom feel free to merge, I guess.

@ErikSchierboom ErikSchierboom merged commit c587770 into exercism:main May 2, 2023
@ErikSchierboom
Copy link
Member

Thanks @MatthijsBlom!

@MatthijsBlom
Copy link
Contributor

And @mx-ws of course.

@ErikSchierboom
Copy link
Member

Yep, and @mx-ws !

@denismaier
Copy link

Thank you all!

@denismaier
Copy link

I've just tested with the exercise (pangram) where I initially ran into this issue. Now, I'm getting a different error:

grafik

I'm getting this error even after reverting the exercise!

@MatthijsBlom
Copy link
Contributor

Aye, there is a platform wide issue: forum thread.

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.

5 participants