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

[WIP] use new dune ctypes stanza #41

Closed
wants to merge 24 commits into from

Conversation

mbacarella
Copy link

Ports ocaml-yaml to use the anticipated dune ctypes feature
ocaml/dune#3905

Using this feature gets rid of the stub generator utilities and intermediate libraries. This PR also drops some deprecated/removed dune features.

ffi/dune Outdated
; hack: repeated include dirs from root and relative since this gets
; called from multiple places
(c_flags (-I../vendor/headers -Ivendor/headers))))
(headers (preamble "#if defined(__MINGW32__) || defined(__MINGW64__)
Copy link
Owner

Choose a reason for hiding this comment

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

This preamble might be better off in a separate file, and referenced from here. I can see it getting awkward fast to embed C headers in dune files.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, the dune stanza already supports that. I just pushed a version that does the include approach instead.

(I think it's still nice to provide an option to include a preamble in the dune feature as well)

@avsm
Copy link
Owner

avsm commented Apr 26, 2021

This is a very pleasing amount of red indeed! The new stanzas are looking awesome...

@mbacarella mbacarella changed the title use new dune ctypes stanza [WIP] use new dune ctypes stanza May 23, 2021
@mbacarella
Copy link
Author

marked as WIP. this depends on dune 3.0 to be released (and some additional meta file fixups) before it can be merged.

@avsm
Copy link
Owner

avsm commented Jul 12, 2021

I haven't reviewed in detail, but the overall approach in this PR looks very good to me.

@TheLortex
Copy link

How ready is this PR ? It will be useful for dune monorepos as there are issues with the ctypes port currently.
For info Dune 3 is getting released very soon ! ocaml/opam-repository#20721

@mbacarella
Copy link
Author

How ready is this PR ? It will be useful for dune monorepos as there are issues with the ctypes port currently. For info Dune 3 is getting released very soon ! ocaml/opam-repository#20721

I'm not aware of any deficiencies, it's WIP because I don't want it accidentally merged with the pin-depends to a specific dune version in there. I'll freshen this PR up after dune 3.0 is released.

(vendored
; hack: repeated include dirs from root and relative since this gets
; called from multiple places
(c_flags (-I../vendor/headers -Ivendor/headers))))

Choose a reason for hiding this comment

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

I think the only issue is here: this doesn't work in a dune workspace created with opam-monorepo.
This is already known but I'm linking the relevant issue : ocaml/dune#5325

Copy link
Author

Choose a reason for hiding this comment

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

Ah. It may take me awhile to get around to updating dune-ctypes to fix this the clean way.
Thoughts on what a better hack than the current hack would be in the meantime? I assume this comes up fairly often in opam-monorepo?

@sim642
Copy link
Contributor

sim642 commented Jun 18, 2022

I'll freshen this PR up after dune 3.0 is released.

That has now been released, so this PR could be finalized.

@mbacarella
Copy link
Author

Closing in favor of #67

@mbacarella mbacarella closed this Jun 18, 2022
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.

4 participants