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

generate should return string rather than Cstuct.t #388

Merged
merged 6 commits into from Mar 25, 2024

Conversation

oemmerson
Copy link
Contributor

Previously building these examples would fail with the error

Error: This expression has type int -> Cstruct.t
       but an expression was expected of type int -> string
       Type Cstruct.t is not compatible with type string
make[1]: *** [build] Error 1

Previously building these examples would fail with the error

```
Error: This expression has type int -> Cstruct.t
       but an expression was expected of type int -> string
       Type Cstruct.t is not compatible with type string
make[1]: *** [build] Error 1
```
@reynir
Copy link
Member

reynir commented Mar 23, 2024

Thank you for your PR! The recent randomconv.0.2.0 release changed the expected type from Cstruct.t to string in anticipation of an upcoming mirage-crypto release that brings a cstruct->string overhaul.

I would say the preferred way forward is to set an upper bound on randomconv<0.2.0 for now.

This reverts commit d11689b.

Reverting this as per Reynir's suggestions in
mirage#388 (comment)
Recent randomconv.0.2.0 release changed the expected type from
Cstruct.t to string in anticipation of an upcoming mirage-crypto
release that brings a cstruct->string overhaul.

This change sets an upper bound on randomconv<0.2.0 until that work is
complete.
@oemmerson
Copy link
Contributor Author

oemmerson commented Mar 23, 2024

@reynir Thanks for explaining. I've reverted the original commit, added the upper bound you suggested and checked the kernel's build without issue, so you can squash them or just cherry pick the final commit if you want the changes.

Copy link
Member

@reynir reynir left a comment

Choose a reason for hiding this comment

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

Thanks! This is great. One thing though: The ~max argument to Mirage.package is exclusive meaning ~max:"0.1.3" is <"0.1.3". I forget this all the time, and the documentation is not-so-helpfully referring to some Functoria documentation (I will open an issue on that).

tutorial/lwt/echo_server/config.ml Outdated Show resolved Hide resolved
tutorial/lwt/timeout1/config.ml Outdated Show resolved Hide resolved
tutorial/lwt/timeout2/config.ml Outdated Show resolved Hide resolved
oemmerson and others added 3 commits March 24, 2024 09:09
Max is exclusive.

Co-authored-by: Reynir Björnsson <reynir@reynir.dk>
Max is exclusive.

Co-authored-by: Reynir Björnsson <reynir@reynir.dk>
Max is exclusive.

Co-authored-by: Reynir Björnsson <reynir@reynir.dk>
@reynir reynir merged commit c4936ad into mirage:main Mar 25, 2024
3 checks passed
@reynir
Copy link
Member

reynir commented Mar 25, 2024

Thanks a lot!

samoht pushed a commit that referenced this pull request Apr 10, 2024
Co-authored-by: "O. Emmerson" <oemmerson@gmx.com>
Co-authored-by: Reynir Björnsson <reynir@reynir.dk>
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.

None yet

2 participants