Use hpack not cabal #626
Use hpack not cabal #626
Conversation
Adds *.cabal to gitignore Generates package.yaml using [`hpack-convert`](https://github.com/yamadapc/hpack-convert)
libs/api-client/package.yaml
Outdated
| author: Wire Swiss GmbH | ||
| maintainer: Wire Swiss GmbH <backend@wire.com> | ||
| license: AGPL-3 | ||
| default-extensions: |
There was a problem hiding this comment.
Let's use https://github.com/sol/hpack#defaults for this section.
There was a problem hiding this comment.
I didn't make any manual edits since that would take a fair amount more time than just using the automated tool; do you think it's worthwhile to dig through and clean everything up? I suppose if we don't do it now it's unlikely to happen. Should I look into somehow adding some sort of yaml inheritance for these sort of settings or do we not care enough to look into that?
There was a problem hiding this comment.
You don't need YAML inheritance, it's a built-in hpack feature. You can create a file in the repo:
-- package-defaults.yaml
default-extensions:
- ...
And then in every package.yaml file you say on the top level:
defaults:
path: package-defaults.yaml
There was a problem hiding this comment.
Oh, or maybe it should be local: package-defaults.yaml; I'm not sure.
There was a problem hiding this comment.
@ChrisPenner if you don't to I'm happy to do this.
There was a problem hiding this comment.
After taking a peak; yup this is a good idea 😄
I added it.
hpack-convert didn't recognize the custom setup cabal flag
If we warn on it, it will fail due to -Werror I believe these errors come from using Cabal2; but I have little evidence to support it.
|
It doesn't seem that Regardless, approved. |
| @@ -1,2 +1 @@ | |||
| turn:localhost:3478 | |||
| turn:localhost:3479 | |||
| turn:localhost:3478 | |||
There was a problem hiding this comment.
Very strange, no idea what happened here; great catch!
There was a problem hiding this comment.
Out of curiosity: when running brig's integration tests, one of the tests creates that 2nd entry... but it should always fallback to the original after, something funky happened there
| ghc-options: | ||
| - -j | ||
| - -Wno-redundant-constraints | ||
| - -Werror |
There was a problem hiding this comment.
Can you motivate the change (removal) of ghc options here?
There was a problem hiding this comment.
As Artyom mentioned, several of the more pervasive options are now globally applied via module-defaults.yaml
|
-O2 went into module-defaults.yaml. |
Ah I see, thanks, I missed that in the rather large diff. 👍 |
| @@ -39,7 +39,6 @@ services/nginz/src | |||
| services/.env | |||
| tools/api-simulations/mailboxes.json | |||
| tools/api-simulations/reports | |||
| .DS_Store | |||
There was a problem hiding this comment.
This was duplicated
| @@ -61,6 +60,8 @@ spar.integration-aws.yaml | |||
| integration-aws.yaml | |||
| DOCKER_ID* | |||
| swagger-ui | |||
| services/spar/spar.cabal | |||
There was a problem hiding this comment.
Now covered by *.cabal
|
Any have other global GHC options they'd like added before I send this through? |
There was a problem hiding this comment.
@ChrisPenner please merge this if you're comfortable doing so. We can always tweak the ghc opts later.
Adds *.cabal to gitignore
Generated package.yaml using
hpack-convertUnfortunately I'm getting some compilation problems related to file path case sensitivity :'(
There's a note on this here;
haskell/cabal#4739
I've tried adding
--ghc-options=-optP-Wno-nonportable-include-path; which removes the warning but the actual gcc linking fails later on.Not entirely sure why this is breaking as a result of the switch; seems mostly to be affecting Proxy; so maybe I'll switch the cabal file back for just that one?
Definitely open to thoughts on this PR; is this something we should bother doing at all?