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

Nix + macOS codesigning: -f flag needed #6265

Closed
anmonteiro opened this issue Oct 19, 2022 · 16 comments · Fixed by #7183 or ocaml/opam-repository#23814
Closed

Nix + macOS codesigning: -f flag needed #6265

anmonteiro opened this issue Oct 19, 2022 · 16 comments · Fixed by #7183 or ocaml/opam-repository#23814
Assignees
Milestone

Comments

@anmonteiro
Copy link
Collaborator

    The error I get without `-f` is:
ocaml5.0.0+alpha1-conan-cli> libc++abi: terminating with uncaught exception of type std::runtime_error: file is already signed. pass -f to sign regardless.

Originally posted by @anmonteiro in #6260 (comment)

@emillon emillon self-assigned this Oct 19, 2022
@emillon
Copy link
Collaborator

emillon commented Oct 19, 2022

Thanks! Can you see this error in the test suite? If not, I'd be interested in a repro case if you can.

@anmonteiro
Copy link
Collaborator Author

You can reproduce it with any already signed binary:

$ nix shell nixpkgs#darwin.sigtool
$ command -v codesign
/nix/store/31x5zrydp6vygqx218hipi81c80n7lc1-sigtool-0.1.2/bin/codesign
$ cp /usr/bin/file ./random-file
$ codesign -s - ./random-file
libc++abi: terminating with uncaught exception of type std::runtime_error: file is already signed. pass -f to sign regardless.
[1]    25473 abort      codesign -s - ./random-file
$ codesign -s - -f ./random-file

@emillon
Copy link
Collaborator

emillon commented Oct 19, 2022

I meant in the context of a dune build.

@anmonteiro
Copy link
Collaborator Author

Created a repro here (https://github.com/anmonteiro/dune-repro). Just using dune-site triggers the issue, as I assume dune does substitutions there for sourceroot.

Happy to walk you through how to modify / set up a new test version. Should be as easy as modifying the dune_3 src in flake.nix: https://github.com/anmonteiro/dune-repro/blob/6c3e94c8d976c35238acf6cbac133c01728cdfeb/flake.nix#L14-L18

You can probably add /Users/username/path/to/dune instead, and try a local development version. You might just need to pass --impure for nix but it'll warn you.

@emillon emillon added this to the 3.6.0 milestone Nov 4, 2022
@emillon
Copy link
Collaborator

emillon commented Nov 9, 2022

I don't have access to a M1 machine at the moment (should be the case later) so I won't be able to fix that for 3.6 but that should be doable for 3.7.

Are you fine with patching in the meantime @anmonteiro ?

(For context, we can't just add -f to the flags in dune because this adds different output in vanilla macos and in macos+nix.)

@rgrinberg
Copy link
Member

(For context, we can't just add -f to the flags in dune because this adds different output in vanilla macos and in macos+nix.)

Why is that an issue?

@emillon
Copy link
Collaborator

emillon commented Nov 9, 2022

The test suite is going to fail on one and succeed on the other, so we need some kind of postprocessing if we go that route.
The alternative is making sure we never call codesign on an already signed binary, which I thought was the case.

@rgrinberg
Copy link
Member

The test suite already fails on nixos for many other unrelated reasons.

@rgrinberg
Copy link
Member

Btw, which test case do you have in mind?

@emillon
Copy link
Collaborator

emillon commented Nov 9, 2022

I don't have a mac machine to test at the moment, but (in the non-nix case) if you add -f to the list of flags, there are some places where codesign output appears.

@rgrinberg
Copy link
Member

Okay, then I think basically both options seem similar. Either the nix distribution patches the binary or the output of the tests. @anmonteiro what do you prefer?

@emillon
Copy link
Collaborator

emillon commented Nov 9, 2022

Part of the issue seems that -f seems to have different semantics on the two codesign tools. When we call codesign in dune, the binary already has a signature, so I'm not sure why -f is not necessary and there's no "replacing a signature" log output.

@anmonteiro
Copy link
Collaborator Author

I'm happy to continue patching on my end, but might be a little selfish, as only folks using my overlays will get the fix.

@emillon
Copy link
Collaborator

emillon commented Nov 14, 2022

Great! I do want to solve this for everybody but I don't have the hardware to make it happen for 3.6, but that should be fine for 3.7. Thanks!

@emillon emillon modified the milestones: 3.6.0, 3.7.0 Nov 14, 2022
@emillon
Copy link
Collaborator

emillon commented Jan 30, 2023

We're going to have a look at this with @voodoos soon.

emillon added a commit to voodoos/dune that referenced this issue Feb 1, 2023
We add `-f` to the list of flags passed to `codesign`.
In some cases, the binary already has a signature so the `codesign` tool
from Apple prints some error messages on stderr. We filter out these
error messages as they are innocuous. In addition, this ensures that the
test suite has the same output on macos and Linux.

Fixes ocaml#6265

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <me@emillon.org>
emillon added a commit to voodoos/dune that referenced this issue Feb 1, 2023
We add `-f` to the list of flags passed to `codesign`.
In some cases, the binary already has a signature so the `codesign` tool
from Apple prints some error messages on stderr. We filter out these
error messages as they are innocuous. In addition, this ensures that the
test suite has the same output on macos and Linux.

Fixes ocaml#6265

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Etienne Millon <me@emillon.org>
Co-authored-by: Etienne Millon <me@emillon.org>
@emillon emillon modified the milestones: 3.7.0, 3.8.0 Feb 1, 2023
@greedy
Copy link
Contributor

greedy commented Feb 24, 2023

This already causes a failure in the dune test suite on macos+nix in promote-only-when-needed:

$ dune build @promote-only-when-needed
File "test/blackbox-tests/test-cases/promote/promote-only-when-needed.t", line 1, characters 0-0:
------ test/blackbox-tests/test-cases/promote/promote-only-when-needed.t
++++++ test/blackbox-tests/test-cases/promote/promote-only-when-needed.t.corrected
File "test/blackbox-tests/test-cases/promote/promote-only-when-needed.t", line 52, characters 0-1:
 |  > (executable
 |  >  (name hello)
 |  >  (libraries dune-build-info)
 |  >  (promote))
 |  > EOF
 |
 |  $ cat >hello.ml <<EOF
 |  > print_endline "Hello, World!";
 |  > (match Build_info.V1.version () with
 |  >   | Some _ -> print_endline "Has version info"
 |  >   | None -> print_endline "Has no version info")
 |  > EOF
 |
 |  $ dune build hello.exe --verbose 2>&1 | grep "Promoting"
 |  Promoting "_build/default/hello.exe" to "hello.exe"
 |  $ ./hello.exe
-|  Hello, World!
-|  Has version info
+|  ./hello.exe: No such file or directory
+|  [127]
 |
 |Bug: Dune currently re-promotes versioned executables on every restart.
 |
 |# CR-someday amokhov: Fix this.
 |
 |  $ dune build hello.exe --verbose 2>&1 | grep "Promoting"
 |  Promoting "_build/default/hello.exe" to "hello.exe"

The grep "Promoting" is hiding a codesign error that fails the build:

File "dune", line 2, characters 7-12:
2 |  (name hello)
           ^^^^^
.#hello.exe.dune-temp: is already signed

greedy added a commit to greedy/dune that referenced this issue Feb 24, 2023
When using dune in macOS + nix, the nix tooling adds a codesigning hook
so when dune goes to sign files after substitution there is already an
existing signature which fails the build as could be seen in the
failure of the promote-only-when-needed test.

The failure can be avoided by providing the -f option to codesign which
will replace any existing signatures on the file.

However, when -f is used and a signature is replaced, codesign prints a
message that it is replacing the existing signature. This additional
message pollutes dune's output and causes spurious failures of the cram
tests on macOS + nix.

This secondary negative effect is eliminated by running the codesign
tool with output swallowed as long as the tool runs successfully.

Fixes ocaml#6265

Signed-off-by: Geoff Reedy <geoff@programmer-monk.net>
greedy added a commit to greedy/dune that referenced this issue Feb 25, 2023
When using dune in macOS + nix, the nix tooling adds a codesigning hook
so when dune goes to sign files after substitution there is already an
existing signature which fails the build as could be seen in the
failure of the promote-only-when-needed test.

The failure can be avoided by providing the -f option to codesign which
will replace any existing signatures on the file.

However, when -f is used and a signature is replaced, codesign prints a
message that it is replacing the existing signature. This additional
message pollutes dune's output and causes spurious failures of the cram
tests on macOS + nix.

This secondary negative effect is eliminated by running the codesign
tool with output swallowed as long as the tool runs successfully.

Fixes ocaml#6265

Signed-off-by: Geoff Reedy <geoff@programmer-monk.net>
@Alizter Alizter added the macos label Mar 19, 2023
emillon pushed a commit to greedy/dune that referenced this issue Apr 20, 2023
When using dune in macOS + nix, the nix tooling adds a codesigning hook
so when dune goes to sign files after substitution there is already an
existing signature which fails the build as could be seen in the
failure of the promote-only-when-needed test.

The failure can be avoided by providing the -f option to codesign which
will replace any existing signatures on the file.

However, when -f is used and a signature is replaced, codesign prints a
message that it is replacing the existing signature. This additional
message pollutes dune's output and causes spurious failures of the cram
tests on macOS + nix.

This secondary negative effect is eliminated by running the codesign
tool with output swallowed as long as the tool runs successfully.

Fixes ocaml#6265

Signed-off-by: Geoff Reedy <geoff@programmer-monk.net>
emillon added a commit to emillon/opam-repository that referenced this issue May 3, 2023
CHANGES:

- Correctly set `MANPATH` in `dune exec`. Previously, we would use the `bin/`
  directory of the context. (ocaml/dune#7655, @rgrinberg)

- Allow overriding the `ocaml` binary with findlib configuration (ocaml/dune#7648,
  @rgrinberg)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will cause Dune to emit a warning which will become an error in 3.9. (ocaml/dune#7608,
  fixes ocaml/dune#7026, @Alizter)

- Dune can now detect Coq theories from outside the workspace. This allows for
  composition with installed theories (not necessarily installed with Dune).
  (ocaml/dune#7047, @Alizter, @ejgallego)

- Fix regression where Merlin was unable to handle filenames with uppercase
  letters under Windows. (ocaml/dune#7577, @nojb)

- On nix+macos, pass `-f` to the codesign hook to avoid errors when the binary
  is already signed (ocaml/dune#7183, fixes ocaml/dune#6265, @greedy)

- Fix bug where RPC clients built with dune-rpc-lwt would crash when closing
  their connection to the server (ocaml/dune#7581, @gridbugs)

- Introduce mdx stanza 0.4 requiring mdx >= 2.3.0 which updates the default
  list of files to include `*.mld` files (ocaml/dune#7582, @Leonidas-from-XIV)

- Fix RPC server on Windows (used for OCaml-LSP). (ocaml/dune#7666, @nojb)

- In `(executable)`, `(public_name -)` is now equivalent to no `(public_name)`.
  This is consistent with how `(executables)` handles this field.
  (ocaml/dune#7576 , fixes ocaml/dune#5852, @emillon)

- Change directory of odoc assets to `odoc.support` (was `_odoc_support`) so
  that it works with Github Pages out of the box. (ocaml/dune#7588, fixes ocaml/dune#7364,
  @emillon)
emillon added a commit to emillon/opam-repository that referenced this issue May 23, 2023
CHANGES:

- Fix string quoting in the json file written by `--trace-file` (ocaml/dune#7773,
  @rleshchinskiy)

- Read `pkg-config` arguments from the `PKG_CONFIG_ARGN` environment variable
  (ocaml/dune#1492, ocaml/dune#7734, @anmonteiro)

- Correctly set `MANPATH` in `dune exec`. Previously, we would use the `bin/`
  directory of the context. (ocaml/dune#7655, @rgrinberg)

- Allow overriding the `ocaml` binary with findlib configuration (ocaml/dune#7648,
  @rgrinberg)

- merlin: ignore instrumentation settings for preprocessing. (ocaml/dune#7606, fixes
  ocaml/dune#7465, @Alizter)

- When a rule's action is interrupted, delete any leftover directory targets.
  This is consistent with how we treat file targets. (ocaml/dune#7564, @rgrinberg)

- Fix plugin loading with findlib. The functionality was broken in 3.7.0.
  (ocaml/dune#7556, @anmonteiro)

- Introduce a `public_headers` field on libraries. This field is like
  `install_c_headers`, but it allows to choose the extension and choose the
  paths for the installed headers. (ocaml/dune#7512, @rgrinberg)

- Load the host context `findlib.conf` when cross-compiling (ocaml/dune#7428, fixes
  ocaml/dune#1701, @rgrinberg, @anmonteiro)

- Add a `coqdoc_flags` field to the `coq.theory` stanza allowing the user to
  pass extra arguments to `coqdoc`. (ocaml/dune#7676, fixes ocaml/dune#7954 @Alizter)

- Resolve `ppx_runtime_libraries` in the target context when cross compiling
  (ocaml/dune#7450, fixes ocaml/dune#2794, @anmonteiro)

- Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary  (ocaml/dune#7469, fixes
  ocaml/dune#2572, @anmonteiro)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will cause Dune to emit a warning which will become an error in 3.9. (ocaml/dune#7608,
  fixes ocaml/dune#7026, @Alizter)

- Preliminary support for Coq compiled intefaces (`.vos` files) enabled via
  `(mode vos)` in `coq.theory` stanzas. This can be used in combination with
  `dune coq top` to obtain fast re-building of dependencies (with no checking
  of proofs) prior to stepping into a file. (ocaml/dune#7406, @rlepigre)

- Fix dune crashing on MacOS in watch mode whenever `$PATH` contains `$PWD`
  (ocaml/dune#7441, fixes ocaml/dune#6907, @rgrinberg)

- Fix `dune install` when cross compiling (ocaml/dune#7410, fixes ocaml/dune#6191, @anmonteiro,
  @rizo)

- Find `pps` dependencies in the host context when cross-compiling,  (ocaml/dune#7410,
  fixes ocaml/dune#4156, @anmonteiro)

- Dune in watch mode no longer builds concurrent rules in serial (ocaml/dune#7395
  @rgrinberg, @jchavarri)

- Dune can now detect Coq theories from outside the workspace. This allows for
  composition with installed theories (not necessarily installed with Dune).
  (ocaml/dune#7047, @Alizter, @ejgallego)

- `dune coq top` now correctly respects the project root when called from a
  subdirectory. However, absolute filenames passed to `dune coq top` are no
  longer supported (due to being buggy) (ocaml/dune#7357, fixes ocaml/dune#7344, @rlepigre and
  @Alizter)

- Added a `--no-build` option to `dune coq top` for avoiding rebuilds (ocaml/dune#7380,
  fixes ocaml/dune#7355, @Alizter)

- RPC: Ignore SIGPIPE when clients suddenly disconnect (ocaml/dune#7299, ocaml/dune#7319, fixes
  ocaml/dune#6879, @rgrinberg)

- Always clean up the UI on exit. (ocaml/dune#7271, fixes ocaml/dune#7142 @rgrinberg)

- Bootstrap: remove reliance on shell. Previously, we'd use the shell to get
  the number of processors. (ocaml/dune#7274, @rgrinberg)

- Bootstrap: correctly detect the number of processors by allowing `nproc` to be
  looked up in `$PATH` (ocaml/dune#7272, @Alizter)

- Speed up file copying on macos by using `clonefile` when available
  (@rgrinberg, ocaml/dune#7210)

- Adds support for loading plugins in toplevels (ocaml/dune#6082, fixes ocaml/dune#6081,
  @ivg, @richardlford)

- Support commands that output 8-bit and 24-bit colors in the terminal (ocaml/dune#7188,
  @Alizter)

- Speed up rule generation for libraries and executables with many modules
  (ocaml/dune#7187, @jchavarri)

- Add `--watch-exclusions` to Dune build options (ocaml/dune#7216, @jonahbeckford)

- Do not re-render UI on every frame if the UI doesn't change (ocaml/dune#7186, fix
  ocaml/dune#7184, @rgrinberg)

- Make coq_db creation in scope lazy (@ejgallego, ocaml/dune#7133)

- Non-user proccesses such as version control or config checking are now run
  silently. (ocaml/dune#6994, fixes ocaml/dune#4066, @Alizter)

- Add the `--display-separate-messages` flag to separate the error messages
  produced by commands with a blank line. (ocaml/dune#6823, fixes ocaml/dune#6158, @esope)

- Accept the Ordered Set Language for the `modes` field in `library` stanzas
  (ocaml/dune#6611, @anmonteiro).

- dune install now respects --display quiet mode (ocaml/dune#7116, fixes ocaml/dune#4573, fixes
  ocaml/dune#7106, @Alizter)

- Stub shared libraries (dllXXX_stubs.so) in Dune-installed libraries could not
  be used as dependencies of libraries in the workspace (eg when compiling to
  bytecode and/or Javascript).  This is now fixed. (ocaml/dune#7151, @nojb)

- Allow the main module of a library with `(stdlib ...)` to depend on other
  libraries (ocaml/dune#7154, @anmonteiro).

- Bytecode executables built for JSOO are linked with `-noautolink` and no
  longer depend on the shared stubs of their dependent libraries (ocaml/dune#7156, @nojb)

- Added a new user action `(concurrent )` which is like `(progn )` but runs the
  actions concurrently. (ocaml/dune#6933, @Alizter)

- Allow `(stdlib ...)` to be used with `(wrapped false)` in library stanzas
  (ocaml/dune#7139, @anmonteiro).

- Allow parallel execution of inline tests partitions (ocaml/dune#7012, @hhugo)

- Support `(link_flags ...)` in `(cinaps ...)` stanza. (ocaml/dune#7423, fixes ocaml/dune#7416,
  @nojb)

- Allow `(package ...)` in any position within `(rule ...)` stanza (ocaml/dune#7445,
  @Leonidas-from-XIV)

- Always include `opam` files in the generated `.install` file. Previously, it
  would not be included whenever `(generate_opam_files true)` was set and the
  `.install` file wasn't yet generated. (ocaml/dune#7547, @rgrinberg)

- Fix regression where Merlin was unable to handle filenames with uppercase
  letters under Windows. (ocaml/dune#7577, @nojb)

- On nix+macos, pass `-f` to the codesign hook to avoid errors when the binary
  is already signed (ocaml/dune#7183, fixes ocaml/dune#6265, @greedy)

- Fix bug where RPC clients built with dune-rpc-lwt would crash when closing
  their connection to the server (ocaml/dune#7581, @gridbugs)

- Introduce mdx stanza 0.4 requiring mdx >= 2.3.0 which updates the default
  list of files to include `*.mld` files (ocaml/dune#7582, @Leonidas-from-XIV)

- Fix RPC server on Windows (used for OCaml-LSP). (ocaml/dune#7666, @nojb)

- Coq language versions less 0.8 are deprecated, and will be removed
  in an upcoming Dune version. All users are required to migrate to
  `(coq lang 0.8)` which provides the right semantics for theories
  that have been globally installed, such as those coming from opam
  (@ejgallego, @Alizter)

- Bump minimum version of the dune language for the melange syntax extension
  from 3.7 to 3.8 (ocaml/dune#7665, @jchavarri)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment