Skip to content

Add support for multi unit argument syntax#3462

Merged
mergify[bot] merged 13 commits intomasterfrom
wip/multi-unit
Nov 23, 2023
Merged

Add support for multi unit argument syntax#3462
mergify[bot] merged 13 commits intomasterfrom
wip/multi-unit

Conversation

@wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Jan 23, 2023 •
edited
Loading

Implements the argument parsing logic for multiple home units found in GHC 9.4: https://downloads.haskell.org/ghc/9.4.4/docs/users_guide/using.html#multiple-home-units

We now support arguments of the form

-unit @unitA -unit @unitB

where the response files unitA and unitB contain the actual list of arguments for that unit:

-this-unit-id a-0.1.0.0
-i
-isrc
A1
A2

Also refactor the session loader and simplify it.

Also adds error messages on GHC 9.4 if the units are not closed (#3422).

Depends on haskell/hie-bios#387

@wz1000 wz1000 changed the title Add support for the multi unit argument syntax Add support for multi unit argument syntax Jan 23, 2023
@mpickering
Copy link
Contributor

mpickering commented Jan 23, 2023

What happens if the closure property is violated? That seemed the main point of design on the issue?

pepeiborra
pepeiborra reviewed Jan 24, 2023
View reviewed changes
ghcide/test/data/multi-unit/cabal.project
@@ -0,0 +1,2 @@
packages: a b c
multi-repl: True
Copy link
Collaborator

@pepeiborra pepeiborra Jan 24, 2023

Choose a reason for hiding this comment

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

Wat?

Copy link
Collaborator

@fendor fendor Jan 24, 2023

Choose a reason for hiding this comment

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

Probably the new cabal feature that uses multiple home-units when loading them into a repl?

Copy link
Collaborator Author

@wz1000 wz1000 Jan 25, 2023

Choose a reason for hiding this comment

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

Yes, @fendor is right, I generated the -unit files by using a special branch of cabal with the new feature and then editing it a bit.

This cabal.project isn't necessary or even useful in this patch so I will remove it.

Copy link
Collaborator Author

@wz1000 wz1000 Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

With the cabal feature you can set up your hie.yaml to load your entire project including all components with this patch like so:

cradle:
  cabal:
    - path: "./"
      component: "all"

Copy link
Collaborator

@fendor fendor Jan 25, 2023

Choose a reason for hiding this comment

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

I am interested to know what branch and what @mpickering is planning there, and whether we still need the work from haskell/cabal#7500, or whether the changes done in that fork render cabal status command irrelevant?
Maybe we can briefly discuss that at the next meeting.

Copy link
Collaborator Author

@wz1000 wz1000 Jan 26, 2023

Choose a reason for hiding this comment

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

Here is the branch, it is still a WIP: https://github.com/mpickering/cabal/tree/wip/no-configure

@wz1000
Copy link
Collaborator Author

wz1000 commented Jan 25, 2023

What happens if the closure property is violated?

We get the same error we would get from GHC, which is not ideal and could probably be improved.

I checked if recovery is possible once more components are loaded but it seems it is not, so that needs a bit more work.

@mpickering
Copy link
Contributor

mpickering commented Jan 27, 2023

What happens if the closure property is violated?

We get the same error we would get from GHC, which is not ideal and could probably be improved.

I checked if recovery is possible once more components are loaded but it seems it is not, so that needs a bit more work.

I think that things should carry on working for the unit you have currently loaded but just ignore any files which would be in the new component (until the require components are loaded in).

@mpickering
Copy link
Contributor

mpickering commented Mar 17, 2023

I tested again and the closure check seems to be working properly now.

But I think the question I raised about what should happen if the closure check is violated is still unresolved.

@fendor fendor mentioned this pull request Jun 11, 2023
Open
@mpickering
Copy link
Contributor

mpickering commented Jun 19, 2023

Branch appears to no longer build due to a version mismatch of hie-bios. What's the plan @wz1000 ?

@fendor
Copy link
Collaborator

fendor commented Jun 19, 2023

I think, the plan looks roughly as follows:

  • For cabal versions that support multi-repl, we remember the filepaths that caused components to be loaded.
    On subsequent calls to runCradle, we pass in all previous filepaths, s.t. hie-bios invokes cabal repl fp1 fp2 fp3 and cabal then calculates the dependency closure. That works fine, afaict, except for potential issues with the maximal cli parameter length. This branch is currently using an extension to hie-bios allowing to pass in multiple filepaths, but it is neither merged nor has a PR yet.
  • For other cabal versions, we might also be able to hack something together. After the closure check is violated, we know exactly which unit-ids are missing. So, we can look them up in plan.json, then look up build-info.json files and report these back. This will not work for certain project setups, e.g. the dependency chain of hie-compat -> hiedb -> ghcide, since hiedb is not part of this project, we can't find the compilation options, afaict.

@fendor fendor mentioned this pull request Jul 31, 2023
Closed
8 tasks
@wz1000 wz1000 force-pushed the wip/multi-unit branch 2 times, most recently from cd9cc3e to ed8f16e Compare August 7, 2023 10:16
fendor
fendor approved these changes Nov 21, 2023
View reviewed changes
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, let's go!

@fendor fendor mentioned this pull request Nov 21, 2023
Closed
@wz1000 wz1000 added the merge me Label to trigger pull request merge label Nov 22, 2023
@michaelpj michaelpj mentioned this pull request Nov 22, 2023
Merged
20 tasks
@wz1000 wz1000 requested a review from ozkutuk as a code owner November 22, 2023 22:19
@wz1000 wz1000 requested a review from joyfulmantis as a code owner November 22, 2023 22:49
@mergify mergify bot merged commit 7db6215 into master Nov 23, 2023
@wz1000 wz1000 mentioned this pull request Nov 24, 2023
Closed
@fendor fendor mentioned this pull request Dec 2, 2023
Closed
@chenrui333 chenrui333 mentioned this pull request Jan 17, 2024
Merged
@fendor fendor mentioned this pull request Apr 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pepeiborra pepeiborra pepeiborra left review comments

@fendor fendor fendor approved these changes

@berberman berberman Awaiting requested review from berberman

@michaelpj michaelpj Awaiting requested review from michaelpj michaelpj is a code owner

@guibou guibou Awaiting requested review from guibou

@Anton-Latukha Anton-Latukha Awaiting requested review from Anton-Latukha

@Ailrun Ailrun Awaiting requested review from Ailrun

@isovector isovector Awaiting requested review from isovector

@santiweight santiweight Awaiting requested review from santiweight

@OliverMadine OliverMadine Awaiting requested review from OliverMadine

@georgefst georgefst Awaiting requested review from georgefst

@eddiemundo eddiemundo Awaiting requested review from eddiemundo

@July541 July541 Awaiting requested review from July541 July541 is a code owner

@drsooch drsooch Awaiting requested review from drsooch

@ozkutuk ozkutuk Awaiting requested review from ozkutuk ozkutuk is a code owner

@joyfulmantis joyfulmantis Awaiting requested review from joyfulmantis joyfulmantis is a code owner

Assignees

No one assigned

Labels

merge me Label to trigger pull request merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wz1000 @mpickering @fendor @pepeiborra