-
Notifications
You must be signed in to change notification settings - Fork 697
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
rm-old-base: remove ifdefs for pre-4.13 bases #10092
rm-old-base: remove ifdefs for pre-4.13 bases #10092
Conversation
You're supposed to pick a template based on whether your PR changes the user-visible or programming API. This one should not, so you would use template B. |
a09118a
to
442c7bd
Compare
442c7bd
to
f175323
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.
LGTM. Thank you so much! <3
@fgaz Can you have another look when you have time? |
e14ce68
to
9ce4213
Compare
9ce4213
to
5ce9664
Compare
@fgaz @ulysses4ever @Mikolaj @Kleidukos could you give a review? |
5ce9664
to
b8e89de
Compare
Thank you so much Andrea! That's a lot of LOC you put into the fixups! |
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.
It might be worth looking for more instances of FOURMOLU_DISABLE
and see if CPP has been removed; in general, CPP causes fourmolu to choke.
b8e89de
to
4ec4202
Compare
Yes! Less CPP makes happier tooling |
I see "base >= 4.11" in |
Maybe something went wrong with the rebase I had changed the bounds the cabal files in my commit acffcd4. Edit: I pushed my last commit (9ce4213) to https://github.com/andreabedini/cabal/tree/rm-old-base-001 |
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.
Some of my previous comments are still unresolved, I marked them as such.
Right here it's as it should be. That's rather disturbing. Was this mergify's rebase or your manual one? Can you recover from this corruption? |
1d2926a
to
47c0067
Compare
4.13 and older have fallen out of the support window. Hence this commit removes code only conditionally included for base 4.13 and older. Some occasional transitive removals were implied and done in this same commit.
13de64b
to
e4208ca
Compare
The change was likely an artifact of a rebase.
The #ifdefs being generated need to be kept here so that projects other than cabal can be built using older ghc versions and current cabal versions.
This needs to be included so running with older bases and ghcs can be done even while building cabal itself demands recent ghcs.
e4208ca
to
356e6f7
Compare
@fgaz you requested changes in this PR, and the issue, they claim, was fixed (I didn't check). Could you see if you could reevaluate it and maybe approve? |
FWIW everything except the |
I stand corrected; I misunderstood what was being requested, and removed the wrong thing. Testing again now. (ETA: builds with 9.6.6; trying with 8.8.4 locally now.) |
All opened conversations (3 at this time) are still unresolved |
|
It's not needed with our currently supported ghcs.
@fgaz, I think we've dealt with the remaining issues, could you confirm? |
@geekosaur @fgaz @ulysses4ever Thank you for the help. I am afraid some changes kept getting lost in the rebases :-/ |
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.
Looks good!
Template B: This PR does not modify behaviour or interface
4.13 and older have fallen out of the support window. Hence this commit removes code only conditionally included for base 4.13 and older. Some occasional transitive removals were implied and done in this same commit.