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 multi unit argument syntax #3462

Merged
merged 13 commits into from
Nov 23, 2023
Merged

Add support for multi unit argument syntax #3462

merged 13 commits into from
Nov 23, 2023

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Jan 23, 2023

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

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

@@ -0,0 +1,2 @@
packages: a b c
multi-repl: True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wat?

Copy link
Collaborator

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

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

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

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

Choose a reason for hiding this comment

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

@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

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

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.

@mpickering
Copy link
Contributor

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
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
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!

@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
20 tasks
@wz1000 wz1000 requested a review from ozkutuk as a code owner November 22, 2023 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants