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

Split off unison-runtime package #5308

Merged
merged 4 commits into from
Aug 30, 2024
Merged

Split off unison-runtime package #5308

merged 4 commits into from
Aug 30, 2024

Conversation

ChrisPenner
Copy link
Contributor

Overview

Split off the runtime for two reasons:

  1. parser-typechecker is still a behemoth with too much stuff
  2. Moving unison-runtime out means I can tweak runtime things without needing to rebuild ALL of parser-typechecker in -profile -O2; which takes ages.

While I was at it, I added -Wunused-packages and cleaned that up. There were a lot.

Implementation notes

  • Split off runtime stuff
  • Had to move a single definition (codebaseToCodeLookup)from Codebase.hs into Codebase.Execute.hs
  • -Wunused-packages doesn't love the "shared" top-level package dependencies, so for packages with tests I switched to specifying dependencies for lib and test separately; which is annoying, but a very small price to pay ultimately.

Test coverage

Discovered that the Rsa.hs tests weren't actually included in any test suite for some reason haha. Added that.

@ChrisPenner ChrisPenner force-pushed the cp/split-off-runtime branch from 19ca084 to 494be0b Compare August 29, 2024 22:10
@ChrisPenner ChrisPenner marked this pull request as ready for review August 29, 2024 22:11
@ChrisPenner ChrisPenner requested review from mitchellwrosen and aryairani and removed request for mitchellwrosen August 29, 2024 22:11
module Unison.Codebase.Execute where
module Unison.Codebase.Execute
( execute,
codebaseToCodeLookup,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one definition I had to move

@@ -418,12 +415,6 @@ typeLookupForDependencies codebase s = do
<|> Map.lookup r (TL.effectDecls tl) $> ()
)

toCodeLookup :: (MonadIO m) => Codebase m Symbol Parser.Ann -> CL.CodeLookup Symbol m Parser.Ann
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to Codebase.Execute.codebaseToCodeLookup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copy-pasta'd these test utils from parser-typechecker

@aryairani
Copy link
Contributor

I poked around a little but couldn't see why things were going wrong nix-wise. Any ideas? cc @sellout

@aryairani
Copy link
Contributor

aryairani commented Aug 30, 2024

On a second run the error is different or more obvious:

       > Configuring benchmark 'relation' for unison-util-relation-0.0.0..
       > building
       > Preprocessing benchmark 'relation' for unison-util-relation-0.0.0..
       > Building benchmark 'relation' for unison-util-relation-0.0.0..
       > [1 of 1] Compiling Main             ( benchmarks/relation/Main.hs, dist/build/relation/relation-tmp/Main.o )
       >
       > benchmarks/relation/Main.hs:8:1: error:
       >     Could not load module ‘Unison.Prelude’
       >     It is a member of the hidden package ‘unison-prelude-0.0.0’.
       >     Perhaps you need to add ‘unison-prelude’ to the build-depends in your .cabal file.
       >     Use -v (or `:set -v` in ghci) to see a list of the files searched for.
       >   |
       > 8 | import Unison.Prelude
       >   | ^^^^^^^^^^^^^^^^^^^^^
       For full logs, run 'nix log /nix/store/kisnjl7rqgpmijdi195h8zhkdqpchcs2-unison-util-relation-bench-relation-0.0.0.drv'.

Maybe one dependency deletion too many, @ChrisPenner?

Though I wonder why stack didn't complain? Could be something about hpack getting run or not getting run and/or an updated file not getting checked in?

@ChrisPenner
Copy link
Contributor Author

ChrisPenner commented Aug 30, 2024

@aryairani Oh interesting; I'm guessing we just don't build benchmarks in CI (which if we're not running them in CI is probably fine, but of course not great if we can accidentally break their builds)

You need to stack build --bench to build benchmarks usually, but that also runs the benchmarks.

@ChrisPenner ChrisPenner force-pushed the cp/split-off-runtime branch from 494be0b to 9cdff27 Compare August 30, 2024 16:30
@aryairani
Copy link
Contributor

You need to stack build --bench to build benchmarks usually, but that also runs the benchmarks.

Need stack build --bench --no-run-benchmarks 😅

@aryairani aryairani merged commit b17640a into trunk Aug 30, 2024
34 of 35 checks passed
@aryairani aryairani deleted the cp/split-off-runtime branch August 30, 2024 21:59
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.

2 participants