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

Update to Eio 0.12 #182

Merged
merged 1 commit into from Sep 18, 2023
Merged

Update to Eio 0.12 #182

merged 1 commit into from Sep 18, 2023

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Aug 30, 2023

Eio 0.12 no longer uses OCaml objects for OS resources. This requires a minor change to mirage-crypto-rng-eio to use the new variant types instead.

(tested using my Eio 0.12 branch of ocaml-tls)

@bikallem
Copy link
Contributor

bikallem commented Sep 9, 2023

Seems straightforward. LGTM.

@haesbaert ocaml-tls depends on this package so would appreciate if we could expedite a new release of mirage-crypto-rng-eio

@haesbaert
Copy link
Member

Seems straightforward. LGTM.

@haesbaert ocaml-tls depends on this package so would appreciate if we could expedite a new release of mirage-crypto-rng-eio

I'm sure you meant @hannesm here though :)

@bikallem
Copy link
Contributor

I'm sure you meant @hannesm here though :)

Oops, sorry. Yes, I meant @hannesm.

@talex5
Copy link
Contributor Author

talex5 commented Sep 14, 2023

There are some CI failures here (error: writing 1 byte into a region of size 0 and Cannot parse point: point is not on curve), but they are unrelated to this PR and are also failing on the main branch.

As this PR is only changing a type, and only affects the Eio package, it should be safe to merge.

@hannesm
Copy link
Member

hannesm commented Sep 14, 2023

Thanks for your PR. Indeed the CI failures look unrelated (see #178 and a lengthy discussion in mit-plv/fiat-crypto#1606 (comment)).

In order to get a new release of mirage-crypto, I'd like to solve these outstanding issues, and merge this PR as well. I hope to find some time next week to get around cutting a release.

Just to be sure we're on the same page -- the Eio.Flow.read_exact env#secure_random buf; and Eio.Time.sleep env#clock (Duration.to_f delta); are fine with the most recent eio (I'm asking since in both cases still objects are used..).

@talex5
Copy link
Contributor Author

talex5 commented Sep 14, 2023

Just to be sure we're on the same page -- the Eio.Flow.read_exact env#secure_random buf; and Eio.Time.sleep env#clock (Duration.to_f delta); are fine with the most recent eio (I'm asking since in both cases still objects are used..).

Yes, that's fine (clock and secure_random are no longer objects, but env is).

We could pass the resources to mirage-crypto as separate arguments instead of bundling them up in an object, e.g.

 val run
   :  ?g:'a
   -> ?sleep:int64
   -> 'a Mirage_crypto_rng.generator
-  -> _ env
+  -> clock:_ Eio.Time.clock
+  -> mono_clock:_ Eio.Time.Mono.t
+  -> secure_random:_ Eio.Flow.source
   -> (unit -> 'b) -> 'b

However, continuing to use an object here means code using mirage-crypto doesn't need to change, and it allows adding extra resources without requiring changes to user code in the common case (e.g. I see that #176 added a monotonic clock that way).

BTW, the only use of the non-monotonic clock is to sleep for 10s, which could also be done with the monotonic clock, avoiding the need for clock at all. If you'd like clock removed, I could do that here, or in a separate PR.

@hannesm hannesm merged commit 5bbfe2c into mirage:main Sep 18, 2023
26 of 27 checks passed
hannesm added a commit to hannesm/opam-repository that referenced this pull request Sep 18, 2023
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.2)

CHANGES:

* mirage-crypto-rng-eio: improve portability by using eio 0.7's monotonic clock
  interface instead of mtime.clock.os. (mirage/mirage-crypto#176 @TheLortex)
* mirage-crypto-rng-eio: update to eio 0.12 (mirage/mirage-crypto#182 @talex5)
* mirage-crypto-rng: fix typo in RNG setup (mirage/mirage-crypto#179 @samueldurantes)
* macOS: on arm64 with clang 14.0.3, avoid instcombine (due to miscompilations)
  reported by @samoht mit-plv/fiat-crypto#1606 (comment)
  re-reported in ulrikstrid/ocaml-jose#63 and mirleft/ocaml-tls#478
  (mirage/mirage-crypto#185 @hannesm @kit-ty-kate)
* avoid "stringop-overflow" warning on PPC64 and S390x (spurious warnings) when
  in devel mode (mirage/mirage-crypto#178 mirage/mirage-crypto#184 @avsm @hannesm)
* stricter C prototypes, unsigned/signed integers (mirage/mirage-crypto#175 @MisterDA @haesbaert
  @avsm @hannesm)
* support DragonFlyBSD (mirage/mirage-crypto#181 @movepointsolutions)
* support GNU/Hurd (mirage/mirage-crypto#174 @pinotree)
@hannesm
Copy link
Member

hannesm commented Sep 18, 2023

this is part of the 0.11.2 release at ocaml/opam-repository#24461. thanks again for your PR. You can PR what you like, I have no preference when it comes to "eio" and its usage of objects and clocks (I tried again to understand the whole "env" stuff, but failed and will just merge whatever CI and authors consider as being fine).

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

4 participants