Use text package to decode UTF-8 instead of doing it ourselves

[Bodigrim]

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
  • Is the change significant? If so, remember to add significance: significant in the changelog file.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

[ulysses4ever]

In principle, this looks like the right direction of travel. But I'm wary of the CPP and dependence on an internal text module.

[Bodigrim]

CPP can be avoided if everyone is happy to require text >= 2.0. This would require regenerating bootstrap build plan for GHC 9.2.8, but otherwise I hardly see any drawbacks. Shall we?

[geekosaur]

With all the stuff we already need for bootstrapping, and 9.2.8 being disrecommended due to bugs, I think we should go ahead and bump it if it avoids CPP.

[Bodigrim]

One does not simply update a bootstrap plan. Raised #11464 to discuss.

[geekosaur]

And for what it's worth, I don't see any other text version checks in the codebase.

[ulysses4ever]

Since it's a change to the library, not the tool, I'd be more careful. In particular, I'd only require text>=2.0 after the last GHC shipping text<2.0 falls out of our support window. Thoughts @Mikolaj @mpickering @ffaf1?

[Bodigrim]

In particular, I'd only require text>=2.0 after the last GHC shipping text<2.0 falls out of our support window.

Could you elaborate why? What's the relationship?

[geekosaur]

All I can think of is someone building such an older ghc from source, since that would require the newer text to be a bootlib.

[Bodigrim]

All I can think of is someone building such an older ghc from source, since that would require the newer text to be a bootlib.

If they are building an old GHC from source, surely they are building an old Cabal boot library from the GHC source tree, so all is fine.

(For the record, not a hill I'm going to die on; I'm genuinely curious)

[Mikolaj]

This would require regenerating bootstrap build plan for GHC 9.2.8

We can drop the plan for GHC 9.2.8, even because we already have more plans than we normally offer.

Re requiring new text vs CPP, that's a tough one, in particular, because CPP (and non-strict deps) make it much harder to test all paths in CI. Text 2.0 is 4 years old, so maybe it's time to switch? That obviously needs a major version, asking major lib users ahead of time (especially @mpilgrem and the illustrious distro maintainers) and I may still be missing some old promise or custom around this that @ulysses4ever remembers.

[ulysses4ever]

I don't have any reasoning, just things I came to believe over time, overhearing various talks and reading various ancient threads on this bug tracker (no links, sorry). I'd ask Oleg hadn't he explicitly asked not to ping him on ongoing cabal matters.

For distributors, apart from Mike, I think it'd be prudent to ping @juhp for feedback. Maybe @maralorn could also opine. (Apologies in advance for a rather vague ping.) The question is whether you guys see any issue in Cabal-the-library requiring text>=2.

[Bodigrim]

Text 2.0 is 4 years old, so maybe it's time to switch? That obviously needs a major version, asking major lib users ahead of time (especially @mpilgrem and the illustrious distro maintainers) and I may still be missing some old promise or custom around this that @ulysses4ever remembers.

Stack already requires text>=2.1.3.

text-2.0 has been a boot package since GHC 9.4 and pretty much all major Linux distributives are already shipping it or newer.

[maralorn]

Yeah, I this would be no problem on Nixos.

[Bodigrim]

@ulysses4ever @Mikolaj @geekosaur is this enough of assurances to go ahead with this or do you have further reservations?

[ulysses4ever]

With respect to text 2, yes, thank you!

Is there no way to avoid depending on an internal module?

[Bodigrim]

Is there no way to avoid depending on an internal module?

It's possible with decodeUtf8Chunk and decodeUtf8More, but very awkward, to a degree that I'd rather revert changes to Distribution.Parsec.FieldLineStream if it proves to be a sticking point. What do you think?

[mpilgrem]

Stack would be unaffected:

• Stack builds (by default) with the version of Cabal that is the boot package of the specified GHC version
• Stack's own reliance on Cabal is on a single version - most often the version in the most recent Stackage LTS Haskell snapshot (unless there is a compelling reason to jump forwards).

[Bodigrim]

It's possible with decodeUtf8Chunk and decodeUtf8More, but very awkward, to a degree that I'd rather revert changes to Distribution.Parsec.FieldLineStream if it proves to be a sticking point. What do you think?

@ulysses4ever @Mikolaj @geekosaur @ffaf1?

(While my preference is to keep changes, I will not be grossly offended if you ask me to revert. I'd rather move forward one way or another then drain myself on a protracted dicussion about a relatively minor change)

[ulysses4ever]

I don't have an informed opinion and will trust your judgement w.r.t. internals.

Otherwise, great job!

[geekosaur]

Artem was the one concerned about it; I was just making a suggestion, but in thinking about it the whole Text thing probably means a different internal function to access the resulting Text directly to make it a ByteString.

So probably just go with it as is until we have a chance to redesign the APIs to use Text like they should have in the first place.

[geekosaur]

One thing that occurs to me: you used the wrong template, this is an internal change that shouldn't be exposed I think?

[geekosaur]

Oh, I'm wrong because of the Distribution.Utils.String/Distribution.Utils.ShortText changes. So it'd be nice if you could fill out the rest of the template.

[Bodigrim]

@geekosaur is there anything specific you’d like me to fill out? Up to my understanding, I ticked the required checkboxes (and added a changelog entry).

[geekosaur]

Mmm, I'd be happier with a test that verified that it does the same thing, but I think that requires copying the old implementation into a test (or generating a golden test with the old one and verifying with the new). I also kinda read the "if necessary" on the docs checkbox to mean just check it off if it's not necessary.