-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Some fixes for multi component stuff #3686
Conversation
bf4419b
to
021ecf4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, looks good to me!
case unitIdString (homeUnitId_ df) of | ||
-- cabal uses main for the unit id of all executable packages | ||
-- This makes multi-component sessions confused about what | ||
-- options to use for that component. | ||
-- Solution: hash the options and use that as part of the unit id | ||
-- This works because there won't be any dependencies on the | ||
-- executable unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think haskell/cabal#8726 fixes this, right? If so, I think we should include that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any way of knowing what cabal version we're working with? In the future will we support MHU only with a sufficiently new cabal? If so then perhaps this needs a note saying when we can throw it away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we will likely have support for older cabal versions as well, but it will be slightly hacky.
Is this a cabal bug? Presumably this means that e.g. a multi-repl with multiple executables will go wrong also? |
Ah I see, Fendor says it might be fixed. |
case unitIdString (homeUnitId_ df) of | ||
-- cabal uses main for the unit id of all executable packages | ||
-- This makes multi-component sessions confused about what | ||
-- options to use for that component. | ||
-- Solution: hash the options and use that as part of the unit id | ||
-- This works because there won't be any dependencies on the | ||
-- executable unit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any way of knowing what cabal version we're working with? In the future will we support MHU only with a sufficiently new cabal? If so then perhaps this needs a note saying when we can throw it away.
021ecf4
to
60703f0
Compare
60703f0
to
3123d59
Compare
Test failures seem maybe genuine? |
I don't think so, different tests fail every time |
08624ea
to
fda63c5
Compare
IDK, it looks like it's almost always the |
Yeah, you are probably right, I see consistent failures in the 9.4+ jobs, I was looking at the <=9.2 jobs and saw random failures earlier. I will investigate. |
…fused multi component sessions. Solution: include the hash of the options in the unit id when the unit id is called "main". Fixes #3513
fda63c5
to
8ea0fee
Compare
Turned out to be a missing fallthrough case that snuck in while cherry picking the commit 🤦 |
I've had my eye on this PR for a while since #3513 has been really annoying me, and for the record, it now works perfectly for my projects, where previous iterations before today would hang at initialisation. Thanks for the fix! |
main
as the unit id of all executable packages. This confused multi component sessions.Solution: include the hash of the options in the unit id when the unit id is called "main".
Fixes #3513