Skip to content

ocaml: fix compatibility with ctypes 0.21.0#421

Merged
franziskuskiefer merged 2 commits intocryspen:mainfrom
emillon:ocaml-ctypes-compat
Jul 25, 2023
Merged

ocaml: fix compatibility with ctypes 0.21.0#421
franziskuskiefer merged 2 commits intocryspen:mainfrom
emillon:ocaml-ctypes-compat

Conversation

@emillon
Copy link
Copy Markdown
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.

@emillon
ocaml: fix compatibility with ctypes 0.21.0
d3272c0 
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
Copy Markdown

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
Copy Markdown
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
Closed
18 tasks
emillon added a commit to yallop/opam-repository that referenced this pull request Jul 17, 2023
@emillon
Restrict hacl-star-raw
c8b0b2c 
Upstream fix: cryspen/hacl-packages#421
franziskuskiefer
franziskuskiefer approved these changes Jul 19, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown

coveralls commented Jul 19, 2023

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
Copy Markdown
Contributor

victor-dumitrescu commented Jul 20, 2023

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
Copy Markdown
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
Copy Markdown
Contributor

victor-dumitrescu commented Jul 21, 2023

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
Copy Markdown
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
@franziskuskiefer franziskuskiefer mentioned this pull request Aug 7, 2023
Merged
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@emillon @nberth
Restrict hacl-star-raw
ab898c3 
Upstream fix: cryspen/hacl-packages#421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@franziskuskiefer franziskuskiefer franziskuskiefer approved these changes

+1 more reviewer

@victor-dumitrescu victor-dumitrescu victor-dumitrescu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@emillon @coveralls @victor-dumitrescu @franziskuskiefer