haskell / text-icu

This package provides the Haskell Data.Text.ICU library, for performing complex manipulation of Unicode text.
BSD 2-Clause "Simplified" License
47 stars 41 forks source link

Make homebrew optional #99

Closed angerman closed 6 months ago

angerman commented 6 months ago

Force-adding homebrew paths to text-icu, causes compilation and link errors on systems that have a mixture of homebrew and other system library providers. There currently is no way to prohibit text-icu from not force-injecting homebrew paths.

This change adds a flag to allow disabling the forced addition of homebrew paths to the package description.

Morally I'd argue that this flag should be off by default, I do however see this as a harder case to argue, and the current on-by-default to be less contentious.

vshabanov commented 6 months ago

Thank you. It's now on Hackage, version 0.8.0.5

andreasabel commented 6 months ago

@angerman @vshabanov CI failures should be investigated before merging: https://github.com/haskell/text-icu/actions/runs/8563984229/job/23517908260?pr=99

Continuous integration is precisely here to detect problems in PRs.

vshabanov commented 6 months ago

Yes, I was thinking that the change is benign enough and didn't wait for CI to end (for some reason CI didn't run automatically and I needed to run it manually), but it turned out that flag must go after extra-* and tested-with, otherwise they're ignored.

angerman commented 6 months ago

it turned out that flag must go after extra-* and tested-with, otherwise they're ignored.

This also caught me by surprise. @vshabanov do you consider this a cabal bug?

andreasabel commented 6 months ago

I have stumbled more than once over the weird ordering constraints in .cabal files. By experience, I have learnt the following rule: fields first, then sections. flag is a section, and extra-* and tested-with are fields. (Fields are introduced by a colon, sections have none (except for their inner fields).) I think this limitation of the cabal parser is silly...

angerman commented 6 months ago

I have stumbled more than once over the weird ordering constraints in .cabal files. By experience, I have learnt the following rule: fields first, then sections. flag is a section, and extra-* and tested-with are fields. (Fields are introduced by a colon, sections have none (except for their inner fields).) I think this limitation of the cabal parser is silly...

Yes, I've flagged this with @andreabedini to please look at.

I find the colon, non-colon distinction in cabal files also fairly annoying, as I get it wrong more often than not (and did not come up with some rationalization like you did @andreasabel 🙄). I consider this "seemingly" arbitrary syntax a fairly annoying UX failure.

andreabedini commented 6 months ago

@andreasabel is correct: top-level fields after the first section are ignored.

Sections where introduced later so the current parser (one of the many layers) adapts a package description in the legacy format into the new schema. This is done by parsing all the top-lelve fields first and adding the sections later. It is not pretty.

The parser outputs a warning though:

Warning: simple-package.cabal:12:1: Ignoring trailing fields after sections:
"test"

Did the warning get lost in the weeds?

Edit: it did https://github.com/haskell/text-icu/actions/runs/8563984229/job/23517908260?pr=99#step:18:24

vshabanov commented 5 months ago

I only saw "Ignoring trailing fields after sections:" when I uploaded to Hackage, but I had no idea what it meant (googling didn't give any useful answers), and inferred that flag must come after extra-* after spending some time randomly reordering sections/fields.

Since I mostly just copy cabal fields/sections from other .cabal files, I had no idea that cabal has a field/section distinction and that : changes the meaning (I thought it was just optional if the value or indented section is on the next line).

I think the error message should be improved with a hint "Please, put fields (name: value) before sections (name without colon)". But ideally the parser should work in any order.

andreabedini commented 5 months ago

To be clear I didn't imply any judgement :relaxed:

I think the error message should be improved with a hint "Please, put fields (name: value) before sections (name without colon)". But ideally the parser should work in any order.

I wholeheartedly agree.