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

Add support for loading multiple components into one repl session #8726

Merged
merged 4 commits into from
May 28, 2023

Conversation

mpickering
Copy link
Collaborator

There are several parts to this patch which are logically distinct but work together to support the overal goal of starting a GHCi session with multiple packages loaded at once.

  1. When a user writes "cabal repl " then if the user is using a compiler > ghc-9.4.* then we will attempt to start a multi-session which loads the selected targets into one multi-package session of GHC. 1a. The closure property states that in order to load components p and q into
    the same session that if p depends on z and z depends on q
    then z must also be loaded into the session.
    1b. Only inplace packages are able to be loaded into a multi session (if a component
    z exists then it is already made into an inplace package by
    cabal). Therefore cabal has already engineered that there is source
    code locally available for all packages which we will want to load
    into a session.

  2. It is necessary to modify ./Setup configure to allow users to configure a package without having previously built the dependency. Instead, we promise to the configure phase that we will have built it by the time we build the package. This allows us to configure all the packages we intend to load into the repl without building any dependenices which we will load in the same session, because the promise is satisifed due to loading the package and it's dependency into one multi-session which ensures the dependency is built before it is needed.

    A user of ./Setup configure specifies a promised dependency by prepending a "+" to a normal dependency specification. For example:

   '--dependency=+cabal-install-solver=cabal-install-solver-3.9.0.0-inplace'

2a. The ./Setup repl command is modified to allow a user to defer
starting the repl and instead instruct the command to write the
necessary build flags to a file. The option is called
--repl-multi-file <FILEPATH>.

`cabal-install` then invokes this command for each component which
will populate the session and starts a multi-session with all the
arguments together.
  1. The solver is unmodified, the solver is given the repl targets and creates a build plan as before. After the solver is completed then in setRootTargets and pruneInstallPlan we modify the install plan to enforce the closure property and mark which dependencies need to be promised.

    • Mark the current components as BuildInPlaceOnly InMemory, which indicates to the compiler that it is to be built in a GHC multi-session.
    • Augment the component repl targets to indicate that components required by the closure property (in addition to normal targets) will be loaded into the repl.
    • Modify the dependency edges in compLibDependencies to indicate which dependencies are the promised ones (which is precisely components which are BuildInPlaceOnly InMemory build styles). This is the field which is eventually used to populate the --dependency argument to ./Setup configure.

Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@mpickering
Copy link
Collaborator Author

Tests and documentation are coming, here to test against CI.

@mpickering mpickering force-pushed the wip/no-configure branch 2 times, most recently from bd9c69d to 3b02302 Compare February 7, 2023 23:12
@mpickering
Copy link
Collaborator Author

CI seems broken currently:

  ghcup: /usr/local/.ghcup/cache/ghcup-0.0.7.yaml: setModificationTime:setFileTimes: permission denied (Operation not permitted)

@Mikolaj
Copy link
Member

Mikolaj commented Feb 8, 2023

Yes, thank you, CI was broken. Please kindly rebase. I hope we have hacked around that.

@mpickering mpickering force-pushed the wip/no-configure branch 2 times, most recently from d58a1f6 to 6b95a4a Compare February 9, 2023 15:20
@Mikolaj
Copy link
Member

Mikolaj commented Feb 9, 2023

Actually, this time, the third CI fix will really fix your PR's run. Let me rebase myself...

@Mikolaj
Copy link
Member

Mikolaj commented Feb 9, 2023

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Feb 9, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Feb 9, 2023

rebase

✅ Branch has been successfully rebased

@mpickering mpickering force-pushed the wip/no-configure branch 5 times, most recently from 6b95a4a to 743dcb3 Compare February 16, 2023 17:50
@mpickering
Copy link
Collaborator Author

This should be passing CI and ready for review now @Mikolaj

I am writing a blog post to describe this feature which might be helpful to read as well. I will post a link once it is finished.

@mpickering
Copy link
Collaborator Author

@ulysses4ever
Copy link
Collaborator

Very unfortunate :-( The linker warning looks like what was reported on ghc-devs recently. But I'm not sure if this is the reason for the failure.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

The only error I can find in this log is

2023-02-24T11:55:18.7344280Z Resolving dependencies...
2023-02-24T11:55:19.4365280Z Error: cabal: Could not resolve dependencies:
2023-02-24T11:55:19.4469500Z [__0] trying: hackage-repo-tool-0.1.1.3 (user goal)
2023-02-24T11:55:19.4474820Z [__1] trying: unix-2.8.0.0/installed-2.8.0.0 (dependency of hackage-repo-tool)
2023-02-24T11:55:19.4477990Z [__2] trying: hackage-security-0.6.2.3 (dependency of hackage-repo-tool)
2023-02-24T11:55:19.4479130Z [__3] trying: hackage-security:+lukko
2023-02-24T11:55:19.4480030Z [__4] next goal: lukko (dependency of hackage-security +lukko)
2023-02-24T11:55:19.4481050Z [__4] rejecting: lukko-0.1.1.3 (conflict: unix =>
2023-02-24T11:55:19.4483550Z base==4.18.0.0/installed-4.18.0.0, lukko => base>=4.5 && <4.18)
2023-02-24T11:55:19.4487260Z [__4] rejecting: lukko-0.1.1.2 (conflict: unix =>
2023-02-24T11:55:19.4488390Z base==4.18.0.0/installed-4.18.0.0, lukko => base>=4.5 && <4.15)
2023-02-24T11:55:19.4489700Z [__4] rejecting: lukko-0.1.1.1, lukko-0.1.1, lukko-0.1 (conflict: unix =>
2023-02-24T11:55:19.4492340Z base==4.18.0.0/installed-4.18.0.0, lukko => base>=4.5 && <4.14)
2023-02-24T11:55:19.4493740Z [__4] fail (backjumping, conflict set: hackage-security, lukko, unix,
2023-02-24T11:55:19.4494550Z hackage-security:lukko)
2023-02-24T11:55:19.4495290Z After searching the rest of the dependency tree exhaustively, these were the
2023-02-24T11:55:19.4495980Z goals I've had most trouble fulfilling: base, hackage-security,
2023-02-24T11:55:19.4496490Z hackage-repo-tool, unix, lukko, hackage-security:lukko

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

This may or may not fix it: haskellari/lukko#33

If that's it, for now I'd not worry and review and even merge with this failure.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

But, the other failures are not about GHC 9.6, so I'd worry about those.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

But there is no long. It just says "This job failed". Doh.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

Which probably means this is a GHA outage. Restarting the job may help.

@fgaz fgaz force-pushed the wip/no-configure branch from 4e33a89 to 49ff9dc Compare May 25, 2023 13:09
@fgaz
Copy link
Member

fgaz commented May 25, 2023

All done. I ended up implementing the "good" fix.

Now we need a maintainer to go over my fixup commits, then I can squash them and finally merge this

@fgaz fgaz marked this pull request as ready for review May 25, 2023 13:53
@Mikolaj
Copy link
Member

Mikolaj commented May 25, 2023

The fix indeed looks very civilized, though I got lost reading the code due to how long the replAction functions is. I have low confidence in my assesment, but LGTM.

@fgaz fgaz force-pushed the wip/no-configure branch from 49ff9dc to 13cf185 Compare May 25, 2023 20:03
@fgaz fgaz added the merge me Tell Mergify Bot to merge label May 25, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 27, 2023
When there are multiple home units we need to be able to distinguish
between different executables. The way to do this is to pass the
`-this-unit-id` flag when compiling the executable.

Unfortunately this was broken before ghc-9.2 so there is a guard to
check that the compiler version is old enough. It's only critical we do
this from GHC-9.4 (when multiple home units was first supported) but
there's no harm in always passing this option.
The ./Setup repl command is modified to allow a user to defer
starting the repl and instead instruct the command to write the
necessary build flags to a file. The option is called
--repl-multi-file <FILEPATH>.
It is necessary to modify `./Setup configure` to allow users to
configure a package *without* having previously built the dependency.
Instead, we promise to the configure phase that we will have built it
by the time we build the package. This allows us to configure all the
packages we intend to load into the repl without building any
dependenices which we will load in the same session, because the
promise is satisifed due to loading the package and it's dependency
into one multi-session which ensures the dependency is built before
it is needed.

A user of ./Setup configure specifies a promised dependency by
using the "--promised-dependency" flag with a normal dependency specification. For example:

```
   '--promised-dependency=cabal-install-solver=cabal-install-solver-3.9.0.0-inplace'
```
There are several parts to this patch which are logically distinct but
work together to support the overal goal of starting a GHCi session with
multiple packages loaded at once.

1. When a user writes "cabal repl <target>" then if the user is using a
   compiler > ghc-9.4.* then we will attempt to start a multi-session
   which loads the selected targets into one multi-package session of
   GHC.
1a. The closure property states that in order to load components `p` and `q` into
    the same session that if `p` depends on `z` and `z` depends on `q`
    then `z` must also be loaded into the session.
1b. Only inplace packages are able to be loaded into a multi session (if a component
    `z` exists then it is already made into an inplace package by
    cabal). Therefore cabal has already engineered that there is source
    code locally available for all packages which we will want to load
    into a session.

2. The solver is unmodified, the solver is given the repl targets and
   creates a build plan as before. After the solver is completed then in
   `setRootTargets` and `pruneInstallPlan` we modify the install plan to
   enforce the closure property and mark which dependencies need to be
   promised.

   * Mark the current components as `BuildInPlaceOnly InMemory`, which
     indicates to the compiler that it is to be built in a GHC
     multi-session.
   * Augment the component repl targets to indicate that components
     required by the closure property (in addition to normal targets)
     will be loaded into the repl.
   * Modify the dependency edges in `compLibDependencies` to indicate
     which dependencies are the promised ones (which is precisely
     components which are `BuildInPlaceOnly InMemory` build styles).
     This is the field which is eventually used to populate the
     `--dependency` argument to `./Setup configure`.

Fixes haskell#8491
@fgaz fgaz force-pushed the wip/no-configure branch from 13cf185 to e61b658 Compare May 28, 2023 08:49
@mergify mergify bot merged commit 94615d6 into haskell:master May 28, 2023
@ocharles
Copy link
Contributor

🎉 Great work everyone, this is gonna be mega!

@Mikolaj
Copy link
Member

Mikolaj commented May 29, 2023

Top notch PR and top notch teamwork. Thank you everyone.

github-merge-queue bot pushed a commit to IntersectMBO/ouroboros-consensus that referenced this pull request Jul 19, 2023
This adds a multi-repl-enabled (see
haskell/cabal#8726) cabal (using a recent commit
from `master`) to the shell.

When using GHC >=9.4, one can then run e.g.
```console
cabal-multi-repl repl ouroboros-consensus ouroboros-consensus-diffusion
```
to get a REPL with all components in `ouroboros-consensus` and
`ouroboros-consensus-diffusion` 🎉
`ghcid` also works, e.g.
```console
ghcid -c 'cabal-multi-repl repl ...'
```

Beware that this feature is still somewhat experimental, e.g.
`cabal-multi-repl repl ouroboros-consensus-cardano` (or also
`-protocol`) are stalling for me, and selecting individual components
involving sublibraries fails with this message:
```console
 $ cabal-multi-repl repl ouroboros-consensus:consensus-testlib ouroboros-consensus:consensus-test
Error:
    Dependency on unbuildable library 'consensus-testlib' from ouroboros-consensus
```

Still, this might already be useful, and should not have any
risks/maintenance burden.
TristanCacqueray added a commit to podenv/hspkgs that referenced this pull request Aug 20, 2023
This change pulls cabal HEAD for haskell/cabal#8726
through `pkgs.cabal-multi-repl` instead of `pkgs.cabal-install`.
TristanCacqueray added a commit to ButlerOS/haskell-butler that referenced this pull request Aug 20, 2023
TristanCacqueray added a commit to ButlerOS/haskell-butler that referenced this pull request Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: cmd/repl merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.