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

Make yaml work in monorepos #52

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

TheLortex
Copy link

This PR is in two parts:

  • first an upgrade to dune 2.0
  • then two changes to make yaml work in monorepos:
    • use realpath to make ctypes' include path absolute, because %{lib:ctypes:ctypes_cstubs_internals.h} is relative in a monorepo. I have added a C stub for it, because Unix.realpath only exists since 4.13.
    • add explicit dependencies for all the header files that are used. Otherwise they don't get installed.

@TheLortex
Copy link
Author

@avsm A 3.0.2 release with this PR would be very useful for mirage-www and the irmin monorepo work !

@avsm
Copy link
Owner

avsm commented Mar 4, 2022

This probably breaks Windows due to the C stub. I'm ok with a new release of Yaml that's 4.13+ only and using Unix.realpath, as that's going to be our practical minimum version for Mirage anyway.

Also instead of the absolute path, shouldn't it be normalised to the project root? Otherwise reproducibility is affected.

@TheLortex
Copy link
Author

This probably breaks Windows due to the C stub. I'm ok with a new release of Yaml that's 4.13+ only and using Unix.realpath, as that's going to be our practical minimum version for Mirage anyway.

Okay, I'll update the PR.

Also instead of the absolute path, shouldn't it be normalised to the project root? Otherwise reproducibility is affected.

I do'nt think reproducibility is affected.
Indeed the intermediate file ctypes-cflags won't be reproducible, but it is not installed, only used in the following command: run %{cc} %{c} -I../../vendor %{read-lines:../../config/ctypes-cflags} -I %{ocaml_where} -o %{targets})) whose result is independent of the project root.

@avsm
Copy link
Owner

avsm commented Mar 6, 2022

Sounds good. It does affect relocatability, but we can deal with that once OCaml itself supports it.

@samoht
Copy link
Collaborator

samoht commented Mar 27, 2022

Would be nice to merge and release this as we need it to build mirage-www.

This was referenced Mar 27, 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.

None yet

3 participants