haskell / hackage-security

Hackage security framework based on TUF (The Update Framework)
http://hackage.haskell.org/package/hackage-security
56 stars 47 forks source link

Bump base for ghc-8.4 #306

Closed philderbeast closed 8 months ago

philderbeast commented 10 months ago

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 commented 10 months ago

[!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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

philderbeast commented 10 months ago

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

Mikolaj commented 10 months ago

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 commented 10 months ago

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

andreasabel commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

I haven't heard of any tooling.

philderbeast commented 10 months ago

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

philderbeast commented 10 months ago

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

andreasabel commented 10 months ago

Excellent work!

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

Mikolaj commented 10 months ago

@philderbeast: ready for merge?

philderbeast commented 10 months ago

@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 https://github.com/haskell/hackage-security/issues/303#issuecomment-1902250573. While I could do this locally, I feel the CI version is a more complete check.

philderbeast commented 8 months ago

@philderbeast: ready for merge?

Yes please @Mikolaj.

andreasabel commented 8 months ago

@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 commented 8 months ago

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

Mikolaj commented 8 months ago

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

andreasabel commented 8 months ago

No problem.

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