Skip to content

Conversation

@ChrisPenner
Copy link
Member

@ChrisPenner ChrisPenner commented Jun 27, 2024

Overview

Upgrades to LTS 22.26; GHC 9.6.5

Recommended you update your local GHCup to: HLS 2.9.0.0 and Stack 2.15.5

Does someone who knows nix want to update the flake as well? It seems ghc 965 isn't being found, I'm sure it's an easy fix for someone who knows how it works 🙃

Implementation notes

  • Updates the LTS
  • Removes redundant applicative updates
  • Replaces use of NanoID lib with secure crypto instead, just cuz the NanoID lib has bitrotted
  • Move from x509-* to crypton-x509-* libs since HVR quit Haskell
  • Bump fuzzyfind
  • Removes a few custom pinnings now that they've been added to the snapshot

Interesting/controversial decisions

Not really

Test coverage

Existing tests

Loose ends

  • Should I fix the JWT lib deprecations now?

@aryairani
Copy link
Contributor

Does someone who knows nix want to update the flake as well? It seems ghc 965 isn't being found, I'm sure it's an easy fix for someone who knows how it works 🙃

cc @sellout

Comment on lines +42 to +43
with:
stack-version: 2.15.5
Copy link
Contributor

@aryairani aryairani Jun 27, 2024

Choose a reason for hiding this comment

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

we can also change the default on unisonweb/actions/stack/install if we like that better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I figured it was a bit of a chicken-and-egg thing. TBH I kind of like specifying here because it's easy to see what it's using and easy to bump again in the future 😄

I'm glad you added it as an arg for that step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I like specifying it in this project too; I just wish we didn't have to specify it 4 times in this project, but maybe that's fine. Replace All.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can read JSON in GH workflows, and also in Nix (I don’t know if there are other places we need versions).

So, we could write a dependency-versions.json file or something that the flake can read into its versions attrSet, and that workflows can read as well. I’ve been wanting to extract it from the flake anyway.

@aryairani
Copy link
Contributor

Nice!

On testing, could you smoke test that the new file watch code works locally

@sellout
Copy link
Contributor

sellout commented Jun 27, 2024

Does someone who knows nix want to update the flake as well? It seems ghc 965 isn't being found, I'm sure it's an easy fix for someone who knows how it works 🙃

I would normally agree with you, but haskell.nix (which is still used a lot by our flake) doesn’t support much in the way of GHC. But I’ll give it a shot.

Copy link
Member Author

@ChrisPenner ChrisPenner Jun 27, 2024

Choose a reason for hiding this comment

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

Contributed by neduard for keeping the cabal build up to date. I haven't tested this myself.

flake.nix Outdated
(system: let
versions = {
ghc = "928";
ghc = "965";
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 change doesn't actually work.

-- This function does not support parameters, and is mostly useful for executing DDL and migrations.
executeStatements :: Connection -> Text -> IO ()
executeStatements conn@(Connection _ _ (Sqlite.Connection database)) sql = do
executeStatements conn@(Connection _ _ (Sqlite.Connection database _tempNameCounter)) sql = do
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 type just changed.

@ChrisPenner
Copy link
Member Author

Ah, thanks Greg, yeah we can wait then. I'm getting pretty close with project roots now anyways :)

@neduard
Copy link
Contributor

neduard commented Jul 12, 2024

Hey @ChrisPenner , I was trying out this branch and I needed some changes from trunk so I had a go at merging it in & fixing the conflicts: neduard#3 - figured you'll probably encounter the same so it might be worth sharing in case it saves you a bit of time. As before feel free to either cherrypick or simply use as a reference.

Something worth pointing out is that I had to revert #5192 since numerals requires text (>=0.11 && <1.3) but LTS 22.26 has text 2.0.2 ( cc @aryairani ), but other than that the changes were minimal 🚀

@aryairani
Copy link
Contributor

aryairani commented Jul 13, 2024

Hey @ChrisPenner , I was trying out this branch and I needed some changes from trunk so I had a go at merging it in & fixing the conflicts: neduard#3 - figured you'll probably encounter the same so it might be worth sharing in case it saves you a bit of time. As before feel free to either cherrypick or simply use as a reference.

Something worth pointing out is that I had to revert #5192 since numerals requires text (>=0.11 && <1.3) but LTS 22.26 has text 2.0.2 ( cc @aryairani ), but other than that the changes were minimal 🚀

@neduard I used allow-newer-deps: [numerals] for our stack build, because https://github.com/roelvandijk/numerals wanted containers (<6), so it probably accepted text-2.0.2 due to that flag, before the error was even on my radar. The author of numerals invites a PR with updated bounds, but I didn't have the time/knowledge to do it; but maybe you'd want to take a stab at it since you're more familiar with cabal than I am.

Otherwise, is there a similar directive to cabal to ignore the bounds on numerals (only)? I don't really want to revert #5192.

@neduard
Copy link
Contributor

neduard commented Jul 13, 2024

That's a much better shout @aryairani , thanks! I was so focused on getting it to build that I didn't think of other ways of solving it. I've raised the PR upstream and in the meantime worked around it by adding the following to contrib/cabal.project

allow-newer:
  numerals:text,
  numerals:containers

@aryairani
Copy link
Contributor

That's a much better shout @aryairani , thanks! I was so focused on getting it to build that I didn't think of other ways of solving it. I've raised the PR upstream and in the meantime worked around it by adding the following to contrib/cabal.project

allow-newer:
  numerals:text,
  numerals:containers

Okay great — neduard#3 is merged though, is that still where to look?

@sellout
Copy link
Contributor

sellout commented Jul 15, 2024

worked around it by adding the following to contrib/cabal.project

Thanks, @neduard. We really need to get a Cabal GitHub check into CI (even if it’s not required). It should generally be trivial to keep it in sync, but easy to overlook without a reminder.

@aryairani
Copy link
Contributor

worked around it by adding the following to contrib/cabal.project

Thanks, @neduard. We really need to get a Cabal GitHub check into CI (even if it’s not required). It should generally be trivial to keep it in sync, but easy to overlook without a reminder.

Yeah, the Cabal Github check is missing a) because we didn't have anyone to maintain the .cabal file (no longer true) and b) because it would have doubled the resources needed to run CI (not fully true anymore, but not a non-issue either). I guess we could run cabal build on linux only; wouldn't have run tests. We should have some sort of build cache.

@sellout
Copy link
Contributor

sellout commented Jul 15, 2024

Yeah, the Cabal Github check is missing a) because we didn't have anyone to maintain the .cabal file (no longer true) and b) because it would have doubled the resources needed to run CI (not fully true anymore, but not a non-issue either). I guess we could run cabal build on linux only; wouldn't have run tests. We should have some sort of build cache.

I opened #5225 so we could avoid conflating this discussion (my apologies for starting it here), and so we have somewhere to point when it comes up again.

@sellout sellout mentioned this pull request Jul 15, 2024
@neduard
Copy link
Contributor

neduard commented Jul 15, 2024

@ChrisPenner @aryairani #5224 ready for merge into this one (or directly into trunk if you so wish) 😊 - please have a look at the description as there's a couple of things I'm unsure regarding updating a few transcripts and the jit tests potentially being cached but still requiring a couple of small updates - hoping you'll know more!

@aryairani aryairani merged commit 6ed06f3 into trunk Jul 15, 2024
@aryairani aryairani deleted the cp/ghc-upgrade branch July 15, 2024 20:48
sellout added a commit to sellout/unison that referenced this pull request Jul 15, 2024
Reviewing the last merge into unisonweb#5142, I noticed some duplicated and out-
of-date information. This brings things up-to-date and slightly reduces
the duplication.

- bumped Ormolu used by ci.yaml from 0.5.2.0 to 0.7.2.0, to match
  flake.nix
- removed Markdown that claimed ci.yaml was using Ormolu 0.5.0.1
- moved description from `base-codebase` in Markdown to comment on
 `runtime_tests_codebase` in ci.yaml (and updated it to refer to
   builtin-tests/interpreter-tests.md instead of builtin-tests/base.md)
- removed `unison_src_test_results` as it’s no longer managed as a
  single variable
- moved other comments from Markdown to ci.yaml
- added Markdown recommending to look in ci.yaml for specifics
- rearranges the order of vars in ci.yaml to match the order they were
  presented in Markdown

One thing I wasn’t sure how to map over: Markdown claims Racket 8.7 is
used in CI, but ci.yaml doesn’t mention any Racket version.
sellout added a commit to sellout/unison that referenced this pull request Jul 19, 2024
With unisonweb#5142, Ormolu was upgraded from 0.5.2.0 to 0.7.2.0. This formats
the codebase to avoid spurious formatting comingled in other commits.

Almost all of the changes are simply wrapping single constraints in parens, like
```diff
-hashBranch :: forall m. Monad m => Branch m -> m BranchHash
+hashBranch :: forall m. (Monad m) => Branch m -> m BranchHash
```

There is also some reordering of language pragmas and imports,
indentation correction (some of which gets precedence wrong), and switching
some Haddock from `-- ^` to `-- |` .
sellout added a commit to sellout/unison that referenced this pull request Aug 6, 2025
sellout added a commit to sellout/unison that referenced this pull request Aug 6, 2025
sellout added a commit to sellout/unison that referenced this pull request Aug 6, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
sellout added a commit to sellout/unison that referenced this pull request Aug 6, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
sellout added a commit to sellout/unison that referenced this pull request Aug 6, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
sellout added a commit to sellout/unison that referenced this pull request Aug 6, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
sellout added a commit to sellout/unison that referenced this pull request Aug 6, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
sellout added a commit to sellout/unison that referenced this pull request Aug 7, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
sellout added a commit to sellout/unison that referenced this pull request Aug 7, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
sellout added a commit to sellout/unison that referenced this pull request Aug 8, 2025
This seems to have been broken for almost a year.

- Add missing packages to cabal.project
- Allow newer `numerals` deps, as in Stack (this was mentioned in
  unisonweb#5142 (comment)
  but never made it in)
- Disable `terminfo` flag for haskeline, as in Stack (eliminates a
  complaint about not finding libtinfo.so)
- Add a constraint on text-builder, since 1.0.0 has been released, but
  is incompatible

This also adds some comments and sorts the constraints list.
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