janestreet / base

Standard library for OCaml
MIT License
848 stars 124 forks source link

Use _BitScanReverse64 only on _WIN64 #128

Closed jonahbeckford closed 2 years ago

jonahbeckford commented 2 years ago

This is a continuation of your 32-bit Windows fix for _BitScanForward64 introduced in v0.15~preview.123.17+175 in https://github.com/janestreet/base/commit/2e594935c42fe76c53fe43894404b7295ef8d881

Note: This one bug had a large blast radius; without it Windows 32-bit builds can't use the PPX ecosystem (see Testing below). If needed ping me if you'd like to discuss some way to collaborate on (or offload) Win32 testing ... 32 or 64 bit ... before each major Jane Street release.

Testing

I don't have a good way to test v0.15 preview, so I backported your original _BitScanForward64 fix to v0.14.x with a Windows DiskuvOCaml Opam repository commit.

Then, before the PR I would get the following (full logs at https://github.com/diskuv/dkml-installer-ocaml/runs/5338547589?check_suite_focus=true):

#=== ERROR while compiling ppx_assert.v0.14.0 =================================#
# context              2.1.0 | win32/x86_64 |  | [https://opam.ocaml.org#88df89b9](https://opam.ocaml.org/#88df89b9)
# path                 D:\.opam\installer-ocaml\.opam-switch\build\ppx_assert.v0.14.0
# command              D:\.opam\installer-ocaml\bin\dune.exe build -p ppx_assert -j 1
# exit-code            1
# env-file             D:\.opam\log\ppx_assert-4596-a0c928.env
# output-file          D:\.opam\log\ppx_assert-4596-a0c928.out
### output ###
# [...]
# -> required by _build/default/ppx_assert.install
# -> required by alias install
# (cd _build/default && D:\.opam\installer-ocaml\bin\ocamlopt.opt.exe -g -w -24 -o .ppx/9f511ed6dda78ec325c1425b89003f2a/ppx.exe D:/.opam/installer-ocaml/lib/ocaml\compiler-libs\ocamlcommon.cmxa D:\.opam\installer-ocaml\lib\ocaml-compiler-libs\common/ocaml_common.cmxa D:\.opam\installer-ocaml\lib\ppxlib\astlib/astlib.cmxa D:\.opam\installer-ocaml\lib\stdlib-shims\stdlib_shims.cmxa D:\.opam\inst[...]
# ** Cannot resolve symbols for D:\.opam\installer-ocaml\lib\base\libbase_stubs.lib(src\int_math_stubs.obj):
#  __BitScanForward64
#  __BitScanReverse64
# File "caml_startup", line 1:
# Error: Error during linking (exit code 2)
# -> required by _build/default/.ppx/9f511ed6dda78ec325c1425b89003f2a/ppx.exe
# -> required by _build/install/default/lib/ppx_assert/ppx.exe
# -> required by _build/default/ppx_assert.install
# -> required by alias install
...
...
+- The following actions failed
| - build ppx_assert   v0.14.0
| - build ppx_hash     v0.14.0
| - build ppx_js_style v0.14.1
+- 

After the PR (https://github.com/diskuv/diskuv-opam-repository/commit/df8dd009ac5d7c7cfd6d0e1137eb109035102f03) the ppx packages build successfully.

bcc32 commented 2 years ago

Thanks, I've pulled this into our code review system. I'll ask the devs who do the releases to see what the thoughts on Windows testing are.