Make homebrew optional

[angerman]

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]

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

[andreasabel]

/usr/homebrew/opt/

That meant /opt/homebrew/opt/, I suppose.

[vshabanov]

Yes, I fixed it.

[andreasabel]

I suppose default: true is for backward compatibility.

However, in general one would not need any extra-lib-dirs, because they can be communicated by pkg-config.
See how CI does it:

text-icu/.github/workflows/cabal-mac-win.yml

Lines 53 to 59 in be8f0e8

	# Let `pkg-config` find the ICU libs.
	# Also test whether it actually works, and print some debug information.
	- name: Set up pkg-config for the ICU library (macOS)
	if: ${{ runner.os == 'macOS' }}
	run: |
	PKG_CONFIG_PATH=$(brew --prefix)/opt/icu4c/lib/pkgconfig
	echo "PKG_CONFIG_PATH=${PKG_CONFIG_PATH}" >> "${GITHUB_ENV}"

[vshabanov]

Not everyone has pkg-config. In HsOpenSSL for example there are 3 flags: homebrew-openssl, macports-openssl, and use-pkg-config. There's also a non-trivial Setup.hs that tries to auto-detect settings. Probably text-icu needs the same.

[angerman]

Yes, I wish we could jsut rely on pkg-config, but relying on that is also such a mess, especailly on windows, where you might end up with Strawberry Perls pkg-config mess all of a sudden.

And yes, default: true feels morally wrong to me, but I didn't want to break anyone.

[andreasabel]

Well, I agree that if one does not have pkg-config set up, one does not get good errors from cabal why build is failing, and setting it up is a manual process one would not want to have to do...
So, maybe cabal complaining about non-existent library paths is the lesser evil.

[andreasabel]

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

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]

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]

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]

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]

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

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]

To be clear I didn't imply any judgement ☺️

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.