fix: ppx and reason bug #7932
fix: ppx and reason bug #7932
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
f22874b to
7db5af3
Compare
7db5af3 to
a453b58
Compare
a453b58 to
e591a10
Compare
e9db434 to
2553102
Compare
2553102 to
a9cc4bb
Compare
|
I reproduced that without |
a9cc4bb to
ad3c6aa
Compare
|
@Alizter I rebased and pushed a fix. Let me know what you think. This will need a changelog entry. Thanks! |
|
@emillon this PR needs review from me and @anmonteiro still |
|
sure, to be clear I was just addressing the test |
|
@emillon It already has a changelog entry, is something missing? |
ugh, coffee for me I guess. sorry! |
src/dune_rules/preprocessing.ml
Outdated
| @@ -710,6 +710,10 @@ let pp_one_module sctx ~lib_name ~scope ~preprocessor_deps | |||
| ; Path (Path.build dst) | |||
| ; Command.Ml_kind.ppx_driver_flag ml_kind | |||
| ; Dep (Path.build src) | |||
| ; Hidden_deps | |||
There was a problem hiding this comment.
I don't understand why this is needed.
IIUC, the pipeline looks something like:
.re -> (dialect) .re.ml -> (pps) .pp.ml
So isn't the original source file already a (transitive) dependency of the pps action?
There was a problem hiding this comment.
But sandboxing does not take the transitive closure, onoy the specified deoendencies. Thats why its important to always fully specify them.
ad3c6aa to
2485dc3
Compare
|
@rgrinberg I've dropped the commit modifying the targets in inline_tests.ml. As you can see this creates an exception. I can fix it by tweaking the source files: diff --git a/src/dune_rules/inline_tests.ml b/src/dune_rules/inline_tests.ml
index 77a92acc2..941900c09 100644
--- a/src/dune_rules/inline_tests.ml
+++ b/src/dune_rules/inline_tests.ml
@@ -285,7 +285,10 @@ include Sub_system.Register_end_point (struct
in
List.concat l
in
- let source_files = List.concat_map source_modules ~f:Module.sources in
+ let source_files =
+ List.concat_map source_modules ~f:(fun module_ ->
+ Module.ml_source module_ |> Module.sources)
+ in
Memo.parallel_iter_set
(module Mode_conf.Set)
info.modes
diff --git a/test/blackbox-tests/test-cases/reason-expect.t/run.t b/test/blackbox-tests/test-cases/reason-expect.t/run.t
index d1c599f90..f694d3809 100644
--- a/test/blackbox-tests/test-cases/reason-expect.t/run.t
+++ b/test/blackbox-tests/test-cases/reason-expect.t/run.t
@@ -20,18 +20,7 @@ syntax and ppx_expect break at dune lang 3.3 due to the sandboxing of ppx.
> (lang dune 3.9)
> EOF
$ dune test
- File "dune", line 3, characters 1-15:
- 3 | (inline_tests)
- ^^^^^^^^^^^^^^
- Fatal error: exception Sys_error("$TESTCASE_ROOT/_build/.sandbox/31552d8346d4edb1acb27f906d1a4b1b/default/./test.re.ml: No such file or directory")
- Raised by primitive operation at Stdlib.open_in_gen in file "stdlib.ml", line 405, characters 28-54
- Called from Stdio__In_channel.with_file in file "src/in_channel.ml", line 18, characters 45-66
- Called from Stdio__In_channel.read_all in file "src/in_channel.ml" (inlined), line 86, characters 21-49
- Called from Ppx_expect_evaluator.create_group in file "evaluator/ppx_expect_evaluator.ml", line 113, characters 22-69
- Called from Base__List.count_map in file "src/list.ml", line 462, characters 13-17
- Called from Ppx_expect_evaluator.evaluate_tests in file "evaluator/ppx_expect_evaluator.ml", line 207, characters 2-85
- Called from Stdlib__List.map in file "list.ml", line 92, characters 20-23
- Called from Stdlib__List.map in file "list.ml", line 92, characters 32-39
- Called from Ppx_inline_test_lib__Runtime.exit in file "runtime-lib/runtime.ml", line 646, characters 2-49
- Called from Dune__exe__Inline_test_runner_test in file ".test.inline-tests/inline_test_runner_test.ml-gen", line 1, characters 9-44
+ File "test.re.ml", line 1, characters 0-0:
+ Error: Files _build/default/test.re.ml and
+ _build/default/test.re.ml.corrected differ.
[1] |
3ce0b45 to
be63d7f
Compare
|
I don't understand how this diff is supposed to work: We have a source file |
|
Okay, the issue is that our clone of refmt is bugged because it doesn't output line directives. Unfortunately, inserting the line directive to correct the filename breaks ppx_expect. No idea what's happening but I pushed the commit that demonstrates the issue. Anyway, we can investigate this later. Just remove the copy-pasting between the refmt's and this is good to go |
|
@rgrinberg Right, if you look at the commits, the last one I pushed fixes inline_tests. Before I get the same error as you did. |
|
It's not the same error. The previous error was expected because the refmt error is broken. The subsequent is because ppx_expect doesn't understand directive for some reason |
4e90f16 to
e96994e
Compare
|
@rgrinberg I've consolidated the fake refmt and squashed (fixedup) your commit. |
|
@rgrinberg Actually the backtrace in the bug you found isn't repro. My machine differs from the CI for some reason. Maybe we should leave the removing of the refmt dep to another PR and fix this inline tests issue? |
|
We can't have the test suite depend on refmt |
|
@rgrinberg Alright, shall I just scrub the backtrace then? |
|
The back trace is easily fixable we just need to remove the locations. That's not the point though, there's clearly something wrong with this fake refmt if it's making ppx_expect fail. We should fix it or it will break the test suite in other mysterious ways. |
e96994e to
cd12cee
Compare
|
If that's the best we can do, then it's acceptable. @anmonteiro can you take a look at refmt? Perhaps I'm missing something obvious. |
|
@rgrinberg You added this line to the fake refmt: That was intentional right? |
|
Yes. Without this, how would ppx_expect find the original source? |
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
This highlights a bug with inline_tests not respecting the dialect sources correctly. Signed-off-by: Ali Caglayan <alizter@gmail.com>
cd12cee to
8e81b06
Compare
|
What’s the refmt problem? Btw Reason can’t parse line directives, not sure if that’s related. |
|
We have this fake refmt doesn't need to understand line directives, it just needs to produce a parsetree with the correct locations. I think it does so with |
|
This looks like a ppx_expect bug rather than Reason? https://github.com/search?q=%22pos+%2B+len+past+end%22&type=code |
|
My guess: the ppx_expect evaluator is trying to read the new file expecting the old file contents length? |
|
Hmm I don't know how to progress with this now. |
CHANGES: - Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter) - Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more items. (ocaml/dune#8196, @Alizter) - Add `dune show installed-libraries` as an alias of the `dune installed-libraries` command. (ocaml/dune#8135, @Alizter) - Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193, @Alizter) - Add `dune build --dump-gc-stats FILE` argument to dump garbage collection stats to a named file. (ocaml/dune#8072, @Alizter) - Fix bug with ppx and Reason syntax due to missing dependency in sandboxed action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter) - Add `dune describe package-entries` to print all package entries (ocaml/dune#7480, @moyodiallo) - Improve `dune describe external-lib-deps` by adding the internal dependencies for more information. (ocaml/dune#7478, @moyodiallo) - Re-enable background file digests on Windows. The files are now open in a way that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
CHANGES: - Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter) - Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more items. (ocaml/dune#8196, @Alizter) - Add `dune show installed-libraries` as an alias of the `dune installed-libraries` command. (ocaml/dune#8135, @Alizter) - Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193, @Alizter) - Add `dune build --dump-gc-stats FILE` argument to dump garbage collection stats to a named file. (ocaml/dune#8072, @Alizter) - Fix bug with ppx and Reason syntax due to missing dependency in sandboxed action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter) - Add `dune describe package-entries` to print all package entries (ocaml/dune#7480, @moyodiallo) - Improve `dune describe external-lib-deps` by adding the internal dependencies for more information. (ocaml/dune#7478, @moyodiallo) - Re-enable background file digests on Windows. The files are now open in a way that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
CHANGES: - Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter) - Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more items. (ocaml/dune#8196, @Alizter) - Add `dune show installed-libraries` as an alias of the `dune installed-libraries` command. (ocaml/dune#8135, @Alizter) - Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193, @Alizter) - Add `dune build --dump-gc-stats FILE` argument to dump garbage collection stats to a named file. (ocaml/dune#8072, @Alizter) - Fix bug with ppx and Reason syntax due to missing dependency in sandboxed action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter) - Add `dune describe package-entries` to print all package entries (ocaml/dune#7480, @moyodiallo) - Improve `dune describe external-lib-deps` by adding the internal dependencies for more information. (ocaml/dune#7478, @moyodiallo) - Re-enable background file digests on Windows. The files are now open in a way that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
Fix #7930
ppx needs the original source of a file even if it is a dialect. This was missing causing the sandboxed version to error. We add the original source file as a hidden dependency in the ppx action.
I also found that inline_tests was not using the correct dialect source, which can be seen in my commits as I tried to remove my original reason dependency. I therefore have also submitted a fix for that.