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.

Sign up for GitHub

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

Jump to bottom

Future compat for liftA2 being exported from Prelude #8223

Merged
merged 2 commits into from
Jun 17, 2022
Merged

Future compat for liftA2 being exported from Prelude #8223

merged 2 commits into from
Jun 17, 2022

Conversation

googleson78
Copy link
Contributor

googleson78
googleson78 commented Jun 16, 2022
View reviewed changes
Cabal-syntax/src/Distribution/Fields/ParseResult.hs Outdated
@@ -17,15 +17,15 @@ module Distribution.Fields.ParseResult (
withoutWarnings,
) where

import Distribution.Compat.Prelude
import Distribution.Compat.Prelude hiding (Applicative(..))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, this module will not compile on base < 4.10.0. Not sure what to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(other than introduce more CPP)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think CPP is the only resort.
(BTW it would be nice to mention such situation in the migration guide)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to the migration guide, but I'm not sure I (if it's possible at all) to provide instructions that cover most of the cases, it feels like this might be very varying in what the fix is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to push a fix for this but it's not trivial for me to compile with base <4.10 right now, could you take a look as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to befine on CI.

@googleson78
Copy link
Contributor Author

Seeing as cabal is using a custom prelude, I'm not sure if there's some more elegant way to do this?

@Bodigrim
Copy link
Collaborator

@Mikolaj could you please take a look?
Since Cabal is a boot library, we cannot proceed with https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8449 until this is merged.

ulysses4ever
ulysses4ever approved these changes Jun 17, 2022
View reviewed changes
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.

Approving on the grounds of: the change is eminent and the CI is clear. I don’t know if it’s possible to avoid CPP but I trust the author’s judgement.

Mikolaj
Mikolaj approved these changes Jun 17, 2022
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. If this fails after the intended liftA2 change, we can always fix some more. Similarly, we will probably need to fix cabal-install and other packages in this repo, so from my perspective this doesn't need to be right on the first try.

@Mikolaj Mikolaj added attention: needs-backport 3.8 merge me Tell Mergify Bot to merge labels Jun 17, 2022
@mergify mergify bot merged commit 8963596 into haskell:master Jun 17, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Jun 17, 2022

@googleson78: may I ask you, at your leisure, after the firefighting is over, to create a separate PR with a tiny changelog file for this PR? Thank you.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 19, 2022

@mergify backport 3.8

@mergify mergify bot mentioned this pull request Jul 19, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2022

backport 3.8

✅ Backports have been created

mergify bot added a commit that referenced this pull request Jul 19, 2022
@mergify
Merge pull request #8304 from haskell/mergify/bp/3.8/pr-8223
efa4e1a 
Future compat for liftA2 being exported from Prelude (backport #8223)
@ulysses4ever ulysses4ever mentioned this pull request Feb 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bodigrim Bodigrim Bodigrim left review comments

@Mikolaj Mikolaj Mikolaj approved these changes

@ulysses4ever ulysses4ever ulysses4ever approved these changes

Assignees
No one assigned
Labels
attention: needs-review merge me Tell Mergify Bot to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@googleson78 @Bodigrim @Mikolaj @ulysses4ever