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(x-compilation): find host ppx dependencies in the host context #7415

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Mar 26, 2023

While researching the cross-compilation story to fix #6191, I found about this failing test case from #4155 that never got fixed

cc @TheLortex

@anmonteiro anmonteiro changed the title fix: find host ppx dependencies in the host context fix(x-compilation): find host ppx dependencies in the host context Mar 26, 2023
@anmonteiro anmonteiro force-pushed the anmonteiro/with-host-db-arg branch 3 times, most recently from e783536 to 312f778 Compare March 26, 2023 04:48
@Alizter
Copy link
Collaborator

Alizter commented Mar 26, 2023

Would this mean you could have two opam switches and use the ppx from one when compiling packages depending on the other?

@anmonteiro
Copy link
Collaborator Author

I'm not too familiar with how contexts and opam switches are related, but I think there's a relation. If they're 1:1 I suspect that should be possible.

@anmonteiro
Copy link
Collaborator Author

I'm not too familiar with how contexts and opam switches are related, but I think there's a relation. If they're 1:1 I suspect that should be possible.

Reading the docs and the code a little more, it seems like this is in fact the case if you're setting up your cross compilation environment in OPAM. I think it might be even a bit more restrictive: you can only depend on PPXes in the other switch.

@Alizter
Copy link
Collaborator

Alizter commented Mar 26, 2023

That still seems OK to me. I wouldn't consider ppx a "runtime dependency" i.e. something that needs to be around when the code is executing but rather a "build time dependency". Modifying source code should be independent of platform, so whether this is performed in the target context or the host context doesn't really matter.

@anmonteiro
Copy link
Collaborator Author

Note that this doesn't exclude PPXes from being cross-compiled. This change just interprets the library dependencies in (pps ... ) in the host context.

@Alizter
Copy link
Collaborator

Alizter commented Mar 26, 2023

@anmonteiro Could you add a note about that in the cross-compilation section? i.e. that deps from pps are interpreted in the host?

@anmonteiro
Copy link
Collaborator Author

anmonteiro commented Mar 26, 2023

I believe this is already covered, it's just that it's broken. From https://dune.readthedocs.io/en/stable/cross-compilation.html#how-does-it-work :

One consequence of this is that all preprocessing (PPX or otherwise) will be done using binaries built in the default context.

@rgrinberg rgrinberg added this to the 3.8.0 milestone Mar 27, 2023
@anmonteiro anmonteiro force-pushed the anmonteiro/with-host-db-arg branch 2 times, most recently from a3adfed to 10e71cc Compare March 27, 2023 01:53
src/dune_rules/scope.ml Outdated Show resolved Hide resolved
@anmonteiro
Copy link
Collaborator Author

I tested this PR in my cross-compilation setup and everything seems to work as expected 👍

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

LGTM. Don't forget a CHANGES entry

let host =
let host =
Memo.Lazy.create @@ fun () ->
let+ installed_libs = Lib.DB.installed ~host:None host_context in
Copy link
Member

Choose a reason for hiding this comment

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

One day we should allow host contexts to have their host contexts as well.

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
Copy link
Collaborator Author

Rebased on main and added a CHANGES entry

@Alizter
Copy link
Collaborator

Alizter commented Mar 27, 2023

Just to be clear about the terminology, since this always confuses me, are we saying:

  • Host platform (where we are targetting)
  • Build platform (where we are building)

@anmonteiro
Copy link
Collaborator Author

In my understanding, it's actually a triplet: build, host and target. This is how I think about it:

Autotools Dune
Build Host
Host ---
Target Target

@rgrinberg rgrinberg merged commit 16a7e88 into ocaml:main Mar 27, 2023
@TheLortex
Copy link
Collaborator

That's great, thank you for the fix @anmonteiro !

anmonteiro added a commit to anmonteiro/dune that referenced this pull request Mar 30, 2023
gridbugs added a commit to gridbugs/dune that referenced this pull request Apr 13, 2023
gridbugs added a commit to gridbugs/dune that referenced this pull request Apr 14, 2023
@emillon emillon mentioned this pull request Jun 5, 2023
emillon added a commit to emillon/dune that referenced this pull request Jun 5, 2023
emillon added a commit to emillon/dune that referenced this pull request Jun 5, 2023
…ntext (ocaml#7415)"

This reverts commit 16a7e88.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Jun 5, 2023
…ntext (ocaml#7415)"

This reverts commit 16a7e88.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Jun 5, 2023
* Revert "fix(x-compilation): delay evaluation of `ppx_runtime_deps` until context is known"

This reverts commit ab74a71.

Signed-off-by: Etienne Millon <me@emillon.org>

* Revert "test(x-compilation): demonstrate overlap check failure with ppx_runtime_libraries"

This reverts commit 096fc97.

Signed-off-by: Etienne Millon <me@emillon.org>

* Revert "fix(x-compilation): find host ppx dependencies in the host context (#7415)"

This reverts commit 16a7e88.

Signed-off-by: Etienne Millon <me@emillon.org>

* Changelog

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon mentioned this pull request Jun 5, 2023
emillon added a commit to emillon/dune that referenced this pull request Jun 5, 2023
* Revert "fix(x-compilation): delay evaluation of `ppx_runtime_deps` until context is known"

This reverts commit ab74a71.

Signed-off-by: Etienne Millon <me@emillon.org>

* Revert "test(x-compilation): demonstrate overlap check failure with ppx_runtime_libraries"

This reverts commit 096fc97.

Signed-off-by: Etienne Millon <me@emillon.org>

* Revert "fix(x-compilation): find host ppx dependencies in the host context (ocaml#7415)"

This reverts commit 16a7e88.

Signed-off-by: Etienne Millon <me@emillon.org>

* Changelog

Signed-off-by: Etienne Millon <me@emillon.org>

---------

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Jun 5, 2023
* Revert "fix(x-compilation): delay evaluation of `ppx_runtime_deps` until context is known"

This reverts commit ab74a71.



* Revert "test(x-compilation): demonstrate overlap check failure with ppx_runtime_libraries"

This reverts commit 096fc97.



* Revert "fix(x-compilation): find host ppx dependencies in the host context (#7415)"

This reverts commit 16a7e88.



* Changelog



---------

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 5, 2023
CHANGES:

- Fix a crash when using a version of Coq < 8.13 due to the native compiler
  config variable being missing. We now explicitly default to `(mode vo)` for
  these older versions of Coq. (ocaml/dune#7847, fixes ocaml/dune#7846, @Alizter)

- Duplicate installed Coq theories are now allowed with the first appearing in
  COQPATH being preferred. This is inline with Coq's loadpath semantics. This
  fixes an issue with install layouts based on COQPATH such as those found in
  nixpkgs. (ocaml/dune#7790, @Alizter)

- Revert ocaml/dune#7415 and ocaml/dune#7450 (Resolve `ppx_runtime_libraries` in the target context when
  cross compiling) (ocaml/dune#7887, fixes ocaml/dune#7875, @emillon)
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Jun 5, 2023
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Jun 5, 2023
This reverts commit 886df09.

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Jun 9, 2023
This reverts commit 886df09.

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
emillon added a commit to emillon/dune that referenced this pull request Jun 27, 2023
emillon added a commit to emillon/dune that referenced this pull request Jun 28, 2023
emillon added a commit to emillon/dune that referenced this pull request Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants