Revert "Update to latest prettyprinter API (#2352)"

[pepeiborra]

This reverts commit 1eb133f.

Sorry for the revert @fendor but this change breaks compatibility with Stackage LTS 16 (GHC 8.8).

I considered adding CPP to support both the new and the old API, but it doesn't seem worth it given the little use in the ghcide codebase - it would be easier to drop the dependency.

[jneira]

Sorry for the revert @fendor but this change breaks compatibility with Stackage LTS 16 (GHC 8.8).

out of curiosity, what is the concrete consequence?, circleci stack builds with ghc-8/lts-16.31 are fine afaics

[pepeiborra]

Sorry for the revert @fendor but this change breaks compatibility with Stackage LTS 16 (GHC 8.8).

out of curiosity, what is the concrete consequence?, circleci stack builds with ghc-8/lts-16.31 are fine afaics

That's because we don't build the entire LTS 16 in CircleCI.

Enterprise codebases like ours build a large subset of the snapshot, which means bumping one dependency has a cascading effect. In this case dhall depends on prettyprinter, so to bump the latter I have to bump the former. But dhall has many transitive dependencies including haskeline which is a builtin package and cannot be bumped in our codebase without creating problems.

[michaelpj]

If compatibility with specific stack snapshots like this is important then we need to enforce it and probably include it in our update policy. Otherwise this is just going to happen again next time someone bumps a dep.

[fendor]

Sorry for the revert @fendor

Don't be, thank you for taking care of it!

[jneira]

If compatibility with specific stack snapshots like this is important then we need to enforce it and probably include it in our update policy. Otherwise this is just going to happen again next time someone bumps a dep.

But we are using other extra-deps with newer versions than the used ones in the original lts: from aeson to lsp-types.
So it seems not all extra-deps-already-in-stackage are problematic and otoh some are absolutely needed for us. Not sure what criteria could we follow to enforce it 🤔

[jneira]

lgtm, maybe we should track its eventual inclusion to facilitate ghc-9.2 support (that was the reason to add this, no @fendor ?)

[fendor]

that was the reason to add this

No, the reason was I had warnings while building and thought I was being helpful 😄

[michaelpj]

Enterprise codebases like ours build a large subset of the snapshot, which means bumping one dependency has a cascading effect. In this case dhall depends on prettyprinter, so to bump the latter I have to bump the former. But dhall has many transitive dependencies including haskeline which is a builtin package and cannot be bumped in our codebase without creating problems.

Actually, I don't understand this. Why do you need to build your main codebase and HLS using the same stack snapshot, even? Can't you do an independent build of the HLS binaries, separate from your main codebase?

"All versions of packages we use must be compatible with the build plan for the actual FB codebase" seems like too stringent a requirement for us to follow...

[jneira]

"All versions of packages we use must be compatible with the build plan for the actual FB codebase" seems like too stringent a requirement for us to follow...

Well in this case we dont lose great things and if a new version of a dep is needed due a bug fix or performance improvement etc, probably everybody, including fb, will need or want it as well.

So not something to establish a general criteria but to consider in each case, imho.

[pepeiborra]

Actually, I don't understand this. Why do you need to build your main codebase and HLS using the same stack snapshot, even? Can't you do an independent build of the HLS binaries, separate from your main codebase?

We don't build HLS, we build ghcide and use it as a library.

"All versions of packages we use must be compatible with the build plan for the actual FB codebase" seems like too stringent a requirement for us to follow...

I am not asserting this as a requirement, our infra has support for source patches for dealing with this sort of thing. It's just that in this case it seems that relaxing the compatibility with prettyprinter versions is a good thing, and better than patching.