cabal icon indicating copy to clipboard operation
cabal copied to clipboard

rm-old-base: remove ifdefs for pre-4.13 bases

Open NadiaYvette opened this issue 1 year ago • 8 comments

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.

  • [x] 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).

NadiaYvette avatar Jun 09 '24 15:06 NadiaYvette

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.

geekosaur avatar Jun 09 '24 16:06 geekosaur

@fgaz Can you have another look when you have time?

andreabedini avatar Jun 20 '24 15:06 andreabedini

@fgaz @ulysses4ever @Mikolaj @Kleidukos could you give a review?

andreabedini avatar Jun 27 '24 02:06 andreabedini

@fgaz @ulysses4ever @Mikolaj @Kleidukos could you give a review?

Thank you so much Andrea! That's a lot of LOC you put into the fixups!

NadiaYvette avatar Jun 27 '24 02:06 NadiaYvette

Yes! Less CPP makes happier tooling

Kleidukos avatar Jun 27 '24 07:06 Kleidukos

I see "base >= 4.11" in Cabal/Cabal.cabal. Can we update it, too? Probably in a separate PR, because it would modify the interface. It would also need a little changelog file stating what it does.

Mikolaj avatar Jun 27 '24 07:06 Mikolaj

Maybe something went wrong with the rebase I had changed the bounds the cabal files in my commit acffcd425c2ec7064495550be8f026fac0bc76a6.

Edit: I pushed my last commit (9ce421302920bdbbee8a6e1a51f866328c9ea7d1) to https://github.com/andreabedini/cabal/tree/rm-old-base-001

andreabedini avatar Jun 27 '24 09:06 andreabedini

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

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?

Mikolaj avatar Jun 27 '24 17:06 Mikolaj

@fgaz @Mikolaj I restored the lower bounds on base.

andreabedini avatar Jul 12 '24 10:07 andreabedini

@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?

ulysses4ever avatar Jul 23 '24 18:07 ulysses4ever

FWIW everything except the import is marked Outdated, suggesting they've already been fixed. I'm validating locally with the import removed. (ETA: build fails without the import.)

geekosaur avatar Jul 23 '24 19:07 geekosaur

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.)

geekosaur avatar Jul 23 '24 20:07 geekosaur

@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?

All opened conversations (3 at this time) are still unresolved

fgaz avatar Jul 23 '24 20:07 fgaz

[27 of 51] Compiling Distribution.Solver.Types.ConstraintSource ( src/Distribution/Solver/Types/ConstraintSource.hs, /home/allbery/Sources/cabal/dist-newstyle/build/x86_64-linux/ghc-8.8.4/cabal-install-solver-3.13.0.0/build/Distribution/Solver/Types/ConstraintSource.o )

src/Distribution/Solver/Types/ConstraintSource.hs:48:3: error:
    parse error on input ‘-- | The source of the constraint is not specified.’
   |
48 |   -- | The source of the constraint is not specified.
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

geekosaur avatar Jul 23 '24 20:07 geekosaur

@fgaz, I think we've dealt with the remaining issues, could you confirm?

geekosaur avatar Jul 24 '24 02:07 geekosaur

@geekosaur @fgaz @ulysses4ever Thank you for the help. I am afraid some changes kept getting lost in the rebases :-/

andreabedini avatar Jul 24 '24 09:07 andreabedini