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

Dune 3.11 introduces regression on macOS with regarding to executable promotion #9272

Closed
haochenx opened this issue Nov 22, 2023 · 12 comments · Fixed by #10263 or ocaml/opam-repository#25615
Assignees
Labels

Comments

@haochenx
Copy link
Contributor

haochenx commented Nov 22, 2023

Expected Behavior

Promoted executable runs fine.

Actual Behavior

Promoted executable getting killed by OS with signal 9.
Directly executing executable in _build folder or executing a manually copied version of the executable behave as expected.

Reproduction

See the minimal reproducing project at https://github.com/kxcinc/sample.bc47-dune-reproduce

Specifications

  • Version of dune (output of dune --version): 3.11.0 (also reproduced on 3.11.1)
  • Version of ocaml (output of ocamlc --version): 5.1.0 (also reproduced on 4.14.0 ~ 5.0.0)
  • Operating system (distribution and version): macOS 14.1.1 (Apple Silicon M1 Pro & M2)

Additional information

output of `dune build --verbose`:
Shared cache: disabled
Workspace root: /Users/hx/git/sample.bc47-dune-reproduce
Auto-detected concurrency: 8
Dune context:
 { name = "default"
 ; kind = "default"
 ; profile = Dev
 ; merlin = true
 ; for_host = None
 ; fdo_target_exe = None
 ; build_dir = In_build_dir "default"
 ; ocaml_bin = External "/Users/hx/git/sample.bc47-dune-reproduce/_opam/bin"
 ; ocaml =
     Ok External "/Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocaml"
 ; ocamlc =
     External "/Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamlc.opt"
 ; ocamlopt =
     Ok
       External
         "/Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamlopt.opt"
 ; ocamldep =
     Ok
       External
         "/Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamldep.opt"
 ; ocamlmklib =
     Ok
       External
         "/Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamlmklib"
 ; installed_env =
     map
       { "INSIDE_DUNE" :
           "/Users/hx/git/sample.bc47-dune-reproduce/_build/default"
       ; "OCAML_COLOR" : "always"
       ; "OPAMCOLOR" : "always"
       }
 ; findlib_paths =
     [ External "/Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/ocaml"
     ; External "/Users/hx/git/sample.bc47-dune-reproduce/_opam/lib"
     ]
 ; ocaml_config =
     { version = "5.1.0"
     ; standard_library_default =
         "/Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/ocaml"
     ; standard_library =
         "/Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/ocaml"
     ; standard_runtime = "the_standard_runtime_variable_was_deleted"
     ; ccomp_type = "cc"
     ; c_compiler = "cc"
     ; ocamlc_cflags =
         [ "-O2"; "-fno-strict-aliasing"; "-fwrapv"; "-pthread" ]
     ; ocamlc_cppflags = [ "-D_FILE_OFFSET_BITS=64" ]
     ; ocamlopt_cflags =
         [ "-O2"; "-fno-strict-aliasing"; "-fwrapv"; "-pthread" ]
     ; ocamlopt_cppflags = [ "-D_FILE_OFFSET_BITS=64" ]
     ; bytecomp_c_compiler =
         [ "cc"
         ; "-O2"
         ; "-fno-strict-aliasing"
         ; "-fwrapv"
         ; "-pthread"
         ; "-D_FILE_OFFSET_BITS=64"
         ]
     ; bytecomp_c_libraries =
         [ "-L/opt/homebrew/Cellar/zstd/1.5.5/lib"; "-lzstd"; "-lpthread" ]
     ; native_c_compiler =
         [ "cc"
         ; "-O2"
         ; "-fno-strict-aliasing"
         ; "-fwrapv"
         ; "-pthread"
         ; "-D_FILE_OFFSET_BITS=64"
         ]
     ; native_c_libraries =
         [ "-L/opt/homebrew/Cellar/zstd/1.5.5/lib"; "-lzstd"; "-lpthread" ]
     ; native_pack_linker = [ "ld"; "-r"; "-o" ]
     ; cc_profile = []
     ; architecture = "arm64"
     ; model = "default"
     ; int_size = 63
     ; word_size = 64
     ; system = "macosx"
     ; asm = [ "cc"; "-c"; "-Wno-trigraphs" ]
     ; asm_cfi_supported = true
     ; with_frame_pointers = false
     ; ext_exe = ""
     ; ext_obj = ".o"
     ; ext_asm = ".s"
     ; ext_lib = ".a"
     ; ext_dll = ".so"
     ; os_type = "Unix"
     ; default_executable_name = "a.out"
     ; systhread_supported = true
     ; host = "aarch64-apple-darwin23.1.0"
     ; target = "aarch64-apple-darwin23.1.0"
     ; profiling = false
     ; flambda = false
     ; spacetime = false
     ; safe_string = true
     ; exec_magic_number = "Caml1999X033"
     ; cmi_magic_number = "Caml1999I033"
     ; cmo_magic_number = "Caml1999O033"
     ; cma_magic_number = "Caml1999A033"
     ; cmx_magic_number = "Caml1999Y033"
     ; cmxa_magic_number = "Caml1999Z033"
     ; ast_impl_magic_number = "Caml1999M033"
     ; ast_intf_magic_number = "Caml1999N033"
     ; cmxs_magic_number = "Caml1999D033"
     ; cmt_magic_number = "Caml1999T033"
     ; natdynlink_supported = true
     ; supports_shared_libraries = true
     ; windows_unicode = false
     }
 ; instrument_with = []
 }
Actual targets:
- alias @@default
Running[1]: (cd _build/default && /Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamlopt.opt -I .hello.eobjs/byte -I .hello.eobjs/native -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-site -intf-suffix .ml-gen -no-alias-deps -opaque -o .hello.eobjs/native/dune_site__Dune_site_data.cmx -c -impl .hello.eobjs/dune_site__Dune_site_data.ml-gen)
Running[2]: (cd _build/default && /Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamlc.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -bin-annot -I .hello.eobjs/byte -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camlp-streams -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camomile -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camomile/lib -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-private-libs/dune-section -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-site -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-site/private -no-alias-deps -opaque -o .hello.eobjs/byte/dune__exe__Hello.cmo -c -impl hello.ml)
Running[3]: (cd _build/default && /Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamlopt.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -I .hello.eobjs/byte -I .hello.eobjs/native -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camlp-streams -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camomile -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camomile/lib -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-private-libs/dune-section -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-site -I /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-site/private -intf-suffix .ml -no-alias-deps -opaque -o .hello.eobjs/native/dune__exe__Hello.cmx -c -impl hello.ml)
Running[4]: (cd _build/default && /Users/hx/git/sample.bc47-dune-reproduce/_opam/bin/ocamlopt.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -o hello.exe -linkall /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-private-libs/dune-section/dune_section.cmxa /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-site/private/dune_site_private.cmxa .hello.eobjs/native/dune_site__Dune_site_data.cmx /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/dune-site/dune_site.cmxa /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camlp-streams/camlp_streams.cmxa /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camomile/lib/camomileLib.cmxa /Users/hx/git/sample.bc47-dune-reproduce/_opam/lib/camomile/camomile.cmxa .hello.eobjs/native/dune__exe__Hello.cmx)
Promoting "_build/default/hello.exe" to "hello.exe"
@haochenx haochenx changed the title Dune 3.11 introduce regression on macOS with regarding to executable promotion Dune 3.11 introduces regression on macOS with regarding to executable promotion Nov 23, 2023
@emillon
Copy link
Collaborator

emillon commented Nov 23, 2023

@haochenx Thanks for the bug report. The fact that it's coming from camomile (presumably 2.0.0) which is the exact version fixed in #8361 points again at that PR.

@anmonteiro The opam instructions for camomile.2.0.0 have an explicit dune install step that's different from the classic dune subst / dune build -p @install one (this is because of the sites extension).
In #8361, were you using buildDunePackage, or something that interprets the opam build instructions? If that's the case I think that we should try using dune install in the nix instructions instead of the fix that's merged (and presumably does not work for non-nix users).

@emillon emillon mentioned this issue Nov 23, 2023
26 tasks
@haochenx
Copy link
Contributor Author

haochenx commented Nov 23, 2023

@emillon thanks for the triage. I hope it can be reproduced on your side. I'm happy to help running additional diagnosis if that helps.

(btw I just spot an incorrect commenting-out of the reproduce.sh in the minimal reproduction example project and fixed it (kxcinc/sample.bc47-dune-reproduce@0550f9d). I hope the broken version didn't hurt your setup too much.. 🙏 )

@emillon
Copy link
Collaborator

emillon commented Nov 23, 2023

I don't have an arm mac available at the moment, so yes I would be interested in some testing.
Can you try the following versions and tell me if the issue is present:

  • 3.12.0~alpha3, using opam pin add dune.3.12.0~alpha3 https://github.com/ocaml/dune/releases/download/3.12.0_alpha3/dune-3.12.0.alpha3.tbz
  • my revert-8361 branch, using opam pin add dune git+https://github.com/emillon/dune#revert-8361
  • main, using opam pin add dune --dev-repo

Thanks!

@haochenx
Copy link
Contributor Author

haochenx commented Nov 24, 2023

I don't have an arm mac available at the moment, so yes I would be interested in some testing.
Can you try the following versions and tell me if the issue is present:

Sure! Here's the result:

  • 3.12.0~alpha3 still have the issue
  • revert-8361 fixes it

@emillon
Copy link
Collaborator

emillon commented Nov 24, 2023

Thanks. My impression for now is that the correct fix is revert-8361 and that the fix in #8361 was too specific to nix, and maybe to an issue caused by wrong build instructions. Let's wait for @anmonteiro's opinion.

@emillon
Copy link
Collaborator

emillon commented Nov 24, 2023

Specifically, camomile.2.0.0 installs a Sites.ml which contains some placeholders, so I suppose they need to be substituted even if the file is not executable.

@haochenx
Copy link
Contributor Author

Thanks!

My impression for now is that the correct fix is revert-8361

Do you think we should mark Dune 3.11.x as incompatible with macOS on opam-repository?
Or if we don't want to do that we can probably just add a post-message to warn the user (I don't think it's ideal though)
@emillon

@emillon
Copy link
Collaborator

emillon commented Nov 27, 2023

Ah I didn't realize it was already broken in 3.11. This means it shouldn't block 3.12. It needs to be fixed, though, but this also means that we can fix it in a point release.

Do you think we should mark Dune 3.11.x as incompatible with macOS on opam-repository?

I'll try to write a regression test to see exactly what is wrong. I think that it would probably be dune-site being unavailable in macos arm, but I want to make sure that it's always a problem. If this only happens if there's in addition a promote rule, marking it as unavailable would prevent legitimate uses.

@haochenx
Copy link
Contributor Author

We discussed this during the opam-repo maintainer's meeting a bit. The conclusion was that there's probably very little we can do to precisely disable the installation of affected versions for only the users that may experience this problem (no surprise), and it only affects a tiny percentage of users, so we don't want to mark 3.11.x unavailable on macOS for all the other users.

I will do some testing about making dune-site.3.11.x unavailable on macOS will help alleviate this issue without affecting the majority of Dune users.

@Alizter Alizter added the macos label Dec 5, 2023
@emillon
Copy link
Collaborator

emillon commented Mar 12, 2024

I now have a M1 mac and I was able to reproduce the issue from the linked repo.

@emillon
Copy link
Collaborator

emillon commented Mar 12, 2024

and I confirm that I bisect the issue to #8361

emillon added a commit to emillon/dune that referenced this issue Mar 12, 2024
emillon added a commit to emillon/dune that referenced this issue Mar 12, 2024
emillon added a commit to emillon/dune that referenced this issue Mar 13, 2024
Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this issue Mar 13, 2024
Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this issue Mar 13, 2024
Signed-off-by: Etienne Millon <me@emillon.org>
@haochenx
Copy link
Contributor Author

Thanks for tackling this! Please know if there's anything you'd like me to help.

emillon added a commit to emillon/dune that referenced this issue Mar 13, 2024
Fixes ocaml#9272

The fix in ocaml#8361 relies on a `~executable` argument that was not passed
in the promotion case. Instead, we call `stat` in `Conf.run_sign_hook`.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this issue Mar 14, 2024
* fix(codesign): run hook for promoted executables

Fixes #9272

The fix in #8361 relies on a `~executable` argument that was not passed
in the promotion case. Instead, we call `stat` in `Conf.run_sign_hook`.

Signed-off-by: Etienne Millon <me@emillon.org>
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Mar 26, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this issue Apr 3, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment