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

Add back correct host for ppx.exe #3751

Merged
1 commit merged into from
Mar 4, 2021
Merged

Add back correct host for ppx.exe #3751

1 commit merged into from
Mar 4, 2021

Conversation

toots
Copy link
Contributor

@toots toots commented Aug 29, 2020

Fixes: #3698, which is a regression from #2286. Should we add a test for it?

@rgrinberg
Copy link
Member

Should we add a test for it?

Definitely. The current semantics are subtle and we should make sure that they aren't changed accidentally.

@rgrinberg
Copy link
Member

You also need to run make fmt.

@toots toots force-pushed the host-ppx.exe-bis branch from 864ce61 to c576d99 Compare August 29, 2020 20:11
@toots
Copy link
Contributor Author

toots commented Aug 29, 2020

@rgrinberg Just added the fmt bits. Any advice/example of a similar test I could look at?

@toots toots force-pushed the host-ppx.exe-bis branch from c576d99 to 86a69da Compare August 29, 2020 20:12
@rgrinberg
Copy link
Member

cross-compilation.t shows you how to setup a cross compilation environment. Let me know if that's enough.

@rgrinberg
Copy link
Member

There are also other tests that use dune install --prefix _install to test installed packages.

@rgrinberg rgrinberg modified the milestones: 2.7.1, 2.8 Aug 29, 2020
@toots
Copy link
Contributor Author

toots commented Sep 15, 2020

Hi! Sorry I haven't forgotten this one but haven't found the time to investigate those tests yet. If you're about to release, feel free to include this patch and I'll submit the tests in a separate PR..

@toots
Copy link
Contributor Author

toots commented Nov 6, 2020

Hi @rgrinberg. I'm afraid it's getting difficult for me to work on adding tests for this feature. I am not familiar enough with the test suite to add it quickly and haven't found the time to study it properly. Any chance that at least the patch could be merged as-is? Currently, dune is still broken for cross-compiling with ppx unless one stick to 1.x releases.. Thanks!

@toots toots force-pushed the host-ppx.exe-bis branch 4 times, most recently from 4e5a6a1 to d4df43a Compare November 12, 2020 16:08
@toots
Copy link
Contributor Author

toots commented Nov 12, 2020

Hi again! I've refreshed the PR. It would be nice if it could be considered for inclusion as-is. I'll have another pass at trying to write a test for this but it doesn't seem easy as far as I've been able to see.

Meanwhile, this bug is blocking the migration of ocaml-cross-windows to 4.11.1 as most of the ecosystem for that version has moved to dune >= 2.0. I wouldn't want to have to maintain a forked package just for this repository..

Thanks!

@toots
Copy link
Contributor Author

toots commented Nov 12, 2020

PS: I also ran ocamlformat on the file, it seems fine.

@toots toots mentioned this pull request Jan 2, 2021
@rgrinberg rgrinberg modified the milestones: 2.8, 2.9 Jan 13, 2021
Base automatically changed from master to main January 14, 2021 17:08
@toots toots mentioned this pull request Mar 3, 2021
Signed-off-by: Romain Beauxis <[email protected]>
@ghost ghost force-pushed the host-ppx.exe-bis branch from 122b839 to 099ca5c Compare March 4, 2021 12:38
@ghost ghost merged commit eec019e into ocaml:main Mar 4, 2021
@ghost
Copy link

ghost commented Mar 4, 2021

You are the maintainer of the ocaml-cross repos, so happy to trust you on this. FTR, the testsuite is using cram tests, which is now a public and documented feature of Dune, so adding tests to the Dune testsuite should be easier. We also have a HACKING.md file with a short introduction to Dune's tests.

@rgrinberg what do you think of adding this to 2.8.3?

@rgrinberg rgrinberg removed this from the 2.9 milestone Mar 4, 2021
@rgrinberg rgrinberg added this to the 2.8.3 milestone Mar 4, 2021
rgrinberg pushed a commit that referenced this pull request Mar 7, 2021
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Mar 7, 2021
…ne-action-plugin, dune-private-libs and dune-glob (2.8.3)

CHANGES:

- Make `patdiff` show refined diffs (ocaml/dune#4257, fixes ocaml/dune#4254, @hakuch)

- Fixed a bug that could result in needless recompilation under Windows due to
  case differences in the result of `Sys.getcwd` (observed under `emacs`).
  (ocaml/dune#3966, @nojb).

- Restore compatibility with Coq < 8.10 for coq-lang < 0.3 , document
  that `(using coq 0.3)` does require Coq 8.10 at least (ocaml/dune#4224, fixes
  ocaml/dune#4142, @ejgallego)

- Add a META rule for 'compiler-libs.native-toplevel' (ocaml/dune#4175, @AltGr)

- No longer call `chmod` on symbolic links (fixes ocaml/dune#4195, @dannywillems)

- Dune no longer automatically create or edit `dune-project` files
  (ocaml/dune#4239, fixes ocaml/dune#4108, @jeremiedimino)

- Have `dune` communicate the location of the standard library directory to
  `merlin` (ocaml/dune#4211, fixes ocaml/dune#4188, @nojb)

- Workaround incorrect exception raised by Unix.utimes (OCaml PR#8857) in
  Path.touch on Windows (ocaml/dune#4223, @dra27)

- `dune ocaml-merlin` is now able to provide configuration for source files in
  the `_build` directory. (ocaml/dune#4274, @voodoos)

- Automatically delete left-over Merlin files when rebuilding for the first time
  a project previously built with Dune `<= 2.7`. (ocaml/dune#4261, @voodoos, @aalekseyev)

- Fix `ppx.exe` being compiled for the wrong target when cross-compiling
  (ocaml/dune#3751, fixes ocaml/dune#3698, @toots)

- `dune top` correctly escapes the generated toplevel directives, and make it
  easier for `dune top` to locate C stubs associated to concerned libraries.
  (ocaml/dune#4242, fixes ocaml/dune#4231, @nojb)

- Do not pass include directories containing native objects when compiling
  bytecode (ocaml/dune#4200, @nojb)
This pull request was closed.
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.

Issues cross compiling with sedlex
2 participants