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

Fix multi-repl when only building some internal library targets #10841

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mpickering
Copy link
Collaborator

When combining together --dependency and --promised-dependency flags, we were using Map.union in the wrong place. If you had a dependency and promised-dependency from the same package (ie when using an internal library) then the promised dependency wouldn't be taken into account.

The fix is straightforward, don't use Map.union. First create a list of everything and then create a map using fromListWith.

Fixes #10775

Please read Github PR Conventions and then fill in one of these two templates.


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:


Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ulysses4ever
Copy link
Collaborator

@mpickering don't hesitate to put the needs-review label when you open a PR that you think is ready for review. (I know this may look a little excessive but pragmatically the label sends a Matrix notification that will make the PR more visible.)

Copy link
Collaborator

@MangoIV MangoIV left a comment

Choose a reason for hiding this comment

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

I don't have any say but here's my review anyway: looks good to me!

@Mikolaj
Copy link
Member

Mikolaj commented Mar 25, 2025

@MangoIV: thank you for the review. Please kindly accept the invite to the cabal, I've just sent you.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM

@Mikolaj
Copy link
Member

Mikolaj commented Mar 25, 2025

@mpickering: would you like to submit this for merging once you made the final tweaks?

@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

@mpickering: a humble ping?

When combining together --dependency and --promised-dependency flags, we
were using `Map.union` in the wrong place. If you had a dependency
and promised-dependency from the same package (ie when using an internal
library) then the promised dependency wouldn't be taken into account.

The fix is straightforward, don't use `Map.union`. First create a list
of everything and then create a map using `fromListWith`.

Fixes #10775
@mpickering mpickering added the merge me Tell Mergify Bot to merge label Mar 28, 2025
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-backport 3.14 attention: needs-review merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Error: Dependency on unbuildable library" with three internal libraries and --enable-multi-repl
4 participants