Skip to content

"rescript build" crashes on Windows #5517

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

Closed
cknitt opened this issue Jul 6, 2022 · 8 comments · Fixed by #5516
Closed

"rescript build" crashes on Windows #5517

cknitt opened this issue Jul 6, 2022 · 8 comments · Fixed by #5516
Labels
Milestone

Comments

@cknitt
Copy link
Member

cknitt commented Jul 6, 2022

With the npm package for 10.0.0-alpha.1 (and the package tarball created by CI in current master), the behavior on Windows is as follows with the package installed in a project:

npx rescript build causes a crash:

npm WARN config global `--global`, `--local` are deprecated. Use `--location=global` instead.
/c/hostedtoolcache/windows/node/16.15.1/x64/npx: line 48:   331 Segmentation fault      "$NODE_EXE" "$NPX_CLI_JS" "$@"

This can be observed e.g. here: https://github.com/rescript-lang/rescript-compiler/runs/7209391826?check_suite_focus=true

Compiling a file directly with bsc works fine though, as does npx rescript -h:

npm WARN config global `--global`, `--local` are deprecated. Use `--location=global` instead.
Usage: rescript <options> <subcommand>
`rescript` is equivalent to `rescript build`
Options:
  -v, -version  display version number
  -h, -help     display help
Subcommands:
  init
  build
  clean
  format
  convert
  dump
  help
Run `rescript <subcommand> -h` for subcommand help. Examples:
  rescript build -h
  rescript format -h
The default `rescript` is equivalent to `rescript build` subcommand  
@cknitt cknitt added this to the v10.0 milestone Jul 6, 2022
@cknitt cknitt mentioned this issue Jul 6, 2022
5 tasks
@cknitt cknitt added the bug label Jul 6, 2022
@bobzhang
Copy link
Member

bobzhang commented Jul 8, 2022

do you have a local windows box? Is the crash coming from ninja.exe or bsc.exe/bsb_helper.exe? The log is not very informative.
If it comes from ninja.exe, can you replace it with the old one and still see the issue?

if you don't have windows box, maybe you can also run npx rescript build --verbose to see if there contains more info.

Is the new ninja.exe built with MSVC or MingW?The old one is built with MSVC, I had some bad experience with Mingw

@cknitt
Copy link
Member Author

cknitt commented Jul 8, 2022

The new ninja.exe is built with MSVC (see https://github.com/rescript-lang/ninja/blob/rescript/.github/workflows/ci.yml#L28).

I do have a Windows VM only, and for some reason it got so excruciatingly slow recently that I can hardly work on it. 😞

I experimented a bit more yesterday though, and if I am not mistaken, then it is actually rescript.exe which crashes before (or when?) writing lib\bs\build.ninja.

@bobzhang
Copy link
Member

bobzhang commented Jul 8, 2022

There is a hidden flag that tells rescript.exe only to generate ninja file: https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/main/rescript_main.ml#L150

@cknitt
Copy link
Member Author

cknitt commented Jul 8, 2022

This is what I get (with or without the -regen):

$ ./node_modules/rescript/win32/rescript.exe build -verbose
BSB check build spec : Dependencies information missing
Segmentation fault

(That is when running in cygwin. When running in cmd.exe, "Segmentation fault" is not displayed.)

The lib or lib\bs dir, if missing, is not created.

@cknitt
Copy link
Member Author

cknitt commented Jul 8, 2022

I now have a setup where I can at least make changes to rescript.ml on Windows and compile them rather quickly.

I don't really know how to debug OCaml compiled to native code, so I just added some log statements until I found that the crash occurs when parsing bsconfig.json, at this line: https://github.com/rescript-lang/rescript-compiler/blob/16dceccc07245debf5d4e7ff0f4e5e1af245759b/jscomp/bsb/bsb_parse_sources.ml#L239

With my local paths, this is when calling:

Ext_sys.is_directory_no_exn "G:\\packages\\test\\src\\Test.res"

Ext_sys.is_directory_no_exn is an external definition pointing to the C function caml_sys_is_directory_no_exn.

@cknitt
Copy link
Member Author

cknitt commented Jul 8, 2022

What's weird is that this is basically the same code as in https://github.com/ocaml/ocaml/blob/af13e29e0499d479035b064d9d70d9614e76a4fe/runtime/sys.c#L288.

There is a compiler warning, however:

../jscomp/stubs/ext_basic_hash_stubs.c: In function ‘caml_sys_is_directory_no_exn’:
../jscomp/stubs/ext_basic_hash_stubs.c:199:5: warning: assignment to ‘char_os *’ {aka ‘short unsigned int *’} from incompatible pointer type ‘caml_stat_string’ {aka ‘char *’}
-Wincompatible-pointer-types]
  199 |   p = caml_stat_strdup(String_val(name));
      |     ^

@cknitt
Copy link
Member Author

cknitt commented Jul 8, 2022

Ah no, I had already tried to make some changes there, actually without modification the warning is

In file included from C:/OCaml64/home/chris/.opam/4.14.0+mingw64c/lib/ocaml/caml/mlvalues.h:23,
                 from C:/OCaml64/home/chris/.opam/4.14.0+mingw64c/lib/ocaml/caml/hash.h:21,
                 from ../jscomp/stubs/ext_basic_hash_stubs.c:1:
../jscomp/stubs/ext_basic_hash_stubs.c: In function ‘caml_sys_is_directory_no_exn’:
C:/OCaml64/home/chris/.opam/4.14.0+mingw64c/lib/ocaml/caml/misc.h:339:32: warning: implicit declaration of function ‘caml_stat_strdup_to_utf16’; did you mean ‘caml_stat_strdup_
to_os’? [-Wimplicit-function-declaration]
  339 | #define caml_stat_strdup_to_os caml_stat_strdup_to_utf16
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~
../jscomp/stubs/ext_basic_hash_stubs.c:198:7: note: in expansion of macro ‘caml_stat_strdup_to_os’
  198 |   p = caml_stat_strdup_to_os(String_val(name));
      |       ^~~~~~~~~~~~~~~~~~~~~~
../jscomp/stubs/ext_basic_hash_stubs.c:198:5: warning: assignment to ‘char_os *’ {aka ‘short unsigned int *’} from ‘int’ makes pointer from integer without a cast [-Wint-conver
sion]
  198 |   p = caml_stat_strdup_to_os(String_val(name));
      |     ^

@cknitt
Copy link
Member Author

cknitt commented Jul 8, 2022

If I add

#include "caml/osdeps.h"

then the warnings are gone, and the crash is, too!

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 a pull request may close this issue.

2 participants