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

Binary mode when writing to stdout on Windows #466

Closed
jonahbeckford opened this issue Jan 16, 2024 · 7 comments
Closed

Binary mode when writing to stdout on Windows #466

jonahbeckford opened this issue Jan 16, 2024 · 7 comments

Comments

@jonahbeckford
Copy link
Contributor

jonahbeckford commented Jan 16, 2024

TLDR: Windows should have the binary mode set, but that fix causes other issues.

Context

I'm using Dune in an unusual way that is exposing this ppxlib bug. Basically, since I want to use PPX-es with Dune from a bytecode-only environment, and because Dune requires a C compiler to build a (staged_pps) or (pps) driver, I have to resort to:

 (preprocess
  (action
   (run %{lib:TheNameOfTheLibrary:ppx.exe} -as-pp %{input-file})))

That in turn runs -as-pp which writes to stdout.

So:

# Works
Y:\source\dksdk-type\tests\_std\lib\DkSDKCoder_PPX\ppx.exe -as-pp .\tests\_std\expression.ml -o a.pp.ml

# Fails
Y:\source\dksdk-type\tests\_std\lib\DkSDKCoder_PPX\ppx.exe -as-pp .\tests\_std\expression.ml
Caml1999M031Fatal error: exception Failure("output_value: not a binary channel")
Raised by primitive operation at Stdlib.output_value in file "stdlib.ml", line 386, characters 26-54
Called from Ppxlib__Utils.Ast_io.write.(fun) in file "src/utils.ml", line 187, characters 8-34
Called from Ppxlib__Driver.process_file in file "src/driver.ml", line 1169, characters 6-159
Called from Ppxlib__Driver.standalone in file "src/driver.ml", line 1541, characters 9-27
Re-raised at Location.report_exception.loop in file "parsing/location.ml", line 938, characters 14-25
Called from Ppxlib__Driver.standalone in file "src/driver.ml", line 1544, characters 4-59
Called from Dune__exe___ppx in file ".ppx/6241d9fed22cd957d799b2636a4b50f0/_ppx.ml-gen", line 1, characters 9-36

Initial Attempt. OUTDATED and FIXED.

This now works. metapp fails if the ocamlfind version number of ppxlib is not exactly MAJOR.MINOR.PATCH

I tried to fix it myself in https://github.com/ocaml-ppx/ppxlib/pull/465/files but was unsuccessful. I now get:

File "metapp/dune", line 3, characters 14-30:
3 |   (preprocess (pps metapp.ppx))
                  ^^^^^^^^^^^^^^^^
(cd _build/default && .ppx/e7c203cacf1cd3bb02e6242329effa31/ppx.exe --cookie "library-name=\"metapp\"" -o metapp/metapp.pp.mli --intf metapp/metapp.mli -corrected-suffix .ppx-corrected -diff-cmd - -dump-ast)
Fatal error: exception Dynlink.Error (Dynlink.Library's_module_initializers_failed "File \"version_info/metapp_version_info.ml\", line 9, characters 9-15: Assertion failed")
Raised at Metapp_version_info.ppxlib_version in file "version_info/metapp_version_info.ml", line 9, characters 9-21
Called from Dynlink.Native.run.(fun) in file "otherlibs/dynlink/native/dynlink.ml", line 85, characters 12-29
Called from Dynlink.Native.run.(fun) in file "otherlibs/dynlink/native/dynlink.ml", line 85, characters 12-29
Re-raised at Dynlink.Native.run.(fun) in file "otherlibs/dynlink/native/dynlink.ml", line 87, characters 10-149
Called from Stdlib__List.iter in file "list.ml", line 110, characters 12-15
Called from Dynlink_common.Make.load.(fun) in file "otherlibs/dynlink/dynlink_common.ml", line 337, characters 13-44
Called from Stdlib__List.iter in file "list.ml", line 110, characters 12-15
Called from Dynlink_common.Make.load in file "otherlibs/dynlink/dynlink_common.ml", line 335, characters 8-240
Re-raised at Dynlink_common.Make.load in file "otherlibs/dynlink/dynlink_common.ml", line 345, characters 8-17
Called from Dynlink_common.Make.loadfile in file "otherlibs/dynlink/dynlink_common.ml" (inlined), line 347, characters 26-45
Called from Metapp_ppx.Options.handle.add_object_file in file "ppx/metapp_ppx.ml", line 44, characters 10-38
Called from Stdlib__List.filter_map.aux in file "list.ml", line 258, characters 14-17
Called from Metapp_ppx.Options.handle in file "ppx/metapp_ppx.ml", line 50, characters 30-106
Called from Stdlib__Option.bind in file "option.ml" (inlined), line 22, characters 53-56
Called from Metapp_ppx.transform.Metaopt.map.(fun) in file "ppx/metapp_ppx.ml", line 365, characters 12-69
Called from Ppxlib_ast__Ast_helper_lite.protect_ref.(fun) in file "ast/ast_helper_lite.ml", line 44, characters 10-14
Re-raised at Ppxlib_ast__Ast_helper_lite.protect_ref.(fun) in file "ast/ast_helper_lite.ml", line 50, characters 8-15
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 Metapp_ppx.transform in file "ppx/metapp_ppx.ml", line 382, characters 10-26
Called from Ppxlib__Driver.Transform.register.(fun) in file "src/driver.ml", line 190, characters 61-72
Called from Ppxlib__Driver.apply_transforms.(fun) in file "src/driver.ml", line 574, characters 20-28
Called from Stdlib__List.fold_left in file "list.ml", line 121, characters 24-34
Called from Ppxlib__Driver.apply_transforms in file "src/driver.ml", line 551, characters 6-1023
Called from Ppxlib__Driver.map_signature_gen in file "src/driver.ml", line 785, characters 4-336
Called from Ppxlib__Driver.process_ast in file "src/driver.ml", line 1066, characters 10-127
Called from Ppxlib__Driver.process_file in file "src/driver.ml", line 1120, characters 15-111
Called from Ppxlib__Driver.standalone in file "src/driver.ml", line 1541, characters 9-27
Re-raised at Location.report_exception.loop in file "parsing/location.ml", line 938, characters 14-25
Called from Ppxlib__Driver.standalone in file "src/driver.ml", line 1544, characters 4-59
Called from Dune__exe___ppx in file ".ppx/e7c203cacf1cd3bb02e6242329effa31/_ppx.ml-gen", line 1, characters 9-36

Second Attempt. MOVED TO MERLIN

Edit: Now at ocaml/merlin#1724

It gets past the initial problem so #465 looks fine. Now there is a new problem:

[Error - 4:50:25 PM] Request textDocument/codeAction failed.
  Message: uncaught exception
  Code: -32603 
[object Object]
detached: /-----------------------------------------------------------------------
| Internal error: Uncaught exception.
| Invalid_argument("Marshal.from_bytes")
| Raised at Stdlib.invalid_arg in file "stdlib.ml", line 30, characters 20-45
| Called from Stdlib__Marshal.from_string in file "marshal.ml" (inlined), line 67, characters 2-46
| Called from Merlin_kernel__Pparse.decode_potential_ast in file "src/ocaml/driver/pparse.ml", line 200, characters 14-47
| Called from Merlin_kernel__Mpipeline.process in file "src/kernel/mpipeline.ml", line 253, characters 10-120
| Called from CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 31, characters 17-27
| Re-raised at CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 36, characters 4-11
| Called from Merlin_kernel__Mpipeline.timed_lazy in file "src/kernel/mpipeline.ml", line 17, characters 10-22
| Re-raised at Merlin_kernel__Mpipeline.timed_lazy in file "src/kernel/mpipeline.ml", line 19, characters 34-49
| Called from CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 31, characters 17-27
| Re-raised at CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 36, characters 4-11
| Called from Merlin_kernel__Mpipeline.input_source in file "src/kernel/mpipeline.ml", line 111, characters 25-46
| Called from Merlin_kernel__Mpipeline.with_pipeline.(fun) in file "src/kernel/mpipeline.ml", line 115, characters 39-55
| Called from Stdlib__Fun.protect in file "fun.ml", line 33, characters 8-15
| Re-raised at Stdlib__Fun.protect in file "fun.ml", line 38, characters 6-52
| Called from Merlin_kernel__Mocaml.with_state in file "src/kernel/mocaml.ml", line 18, characters 8-38
| Re-raised at Merlin_kernel__Mocaml.with_state in file "src/kernel/mocaml.ml", line 20, characters 42-53
| Called from Ocaml_lsp_server__Document.Single_pipeline.use.(fun) in file "ocaml-lsp-server/src/document.ml", line 147, characters 22-77
| Called from Stdune__Exn_with_backtrace.try_with in file "otherlibs/stdune/src/exn_with_backtrace.ml", line 9, characters 8-12
| Re-raised at Stdune__Exn.raise_with_backtrace in file "otherlibs/stdune/src/exn.ml" (inlined), line 38, characters 27-56
| Called from Stdune__Exn_with_backtrace.reraise in file "otherlibs/stdune/src/exn_with_backtrace.ml", line 20, characters 33-71
| Called from Fiber__Core.O.(>>|).(fun) in file "fiber/src/core.ml", line 250, characters 36-41
| Called from Fiber__Scheduler.exec in file "fiber/src/scheduler.ml", line 73, characters 8-11
\-----------------------------------------------------------------------

Pulling out my hair trying to find if this is a writing problem (ppxlib?) or a reading problem (merlin).

I fixed what I could in ocaml/merlin#1723 but still there is an open problem with either ppxlib or merlin here.

@pitag-ha
Copy link
Member

Thanks a lot, @jonahbeckford , both for the issue, and for already starting to fix it! The error you're getting now seems specific to the metapp PPX, doesn't it? Do you also get it when using other PPXs?

@jonahbeckford
Copy link
Contributor Author

jonahbeckford commented Jan 29, 2024

The error you're getting now seems specific to the metapp PPX, doesn't it? Do you also get it when using other PPXs?

Oops. I didn't update this ticket after I found the root cause. I did update this ticket, but I didn't make the update prominent. Fixing that now.

Root cause for metapp: it does not like local development of ppxlib since it parses the findlib reported ppxlib version and expects MAJOR.MINOR.PATH. With a local ppxlib findlib reports a git commit id based version number.

@jonahbeckford
Copy link
Contributor Author

jonahbeckford commented Jan 29, 2024

In the description I posted the next problem I encountered. It might belong elsewhere, but AFAIK there is a 50/50 chance it is a ppxlib issue (perhaps addressed by your PR comments). For visibility:

I fixed what I could in ocaml/merlin#1723 but still there is an open problem with either ppxlib or merlin here.

It would make things much easier/quicker if someone who knew ppxlib could verify that the marshalled AST is being written correctly on Windows. Perhaps a test case with a known PPX that used -dump-ast or -as-pp? I realize that many people don't have access to a Windows machine, so this may not be possible.

@pitag-ha
Copy link
Member

From the ppxlib side, this is fixed once #465 is merged, right? The other two problems seem to be on metapp and merlin IIUC. Btw, have you opened an issue on metapp as well?

@jonahbeckford
Copy link
Contributor Author

From the ppxlib side, this is fixed once #465 is merged, right? The other two problems seem to be on metapp and merlin IIUC. Btw, have you opened an issue on metapp as well?

I don't consider metapp to have a bug; it is just unusual. Yes, the two merlin issues have been opened and are closed/being-reviewed.

@NathanReb
Copy link
Collaborator

I guess we can close this now that #465 has been merged. Do do you confirm?

NathanReb added a commit to NathanReb/opam-repository that referenced this issue Feb 1, 2024
CHANGES:

- Fix `Longident.parse` so it properly handles unparenthesized dotted operators
  such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb)

- raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1)

- Sort embedded errors that are appended to the AST by location so the compiler
  reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb)

- Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund)

- Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo)

- Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund)

- Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund)

- When a transformation raises, the last valid AST is used as input to the upcoming
  transformations. All such errors are collected and appended as
  extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1)

- Fix a small mistake in the man pages: Embededding errors is done by default with
  `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha)

- Set appropriate binary mode when writing to `stdout` especially for Windows
  compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)
@jonahbeckford
Copy link
Contributor Author

Closing. Thanks!

NathanReb added a commit to NathanReb/opam-repository that referenced this issue Feb 5, 2024
CHANGES:

- Add an optional `embed_errors` argument to `Context_free.map_top_down` that
  controls how to deal with exceptions thrown by context-free rules.
  (ocaml-ppx/ppxlib#468, @NathanReb)

- Fix `Longident.parse` so it properly handles unparenthesized dotted operators
  such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb)

- raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1)

- Sort embedded errors that are appended to the AST by location so the compiler
  reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb)

- Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund)

- Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo)

- Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund)

- Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund)

- When a transformation raises, the last valid AST is used as input to the upcoming
  transformations. All such errors are collected and appended as
  extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1)

- Fix a small mistake in the man pages: Embededding errors is done by default with
  `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha)

- Set appropriate binary mode when writing to `stdout` especially for Windows
  compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)
nberth pushed a commit to nberth/opam-repository that referenced this issue Jun 18, 2024
CHANGES:

- Add an optional `embed_errors` argument to `Context_free.map_top_down` that
  controls how to deal with exceptions thrown by context-free rules.
  (ocaml-ppx/ppxlib#468, @NathanReb)

- Fix `Longident.parse` so it properly handles unparenthesized dotted operators
  such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb)

- raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1)

- Sort embedded errors that are appended to the AST by location so the compiler
  reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb)

- Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund)

- Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo)

- Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund)

- Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund)

- When a transformation raises, the last valid AST is used as input to the upcoming
  transformations. All such errors are collected and appended as
  extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1)

- Fix a small mistake in the man pages: Embededding errors is done by default with
  `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha)

- Set appropriate binary mode when writing to `stdout` especially for Windows
  compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)
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

No branches or pull requests

3 participants