Skip to content

Chore: bump to lts-20.0 #3642

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

Closed
wants to merge 8 commits into from
Closed

Chore: bump to lts-20.0 #3642

wants to merge 8 commits into from

Conversation

mitchellwrosen
Copy link
Member

Overview

Redoing #3631, this time landing on GHC 9.2.4

@aryairani
Copy link
Contributor

Superficial HLS test seems okay. I do have to open the package containing the definitions I want to jump to before I can jump to them, which seems kind of pointless, but I'm not sure it didn't already work that way.

aryairani
aryairani previously approved these changes Dec 1, 2022
@aryairani
Copy link
Contributor

aryairani commented Dec 1, 2022

Reminder to create corresponding PR for enlil.

stack.yaml Outdated
- lsp-1.5.0.0
- lsp-types-1.5.0.0
- text-rope-0.2@sha256:53b9b4cef0b278b9c591cd4ca76543acacf64c9d1bfbc06d0d9a88960446d9a7,2087
commit: 3b26f1e6c279e9bb5cd052c34c852aa8f8cfa17e
Copy link
Contributor

@aryairani aryairani Dec 1, 2022

Choose a reason for hiding this comment

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

Sanity check: did you ensure color renders properly in a Windows terminal, or what commit is this?

Copy link
Contributor

@aryairani aryairani Dec 1, 2022

Choose a reason for hiding this comment

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

I'm testing it, but it will take a while.

Copy link
Member Author

Choose a reason for hiding this comment

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

This commit is morally the same: the PR that we were pointing at was rebased by the author atop the latest main so it continues to build. The PR still isn't merged :/

Copy link
Contributor

Choose a reason for hiding this comment

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

image

This is what I get; let me check again on trunk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Sorry for making you do all of that, but I understand now.

I too went looking for "what is commit d6c2643b0d5c19be7e440615c6f84d603d4bc648?" when trying to fix compiler errors with haskeline due to the bump, and I saw it in the history of the open PR we were depending on:

image

But I got the history wrong. Looks like the author was not just force-pushing over their PR to keep it atop the main branch, they changed the implementation, too.

image

Meanwhile @ChrisPenner (maybe independently) discovered that the original approach was correct, so pointed us at d6c2643b0d5c19be7e440615c6f84d603d4bc648.

So I believe the correct move is to grab haskeline master (since it builds), and revert haskell/haskeline@f827f10 on top of it (which is how d6c2643b0d5c19be7e440615c6f84d603d4bc648 came to be).

Copy link
Member Author

Choose a reason for hiding this comment

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

@aryairani I've updated this dependency. Now it's pointing at our unisonweb/haskeline fork, branch lts-20.0. The contents of the branch are just judah/haskeline HEAD, plus this commit, which is equivalent to the previous commit we were using (although now rebased on top of judah/haskeline HEAD, because the old commit didn't build against lts-20.0).

Some confusion: we have a unison branch (the default branch) in our unisonweb/haskeline fork, 7 commits ahead and 13 commits behind aryairiani/haskeline. I first tried merging judah/haskeline HEAD into this, but got a ton of conflicts. Is this branch just dead code at this point?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this branch just dead code at this point?

I would guess so.

Copy link
Contributor

Choose a reason for hiding this comment

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

screenshot of 20dacc9 courtesy of @tarikozkanli
image

@@ -166,7 +166,7 @@ authorSuggestion :: P.Pretty P.ColorText
authorSuggestion =
P.newline
<> P.lines
[ P.wrap "📜 🪶 You might want to set up your author information next.",
[ P.wrap "📜 You might want to set up your author information next.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use the 🖊️ character anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't tested on GHC 9.2.4. On GHC 9.0.2, no, which was weird. But I didn't investigate too closely because I thought putting two emojis in a row here was a mistake, anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I try putting it back?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno, I guess it's supposed to be writing on paper, not sure if the paper alone makes sense. @rlmark ? :)

@aryairani aryairani self-requested a review December 1, 2022 12:38
@aryairani aryairani dismissed their stale review December 1, 2022 19:41

new regression discovered

@ChrisPenner
Copy link
Contributor

ChrisPenner commented Dec 2, 2022

Has anything changed on the points listed from the last time I attempted moving to 9.2?

#3304

Don't want to upgrade to (yet) 9.2.3 because :
Wingman isn't yet supported via HLS on 9.2*, which means no case-splitting functionality. However there's active work to split off that functionality to a new plugin which would be supported for 9.2.3: haskell/haskell-language-server#2971

It's okay if we decide that's not a priority, but we should consider it I think 😄, I use the case-splitting feature very often.

@mitchellwrosen
Copy link
Member Author

It's okay if we decide that's not a priority, but we should consider it I think smile, I use the case-splitting feature very often.

I think it's mostly up to you then whether we upgrade now or later. Personally I've never gotten that feature to work with any hls.

@aryairani
Copy link
Contributor

Current status is:

  • windows prompt is still broken (cc @mitchellwrosen)
  • waiting on either case-splitting Wingman support, or for us to declare we don't care enough to keep holding off (cc @ChrisPenner)

@ChrisPenner
Copy link
Contributor

ChrisPenner commented Jan 3, 2023

The windows prompt seems important,

as for wingman, I'll miss the pattern splitting, but it wouldn't kill be me to go without it, copilot can help fill that gap I suppose.

I tried out the GHC 9.2.4 build and I was hoping it would speed up GHC compile times on M1 more than it actually did, so that's a bummer, but still probably good to keep relatively up to date with GHC 👍🏼

@ceedubs
Copy link
Contributor

ceedubs commented Jan 4, 2023

For what it's worth, I'm hoping that these memory changes in GHC 9.2 will help address some memory issues in Unison Cloud related to the GHC runtime using way more memory than the allotted heap.

@mitchellwrosen
Copy link
Member Author

This PR is old and dead, which is a shame. It's sometimes quite a lot of work to bump the LTS, and we are on quite an old GHC at this point, so it would be good to upgrade.

I do sometimes wonder if it's worth it for our small team to support Windows. If even basic libraries like haskeline just don't work out of the box, and it's on us to debug and figure out - well, maybe the Haskell ecosystem just isn't good enough for cross-platform stuff 😓

@ChrisPenner
Copy link
Contributor

I agree that we should find a way to upgrade soon, the longer we wait the further behind we get. It's likely worth a week of off-sprint time to figure out the haskeline thing at this point.

@aryairani
Copy link
Contributor

We do want to support Windows, but yeah agree that the situation is pretty ridiculous.

@aryairani
Copy link
Contributor

@mitchellwrosen Ok, when I build this PR with

- github: judah/haskeline
  commit: d6c2643b0d5c19be7e440615c6f84d603d4bc648

(same as on trunk), then UCM works with allow-newer: true; without it, I'm just missing the bumped bounds that the newer versions of Haskeline have.

In the dependencies for haskeline-0.8.0.0(-terminfo):
  base-4.16.3.0 from Stack configuration does not match >=4.9 && <4.15 (latest matching version is 4.14.3.0)
  bytestring-0.11.3.1 from Stack configuration does not match >=0.9 && <0.11 (latest matching version is 0.10.12.1)
needed due to unison-cli-0.0.0 -> haskeline-0.8.0.0

I confirmed that rebasing our preferred Happy Windows commit onto the latest Haskeline, and building that with the new LTS is no longer happy on Windows, so I guess something else broke in Haskeline that breaks the proposed-and-then-retracted fix from @. this was what you had done too that also didn't work back in December (20dacc9)

I guess we should just use the commit we know, put it on a branch to satisfy #3945, and bump the bounds ourself?

@aryairani aryairani reopened this May 19, 2023
@aryairani
Copy link
Contributor

superseded by #4008

@aryairani aryairani closed this May 20, 2023
@mitchellwrosen mitchellwrosen deleted the 22-11-21-bump-lts branch May 15, 2024 22:19
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