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

Re-expose CryptokitBignum module #31

Conversation

mbacarella
Copy link

The CryptokitBignum module stopped being exposed after cryptokit was updated to build with dune. This breaks users that were depending on it, and requires pinning to cryptokit <v1.16.

Assuming that this module was not intentionally removed from the interface of the library, please kindly consider merging this PR.

@mbacarella
Copy link
Author

I see that my editor helpfully removed trailing whitespaces from some lines and that my very OCaml aware diff tool didn't show that before I submitted.

Let me know if you'd like me to undo the trailing whitespace chopoffs. The only substantial change is the addition of module CryptokitBignum = CryptokitBignum.

@xavierleroy
Copy link
Owner

Before the switch to Dune, CryptokitBignum.cmi was installed along Cryptokit.cmi, making both visible.

You're proposing to make CryptokitBignum a sub-module of Cryptokit.

What about telling Dune to install CryptokitBignum.cmi like before?

@mbacarella
Copy link
Author

mbacarella commented Mar 11, 2022

Oh, right. My PR doesn't necessarily preserve backward compatibility as projects would now need to say open Cryptokit first or qualify it as Cryptokit.CryptokitBignum, whereas before they could just say CryptokitBignum.

Your suggestion would preserve the pre-dune behavior which would be nicer. Just... not sure what the right way of doing that with dune is.

@xavierleroy
Copy link
Owner

Here is a tentative fix: 08fe254 . Let me know if it does what you want.

@mbacarella
Copy link
Author

Here is a tentative fix: 08fe254 . Let me know if it does what you want.

Works fine now. My PR is no longer necessary.

@mbacarella mbacarella closed this May 31, 2022
@mbacarella
Copy link
Author

Thank you!

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.

2 participants