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

windows/dkml: opam install with deps-only and with-test only packages to test, deps-only for only build #198

Merged
merged 6 commits into from
Feb 24, 2024

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Feb 22, 2024

@hannesm
Copy link
Member Author

hannesm commented Feb 22, 2024

the std_cflags are properly picked up (in contrast to #137 (comment)), also this fixes #137 (comment)

remaining is the following issue, on windows 64 bit systems, the fiat 32 bit versions are used (reason behind this is that 128bit unsigned integers are not available) -- now with our pre-computed tables, this results in an error. I guess on a windows 64 bit system, using the 32 bit tables is fine. any other opinion @Firobe @jonahbeckford ? I added that in 975a881 (but unfortunately our mirage-crypto-ec tests aren't running on dkml/windows since mirage-crypto-pk and asn1-combinators don't work there (gmp/zarith).

@hannesm hannesm force-pushed the win branch 2 times, most recently from 5589386 to 65e0b63 Compare February 23, 2024 12:39
@hannesm
Copy link
Member Author

hannesm commented Feb 23, 2024

Thanks to #200, we can now run mirage-crypto-ec tests. Unfortunately, they fail:

File "tests/dune", line 60, characters 7-25:
60 |  (name test_ec_wycheproof)
            ^^^^^^^^^^^^^^^^^^
(cd _build/default/tests && ./test_ec_wycheproof.exe)
Fatal error: exception File "tests/test_ec_wycheproof.ml", line 418, characters 4-10: Assertion failed

It is not entirely clear to me, why, though. The assetion in question:

let to_ed25519_keys (key : eddsa_key) =
  let priv_cs = Cstruct.of_string key.sk
  and pub_cs = Cstruct.of_string key.pk
  in
  match Ed25519.priv_of_cstruct priv_cs, Ed25519.pub_of_cstruct pub_cs with
  | Ok priv, Ok pub ->
    assert (Cstruct.equal Ed25519.(pub_to_cstruct (pub_of_priv priv)) pub_cs); (* that's the failing one *)
    priv, pub
  | _ -> assert false

This means that the (pub_to_cstruct (pub_of_priv _)) isn't equal to the public key. Looks like something is going wrong down the line, but what?

@hannesm
Copy link
Member Author

hannesm commented Feb 23, 2024

to be clear, I don't have any further energy and time to look deeper into "CL.EXE" and what goes wrong. If someone can pick this up, best with an actual setup to have quick compile-test-edit cycles, that'd be amazing.

I propose to do some printf debugging (looking where pub_cs is different from pub_to_cstruct (pub_to_priv priv). It may have to do that OCaml assumes 64bit, and the C code 32bit. There are also a huge amount of warnings from the C compiler, which may be worth to look into.

If there's no interest/time by others, I plan to revert the DKML changes in the main branch to cut a mirage-crypto release.

@jonahbeckford
Copy link
Contributor

I have no time to do this at the moment. While I think it would be better just to mark the ec opam release as unavailable on windows, do what you think is best.

@hannesm
Copy link
Member Author

hannesm commented Feb 24, 2024

ok, I removed mirage-crypto-ec from the windows-dkml-ci script. plan to merge this when CI is green.

I have no idea @jonahbeckford about your plans to use mirage-crypto on dkml/windows -- but I'm sure your suggestion "remove mirage-crypto-ec from the build" fits your needs. I'll open an issue about the mirage-crypto-ec and dkml.

@hannesm hannesm merged commit ccbf964 into mirage:main Feb 24, 2024
29 checks passed
@hannesm hannesm deleted the win branch February 24, 2024 14:15
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