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

ocaml: fix compatibility with ctypes 0.21.0 #421

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

emillon
Copy link
Contributor

@emillon emillon commented Jul 17, 2023

In ctypes < 0.21.0, the ctypes and ctypes.stubs libraries were installed in the same directory, so depending on one would make the other one visible.
ctypes 0.21.0 installs them in different directories so this makes it an error.
hacl-star-raw uses ctypes.stubs so the dependency should be recorded.

In ctypes < 0.21.0, the ctypes and ctypes.stubs libraries were installed
in the same directory, so depending on one would make the other one
visible.
ctypes 0.21.0 installs them in different directories so this makes it an
error.
hacl-star-raw uses ctypes.stubs so the dependency should be recorded.
@emillon emillon requested a review from a team as a code owner July 17, 2023 13:36
@cla-bot
Copy link

cla-bot bot commented Jul 17, 2023

We require contributors to sign our Contributor License Agreement https://github.com/cryspen/hacl/blob/main/CLA.md ensuring that the contribution can be licensed under Apache 2.0 and MIT. In order for us to review and merge your code, please mention @cryspen/core in a comment below to get yourself added.

@emillon
Copy link
Contributor Author

emillon commented Jul 17, 2023

@cryspen/core I agree with the terms of the CLA.

@emillon emillon mentioned this pull request Jul 17, 2023
18 tasks
emillon added a commit to yallop/opam-repository that referenced this pull request Jul 17, 2023
Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks! lgtm but @victor-dumitrescu should have a look as well as maintainer of the package.

@victor-dumitrescu can you have a look and bump versions/do a new release if needed?

@cla-bot cla-bot bot added the cla-signed label Jul 19, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5596166449

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 52.764%

Totals Coverage Status
Change from base Build 5596163700: 0.02%
Covered Lines: 28938
Relevant Lines: 54844

💛 - Coveralls

@victor-dumitrescu
Copy link
Collaborator

Hi, thanks for submitting the fix! @emillon can you confirm that you tested it with ctypes 0.21.0? Do we need to add a dependency to our opam file too?

@emillon
Copy link
Contributor Author

emillon commented Jul 21, 2023

can you confirm that you tested it with ctypes 0.21.0?

Yes, I have tested that with ctypes 0.21.0. To be more precise about the problem, hacl-star-raw main build and installs correctly even with ctypes 0.21.0, but then hacl-star does not build correctly. hacl-star-raw built with this fix will build and install correctly, and be usable when used with hacl-star.

Do we need to add a dependency to our opam file too?

In terms of metadata:

@victor-dumitrescu
Copy link
Collaborator

I see, I was wondering whether I should add an explicit extra dependency in the opam file on ctypes.foreign for example, or if it's ok to only have ctypes

@emillon
Copy link
Contributor Author

emillon commented Jul 21, 2023

ctypes has 2 APIs: stub generation (generate C and ml code from descriptions) and foreign (use libffi to call directly into a .so file). If you use the ctypes.foreign library, you need in addition to record the dependency on the ctypes-foreign package, but that's not the case of hacl-star-raw.

@victor-dumitrescu victor-dumitrescu added this pull request to the merge queue Jul 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 21, 2023
@franziskuskiefer franziskuskiefer added this pull request to the merge queue Jul 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 24, 2023
@franziskuskiefer franziskuskiefer merged commit 81303b8 into cryspen:main Jul 25, 2023
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants