Skip to content

porting hls-refactor to ghc-9.12 #4543

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 5, 2025
Merged

Conversation

GuillaumedeVolpiano
Copy link
Collaborator

These changes allow the hls-refactor plugin to compile and pass the tests with ghc-9.12

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, amazing work!
I have honestly not reviewed in-depth. I will rely on the test-suite to catch any mistakes :)

@fendor fendor requested review from jhrcek, wz1000 and soulomoon April 3, 2025 16:54
@fendor fendor added the status: needs review This PR is ready for review label Apr 3, 2025
@fendor
Copy link
Collaborator

fendor commented Apr 3, 2025

The tests will not run on CI, you need to change this line: https://github.com/haskell/haskell-language-server/blob/master/.github/workflows/test.yml#L137

Very difficult to find, apologies!

Also, just making sure, you can ignore the pre-commit error in CI.

@GuillaumedeVolpiano
Copy link
Collaborator Author

GuillaumedeVolpiano commented Apr 3, 2025

Changed. Hadn't even thought to look there. Hopefully they will run smoothly. Thanks for pointing it out.

@GuillaumedeVolpiano
Copy link
Collaborator Author

GuillaumedeVolpiano commented Apr 3, 2025

Let's make sure this is fine, but I just wanted to note that as a consequence, rename and splice built smoothly on my end, they'll just need unmasking afterwards. Unless you want me to add it here. gadt will need some love, on the other hand.

@fendor
Copy link
Collaborator

fendor commented Apr 3, 2025

It is fine to enable more plugins immediately, spares us a separate CI run :)

You have to accept the changes to the golden files, this is just there so we are sure to catch the changes!

@GuillaumedeVolpiano
Copy link
Collaborator Author

OK. Hold a bit then, because I see that splice is currently <ghc-9.10. I need to check it with ghc-9.10 too

@fendor
Copy link
Collaborator

fendor commented Apr 3, 2025

No hurry, I will be here tomorrow as well :D CI will take a while anyway.

@GuillaumedeVolpiano
Copy link
Collaborator Author

Tests pass on my side. Let's see how the CI goes.

@GuillaumedeVolpiano
Copy link
Collaborator Author

I might need help understanding these failing ghcide tests. They don't seem to relate to any of my changes…

@fendor
Copy link
Collaborator

fendor commented Apr 3, 2025

The windows CI failures are spurious and will be resolved by simply rerunning CI (which I will take care of). The 9.12. ubuntu failure is genuine, as you are enabling plugins, you need to accept the test changes.

@GuillaumedeVolpiano
Copy link
Collaborator Author

So basically I need to patch the test file as indicated by the results of the test, do I understand this right? Sorry if it's a dumb question, it's my first time running into these.

@fendor
Copy link
Collaborator

fendor commented Apr 3, 2025

Yes, this should do the trick: https://github.com/haskell/haskell-language-server/blob/master/test/functional/ConfigSchema.hs#L20

Assuming, you run the test with ghc-9.12.2. Also, not a dumb question, maybe we can add some explanations to the test failure to make this easier to discover.

@GuillaumedeVolpiano GuillaumedeVolpiano force-pushed the master branch 2 times, most recently from 39bdf50 to 1402e38 Compare April 4, 2025 05:23
@GuillaumedeVolpiano
Copy link
Collaborator Author

Now that was easy. Thanks for the pointer

@GuillaumedeVolpiano
Copy link
Collaborator Author

GuillaumedeVolpiano commented Apr 4, 2025

Well, seems I actually failed a few tests.will get back to work on it. Sorry about that

@fendor
Copy link
Collaborator

fendor commented Apr 4, 2025

Don't worry, we are very happy you are working on it :)

@GuillaumedeVolpiano
Copy link
Collaborator Author

Thanks. Maybe worth adding a WIP tag to it for now?

@fendor fendor self-requested a review April 4, 2025 09:33
@fendor fendor marked this pull request as draft April 4, 2025 09:33
@fendor
Copy link
Collaborator

fendor commented Apr 4, 2025

Like this?

@GuillaumedeVolpiano
Copy link
Collaborator Author

Yes thanks!

@GuillaumedeVolpiano GuillaumedeVolpiano force-pushed the master branch 9 times, most recently from 6bf3945 to 24d9657 Compare April 4, 2025 17:12
@GuillaumedeVolpiano
Copy link
Collaborator Author

GuillaumedeVolpiano commented Apr 4, 2025

Only two tests left. Of course, the first four were easy because they're actually just changes in the way ghc report things.

@GuillaumedeVolpiano
Copy link
Collaborator Author

Five downs, one to go

@GuillaumedeVolpiano
Copy link
Collaborator Author

GuillaumedeVolpiano commented Apr 5, 2025

@fendor : We're good to go

@GuillaumedeVolpiano GuillaumedeVolpiano marked this pull request as ready for review April 5, 2025 09:38
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Amazing work, only two small comments, then we will merge!

Thank you for this great contribution :)

@GuillaumedeVolpiano
Copy link
Collaborator Author

I'm going to re-disable splice. I'll look at it and gadt in a second time.

@GuillaumedeVolpiano GuillaumedeVolpiano force-pushed the master branch 4 times, most recently from 3ab95d9 to c4f71a1 Compare April 5, 2025 12:14
@GuillaumedeVolpiano
Copy link
Collaborator Author

@fendor : can you have a look at the failures? I think they are of the spurious kind, but if they're not, I'll also disable rename for now and add it to my todo list ;)

@fendor
Copy link
Collaborator

fendor commented Apr 5, 2025

Yeah, they look spurious. I will rerun the CI job once the others have finished running.

@fendor fendor merged commit 7d5a8e4 into haskell:master Apr 5, 2025
40 of 41 checks passed
@fendor
Copy link
Collaborator

fendor commented Apr 5, 2025

Awesome! There will be likely another release in the next two weeks, our users will be happy to see the refactor plugin enabled for GHC 9.12.2 :)

@GuillaumedeVolpiano
Copy link
Collaborator Author

GuillaumedeVolpiano commented Apr 5, 2025

Thanks for accepting my PR. Will do my best to add a few other plugins in the meantime :p

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants