Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build with crypton #8

Closed
wants to merge 5 commits into from
Closed

Conversation

bmillwood
Copy link

@bmillwood bmillwood commented Jul 16, 2024

The >= syntax is deprecated. Putting the line as the first line in the
file is not strictly necessary for 1.16, but it is allegedly necessary
as of 2.2 and harmless before then.

https://cabal.readthedocs.io/en/stable/cabal-package-description-file.html#pkg-field-cabal-version
This also requires updating dependencies to use crypton.

Unfortunately, cryptostore will only use crypton if the use_crypton
Cabal flag is enabled, and we can't make that happen from our cabal
file. I opened an issue to ask them to remove `manual: True` from their
flag, so that the solver can try enabling it. See:
ocheron/cryptostore#13
Comment on lines +41 to +44
run: cabal build --enable-tests --enable-benchmarks all --constraint="cryptostore +use_crypton"
- name: Run tests
run: cabal test test:testsuite
# as above
run: cabal test test:testsuite --constraint="cryptostore +use_crypton"

Choose a reason for hiding this comment

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

Wouldn't be more simple to create a cabal.project like:

packages: .

package cryptostore
  flags: +use_crypton

This would not solve the problem of the configuring the +use_crypton flag between transitive dependencies (ie: a package using jwt and crypton will have to set the +use_crypton flag in its own cabal.project file), but we would avoid using command line flags in CI.

@@ -33,7 +33,7 @@ library
exposed-modules: Web.JWT
other-modules: Data.Text.Extended, Data.ByteString.Extended
build-depends: base >= 4.8 && < 5
, cryptonite >= 0.6
, crypton >= 0.31
, cryptostore >= 0.2

Choose a reason for hiding this comment

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

I would put cryptostore >= 0.3.0.1, when the use_crypton flag was introduced.

@puffnfresh
Copy link
Owner

I copied the use_crypton flag into jwt 2b3167c. I think cryptostore should drop the flag, making crypton the default. I'll quickly do the same if/when that happens.

@puffnfresh puffnfresh closed this Sep 9, 2024
@marinelli
Copy link

I copied the use_crypton flag into jwt 2b3167c. I think cryptostore should drop the flag, making crypton the default. I'll quickly do the same if/when that happens.

Thanks for having a look.

@bmillwood
Copy link
Author

bmillwood commented Sep 11, 2024

Looks like CI is failing for want of the change I made to tests/src/Web/JWTInteropTests.hs . I think that change is related to upgrading the aeson version -- honestly I don't remember why I included it here but maybe you want to pick that change up too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants