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

Fix C stubs dependency specification in mirage-crypto-ec #122

Merged

Conversation

TheLortex
Copy link
Member

When using mirage-crypto-ec with Mirage 4, I've encountered build failures as mirage_crypto.h couldn't be found when building mirage-crypto-ec's stubs. Indeed dune does not deduce from the -I../src/native flag that it has to track ../src/native as a dependency folder.

Instead the include_dirs option should be used: that's what this PR is about.

In the opam world this hasn't been a problem because the dune file in ec-freestanding correctly specifies the dependency.

For more information on the dune options for C compilation: https://dune.readthedocs.io/en/stable/concepts.html#foreign-stubs

@hannesm
Copy link
Member

hannesm commented May 25, 2021

Thanks. Your change looks good, though I don't like the wording "fix" (neither in the commit message nor in the changes entry). from my perspective, it wasn't broken before - and if dune does not support -I .. (or gets confused by its usage), it should complain about its usage and recommend to use include_dirs.

if you could rephrase that (and mention why and what you changed in this commit), I'm happy to merge this.

@TheLortex TheLortex force-pushed the fix-dependency-specification-crypto-ec branch from a3f8caa to 094c393 Compare May 25, 2021 09:58
When building C stubs, using the `include_dirs` field allows dune to
correctly track the dependency on mirage-crypto's header files.

This is necessary if one wants to build the `mirage-crypto-ec` library
without building `libmirage_crypto_ec_freestanding_stubs.a`.
@TheLortex TheLortex force-pushed the fix-dependency-specification-crypto-ec branch from 094c393 to f241fef Compare May 25, 2021 10:03
@TheLortex
Copy link
Member Author

Thank you for your review. Indeed nothing was "broken" before, but this is by luck, as the header files were grabbed when coincidentally building ec-freestanding. To me, we're "fixing" an issue that's upcoming (when the ec-freestanding build gets removed) -- but I've changed the changes entry and the commit message.

With regards to dune not supporting -I .., it's just that it doesn't parse C flags, it merely transfers them to the underlying C compiler, that's why the include_dirs field exists, and because it's not parsing the flags it cannot complain about the usage of the -I option. Additionally, the extra_deps field is available to track any dependency that would occur in the C build.

I hope that makes sense to you !

@hannesm hannesm merged commit a397200 into mirage:main May 25, 2021
@hannesm
Copy link
Member

hannesm commented May 25, 2021

thanks.

hannesm added a commit to hannesm/opam-repository that referenced this pull request Jun 7, 2021
…ge-crypto-rng, mirage-crypto-rng-mirage and mirage-crypto-rng-async (0.10.2)

CHANGES:

- mirage-crypto-ec: dune C stubs compilation rules: explicitely declare the
  include directory instead of listing it as a flag, so that the dependency
  is correctly tracked (mirage/mirage-crypto#122 by @TheLortex)
- mirage-crypto: compatibility with gcc11 (-Warray-parameters warning)
  (reported in mirage/mirage-crypto#124 by @TheLortex, fixed in mirage/mirage-crypto#125 by @hannesm)
- support for 64 bit RISC-V (mirage/mirage-crypto#127 by @edwintorok)
- Fixed esy cross-compile CI (mirage/mirage-crypto#126 by @EduardoRFS)
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

3 participants