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

Update to resolver 20.11 #72

Merged
merged 8 commits into from
May 2, 2023
Merged

Update to resolver 20.11 #72

merged 8 commits into from
May 2, 2023

Conversation

mx-ws
Copy link
Contributor

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

Hi, in the context of this pull request I also updated the resolver to 20.11 here.

In the process I changed the expected results of the tests. Notice how I changed the run-tests.sh script to work on my macOS machine.

I'm not at home and therefore do not have access to my Linux machine. Therefore feedback whether the tests also succeed on Linux would be appreciated.

@mx-ws mx-ws requested a review from a team as a code owner March 18, 2023 15:55
@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 18, 2023
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Nice work! Would the old solutions work with the new resolver?

@mx-ws
Copy link
Contributor Author

mx-ws commented Apr 5, 2023

Hi, excuse my inactivity, I'm a bit busy at the moment.

The errors are associated with the fact that the sed command apparently leaves the absolute path to the project in the output (/Users/mxw/... in my case).

Nice work! Would the old solutions work with the new resolver?

I'm not sure if I understand. Which old solutions do you mean exactly?

@ErikSchierboom
Copy link
Member

I'm not sure if I understand. Which old solutions do you mean exactly?

Solutions that have already been submitted by students.

@mx-ws
Copy link
Contributor Author

mx-ws commented Apr 23, 2023

Solutions that have already been submitted by students.

I can tell you about my three or so solutions that still worked, but otherwise I don't feel confident in answering this question as I'm not an expert about the ghc.
As a reminder, the version step here is 9.0.2 -> 9.2.5 of ghc or 19.27 -> 20.11 of resolver.

@hesselink
Copy link
Contributor

@mx-ws As you suggested in exercism/haskell#1158 (comment), I've made a few updates to your PR: main...hesselink:haskell-test-runner:lts-20. This passes all tests, and it also updates the docker tag to GHC 9.2 (which also ensures there's an M1 image) and sets the lts to the latest (20.18, using GHC 9.2.7). This is also the 'recommended' version in ghcup.

@mx-ws
Copy link
Contributor Author

mx-ws commented Apr 24, 2023

Thanks for helping out! Notice how we already had a discussion at exercism/haskell#1158 about to which resolver to upgrade. You are right in that ghcup's recommendation has switched from 9.2.5 to 9.2.7 since then.
I'm pretty neutral about this, I just think we should use the same version in both repositories.

I guess if no one intervenes or comments otherwise I'm going to set resolver 20.18 over at exercism/haskell#1158 and pull your changes here at this repository.

@hesselink
Copy link
Contributor

Yeah, I saw that discussion, but since the consideration was what ghcup recommends, I figured switching to the latest point release was best (include the most fixes, without much risk of breaking). Your path forward sounds great, thanks!

@ErikSchierboom
Copy link
Member

I'm not sure if the sed thing works, the CI run says:

"sed: can't read : No such file or directory"

for each test

@hesselink
Copy link
Contributor

That's weird, I'll have another look. I tested it locally on mac and in docker, and both seemed to work.

@hesselink
Copy link
Contributor

I've pushed a fix to my branch. Turns out I accidentally committed the mac fix to the sed command in a later, unrelated commit. I've removed it now. @mx-ws Why did you decide to remove your code to have it work on both mac and linux?

@hesselink
Copy link
Contributor

@ErikSchierboom We could consider failing the scripts on any errors. Something like this: 4928c04. If you're open to this, I can make a proper PR.

@ErikSchierboom
Copy link
Member

If you're open to this, I can make a proper PR.

Yes please!

@hesselink
Copy link
Contributor

Great! I created #77

@ErikSchierboom
Copy link
Member

Great, merged that PR. Could you rebase?

@mx-ws
Copy link
Contributor Author

mx-ws commented Apr 26, 2023

I've pushed a fix to my branch. Turns out I accidentally committed the mac fix to the sed command in a later, unrelated commit. I've removed it now. @mx-ws Why did you decide to remove your code to have it work on both mac and linux?

  1. My understanding is that the haskell-test-runner project will not run on a user's machine (and therefore it's not really an issue if it can't run on M1 in the same way as the haskell repository).
  2. I could not seem to get the run-tests.sh script working for both mac and linux at the same time.

So I chose to rollback the corresponding changes and stop putting time into this particular issue. For running the tests locally I just started using docker.

@ErikSchierboom
Copy link
Member

CI seems to be failing

@hesselink
Copy link
Contributor

hesselink commented Apr 28, 2023

It looks like it's missing my fix from #72 (comment). @mx-ws Could you cherry-pick the latest commit from main...hesselink:haskell-test-runner:lts-20 as well?

On the positive side, the 'fail on error' is working 😄

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

Thanks @mx-ws!

iHiD added a commit that referenced this pull request May 2, 2023
iHiD added a commit that referenced this pull request May 2, 2023
iHiD added a commit that referenced this pull request May 2, 2023
iHiD added a commit that referenced this pull request May 2, 2023
ErikSchierboom added a commit that referenced this pull request May 3, 2023
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.

3 participants