Skip to content

Conversation

@philderbeast
Copy link
Contributor

@philderbeast philderbeast commented Jan 20, 2024

Fixes #303. I bumped base to >=4.11, matched ghc-8.4.4 included packages' lower bounds, and fixed warnings including cabal check warnings except for the example.

This is the change to cabal.project (or the contents of cabal.project.local if you prefer) I used for testing:

git diff
diff --git a/cabal.project b/cabal.project
index 73f0f8e..8d0c7e6 100644
--- a/cabal.project
+++ b/cabal.project
@@ -15,3 +15,12 @@ package hackage-security
 -- packages: precompute-fileinfo
 
 -- constraints: hackage-security -lukko
+
+program-options
+  ghc-options: -Werror=incomplete-uni-patterns
+if impl(ghc >= 8.6)
+  program-options
+    ghc-options: -Werror=star-is-type
+if impl(ghc >= 8.10)
+  program-options
+    ghc-options: -Werror=unused-record-wildcards
\ No newline at end of file

Even though I much prefer -XStandaloneKindSignatures, we can't use that until ghc >= 8.10.

I cleaned up some unused {-# LANGUAGE CPP #-} pragmas but note that some of the packages set this extension in default-extensions or other-extensions:

$ grep -rwn --exclude-dir=".git" --include="*.cabal" . -e 'CPP'
./example-client/example-client.cabal:57:  other-extensions:    CPP
./hackage-security/hackage-security.cabal:174:                       CPP
./hackage-root-tool/hackage-root-tool.cabal:46:  other-extensions:    CPP, ScopedTypeVariables, RecordWildCards
./hackage-security-HTTP/hackage-security-HTTP.cabal:60:  other-extensions:    CPP

I've not tested this with cabal yet.

@philderbeast
Copy link
Contributor Author

philderbeast commented Jan 20, 2024

Warning

I got a ghc panic with ghc-8.4.3 building Cabal-syntax

$ cabal build all --enable-tests --enable-benchmark
...
Failed to build Cabal-syntax-3.10.2.0.
...
[ 29 of 137] Compiling Distribution.CabalSpecVersion ( src/Distribution/CabalSpecVersion.hs, dist/build/Distribution/CabalSpecVersion.o )
ghc: panic! (the 'impossible' happened)
  (GHC version 8.4.3 for x86_64-unknown-linux):
	Prelude.!!: index too large

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

Error: cabal: Failed to build Cabal-syntax-3.10.2.0 (which is required by
test:TestSuite from hackage-security-0.6.2.4, exe:example-client from
example-client-0.1.0.0 and others). See the build log above for details.

@Bodigrim
Copy link
Contributor

Bodigrim commented Jan 20, 2024

Yeah, that's the same issue as https://gitlab.haskell.org/ghc/ghc/-/issues/21905 and probably similar to https://gitlab.haskell.org/ghc/ghc/-/issues/15436, fixed in GHC 8.4.4, which never happened.

@philderbeast
Copy link
Contributor Author

Thanks @Bodigrim. No worries then as we're targeting ghc >= 8.4.4. I didn't go with base >=4.11.1 because that doesn't distinguish between GHC versions {8.4.2, 8.4.3, 8.4.4} and, from the 8.4.2 release notes, base doesn't look much different to base-4.11.0.0.

@philderbeast
Copy link
Contributor Author

@andreasabel, I used your fork of haskell-ci.

@philderbeast
Copy link
Contributor Author

philderbeast commented Jan 20, 2024

I saw the TODO and added precompute-fileinfo to the project. I hope that is alright.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 22, 2024

I saw the TODO and added precompute-fileinfo to the project. I hope that is alright.

As far as I can see that's fine. What an impressive cleanup, BTW. Thanks a lot!

@andreasabel
Copy link
Member

Atm cabal check seems to fail causing Haskell CI to fail...

@andreasabel
Copy link
Member

@philderbeast wrote:

I cleaned up some unused {-# LANGUAGE CPP #-} pragmas but note that some of the packages set this extension in default-extensions or other-extensions:

If none of the files of a package uses {-# LANGUAGE CPP #-}, it can be removed from other-extensions...

@philderbeast
Copy link
Contributor Author

Atm cabal check seems to fail causing Haskell CI to fail...

I wonder why this wasn't failing before this pull request as cabal check fails on the master branch when cabal >= 3.10?

$ pwd
~/.../hackage-security/hackage-security

$ cabal --version
cabal-install version 3.8.1.0
compiled using version 3.8.1.0 of the Cabal library 

$ cabal check
No errors or warnings could be found in the package.

$ cabal --version
cabal-install version 3.10.1.0
compiled using version 3.10.1.0 of the Cabal library 

$ cabal check
Warning: These warnings may cause trouble when distributing the package:
Warning: These packages miss upper bounds:
- ghc-prim
- text
Please add them, using `cabal gen-bounds` for suggestions. For more
information see: https://pvp.haskell.org/

$ cabal --version
cabal-install version 3.10.2.0
compiled using version 3.10.2.1 of the Cabal library 

$ cabal check
Warning: These warnings may cause trouble when distributing the package:
Warning: These packages miss upper bounds:
- ghc-prim
Please add them, using `cabal gen-bounds` for suggestions. For more
information see: https://pvp.haskell.org/

$ cabal --version
cabal-install version 3.10.2.1
compiled using version 3.10.2.1 of the Cabal library 

$ cabal check
Warning: These warnings may cause trouble when distributing the package:
Warning: These packages miss upper bounds:
- ghc-prim
Please add them, using `cabal gen-bounds` for suggestions. For more
information see: https://pvp.haskell.org/

@philderbeast
Copy link
Contributor Author

If none of the files of a package uses {-# LANGUAGE CPP #-}, it can be removed from other-extensions...

I've never previously bothered with other-extensions with my own packages. Maintaining this list would be quite manual, wouldn't it or is there tooling to help with that?

@Mikolaj
Copy link
Member

Mikolaj commented Jan 23, 2024

I haven't heard of any tooling.

@philderbeast
Copy link
Contributor Author

I've bumped the lower bounds to match the either the ghc-8.4.4 release notes or cabal gen-bounds.

@philderbeast
Copy link
Contributor Author

philderbeast commented Jan 23, 2024

I've also removed a couple of if impl(ghc >= 8.2) conditionals in .cabal files.

@andreasabel
Copy link
Member

Excellent work!

However, I think this refactoring should preserve the "impossibles" of the original code, i.e., crash when the original code crashed.

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Mikolaj
Copy link
Member

Mikolaj commented Jan 24, 2024

@philderbeast: ready for merge?

@andreasabel andreasabel added the pr: squash PR should be squashed upon merge label Jan 24, 2024
@philderbeast
Copy link
Contributor Author

@Mikolaj, I'll first setup the cabal CI to do a run through with source-repository-package back to this change, as you suggested in #303 (comment). While I could do this locally, I feel the CI version is a more complete check.

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great maintenance work!

@philderbeast
Copy link
Contributor Author

@philderbeast: ready for merge?

Yes please @Mikolaj.

@Mikolaj Mikolaj merged commit 1066cab into haskell:master Mar 18, 2024
@andreasabel
Copy link
Member

@Mikolaj The banner "pr: squash" is up but the individual commits were merged. Was this intentional? If yes, please change the banner to "pr: preserve commits".

@Mikolaj
Copy link
Member

Mikolaj commented Mar 19, 2024

My fault, I haven't seen the banner. Will do better next time. :)

@Mikolaj Mikolaj added pr: preserve commits Merge the individual commits of this PR (rather than squashing) and removed pr: squash PR should be squashed upon merge labels Mar 19, 2024
@Mikolaj
Copy link
Member

Mikolaj commented Mar 19, 2024

Actually, since this wasn't intentional, let me preserve the documentation of the mistake.

@Mikolaj Mikolaj added pr: squash PR should be squashed upon merge and removed pr: preserve commits Merge the individual commits of this PR (rather than squashing) labels Mar 19, 2024
@andreasabel
Copy link
Member

No problem.

I guess we wait for Cabal-3.12 before we release, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: squash PR should be squashed upon merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bump the lower bound on base?

4 participants