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

test(melange): show crash when melange.emit depends on transitive PPX #7948

Merged

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Jun 12, 2023

@anmonteiro anmonteiro force-pushed the anmonteiro/melange-transitive-ppx-crash branch 3 times, most recently from b6f9ef9 to 5f94231 Compare June 12, 2023 23:05
@anmonteiro anmonteiro requested a review from rgrinberg June 12, 2023 23:05
@@ -469,7 +469,6 @@ let setup_emit_js_rules ~dir_contents ~dir ~scope ~sctx mel =
let* modules_for_js, _obj_dir =
modules_for_js_and_obj_dir ~sctx ~dir_contents ~scope mel
in
Resolve.push_frames resolve_error @@ fun () ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why the error is fixed by not pushing the stack frames.

For reference, the error is:

  Description:
    ("Implicit_output.produce called without any handler in dynamic scope",
    { type = "Rules" })
  Raised at Stdune__Code_error.raise in file
    "otherlibs/stdune/src/code_error.ml", line 11, characters 30-62
  Called from Fiber__Core.O.(>>|).(fun) in file "otherlibs/fiber/src/core.ml",
    line 250, characters 36-41

    ...

  -> required by ("<unnamed>", ())
  -> required by ("<unnamed>", ())
  -> required by ("<unnamed>", ())
  -> required by ("<unnamed>", ())
  -> required by
     ("load-dir", In_build_dir "default/lib/test/dist/lib/test/.dist.mobjs")
  -> required by
     ("build-file",
     In_build_dir "default/lib/test/dist/lib/test/.dist.mobjs/melange.js")
  -> required by ("<unnamed>", ())
  -> required by
     ("build-alias", { dir = In_build_dir "default/lib/test"; name = "all" })
  -> required by ("<unnamed>", ())
  -> required by
     ("build-alias", { dir = In_build_dir "default"; name = "default" })
  -> required by ("toplevel", ())

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, i think we need to go to the old approach of shoving the backtrace inside the Action_builder.fail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pushed a new version that exposes Report_error.format_memo_stack and uses it to format the error stack trace.

@anmonteiro anmonteiro force-pushed the anmonteiro/melange-transitive-ppx-crash branch 2 times, most recently from ce10956 to e430f22 Compare June 14, 2023 20:28
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-transitive-ppx-crash branch from e430f22 to f42f4d3 Compare June 19, 2023 05:08
@anmonteiro
Copy link
Collaborator Author

@rgrinberg could you give this one a look when you have a chance?

@rgrinberg rgrinberg added this to the 3.9.0 milestone Jun 21, 2023
@anmonteiro anmonteiro merged commit a873b08 into ocaml:main Jun 21, 2023
@anmonteiro anmonteiro deleted the anmonteiro/melange-transitive-ppx-crash branch June 21, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants