Add flags to avoid Cryptonite/X509 #871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I understand your pain. I have no objection. To my quick check, the code itself is high quality. |
snoyberg
reviewed
Feb 25, 2022
| @@ -15,8 +16,13 @@ import Data.Function (on) | |||
| import qualified Data.Text as T | |||
| import Data.Ord | |||
| import qualified Data.ByteString as S | |||
| #ifdef USE_CRYPTONITE | |||
There was a problem hiding this comment.
I think you could use the build in Cabal macros for this instead. There's definitely MIN_VERSION_cryptonite, but I think there may be others too.
There was a problem hiding this comment.
MIN_VERSION_cryptonite seems to work perfectly here, so I have force-pushed revised commits that use this instead. Thank you for the review.
snoyberg
approved these changes
Feb 27, 2022
|
Merged. Thank you for your contribution. If you would like to have a new release, please let me know. |
|
@kazu-yamamoto Thank you for the merge. Yes, a release would be very nice, since that would allow me to run CI for 9.2 without using |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many warp users use a reverse proxy for TLS termination. But before this PR,
the warp library had an unconditional dependency on the x509 library. The x509
library has a big dependency footprint, which would be irrelevant for these
users. Another problem with the x509 dependency is that it depends on
Cryptonite, which is updated slowly, and is still not available for GHC 9.2.
The wai-app-static library also depends on Cryptonite, but only for computing
MD5 hashes. It seems excessive to depend on this very large library for just a
single hash function. So this PR adds a flag to enable compilation with a
different, also very well-trusted library.
Because both of these newly added flags are enabled by default, it means that a
user that isn't carefully reading the warp/wai-app-static release notes will
not notice any disruption. People that try to upgrade to GHC 9.2 after this PR
is merged may notice that the client certificates are no longer available. But
that is not so bad, since it only happens while upgrading their compiler, which
commonly introduces all kinds of breaking changes. So I don't consider it a big
detraction.
Tested using the following
cabal.project:packages: wai-app-static warpcabal test all -f -cryptonite -f -x509 -w ghc-9.2.1cabal test all -f cryptonite -f x509 -w ghc-9.0.2