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.

Sign up for GitHub

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

Jump to bottom

Only run codesign if there have been substitutions #6231

Merged
merged 5 commits into from
Oct 18, 2022
Merged

Only run codesign if there have been substitutions #6231

merged 5 commits into from
Oct 18, 2022

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Oct 14, 2022

This ensures that we're not running codesign in cases we don't strictly need it. This in turn prevents a regression in macos+nix, where the codesign binary is not in PATH.

Closes #6226

@emillon
Copy link
Collaborator Author

emillon commented Oct 14, 2022

@anmonteiro can you try this patch to see if this works on mac+nix? 🙏

@emillon emillon force-pushed the test-artifact-substitution branch from 77b3645 to c2230a8 Compare October 14, 2022 14:46
@emillon emillon added this to the 3.5.0 milestone Oct 14, 2022
@rgrinberg
Copy link
Member

Do nix builds skip the substitution step or something? Otherwise, I don't see how it fixes the original issue.

@emillon
Copy link
Collaborator Author

emillon commented Oct 14, 2022

The reasoning is the following: the sign hook runs in all cases where copy is called. This includes cases where the binary does not contain anyplaceholders. Because of this, we're calling codesign in cases that are not necessary (the signature would still be valid).
This is the regression found in #6226: users with a M1 mac, even with nix, should have something that keeps working when they don't use substitution (this was working pre-3.5.0).
For M1 users that want to use substitution, there used to be no support pre 3.5; 3.5 brings support to them (except for nix users), and we'll have to find a way to make this work for nix users post 3.5 (maybe by tweaking nix?).

@anmonteiro
Copy link
Collaborator

I'm still seeing the same error with this patch:

dune> File "_unknown_", line 1, characters 0-0:
dune> Error: Program codesign not found in the tree or in PATH
dune> Hint: codesign should be part of the macOS installation

@anmonteiro
Copy link
Collaborator

If my issue is delaying a Dune 3.5 release, it should be OK to go ahead the current solution, if no one else is experiencing these issues. I can work around it on my end for the time being.

@rgrinberg
Copy link
Member

If my issue is delaying a Dune 3.5 release, it should be OK to go ahead the current solution, if no one else is experiencing these issues. I can work around it on my end for the time being.

Thanks for the offer, but we'd rather not release with known regressions.

@emillon is this just an issue of looking up codesign too eagerly?

@emillon
Copy link
Collaborator Author

emillon commented Oct 17, 2022

Yes, we're looking for codesign a bit too early. I'll amend the PR.

@emillon
Artifact_substitution.copy: return a boolean
65594df 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Only run codesign if there have been substitutions
d319f05 
This ensures that we're not running `codesign` in cases we don't
strictly need it. This in turn prevents a regression in macos+nix, where
the codesign binary is not in PATH.

Closes ocaml#6226

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon force-pushed the test-artifact-substitution branch from c2230a8 to e3bb0d1 Compare October 17, 2022 08:16
@emillon
Compute sign hook lazily
df11db1 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Return a status type instead of a plain bool
4838ed0 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon force-pushed the test-artifact-substitution branch from e3bb0d1 to 4838ed0 Compare October 17, 2022 08:51
@emillon
Copy link
Collaborator Author

emillon commented Oct 17, 2022

@anmonteiro can you try this one? we're now only looking up codesign in PATH as we're substituting (if needed).

@anmonteiro
Copy link
Collaborator

anmonteiro commented Oct 18, 2022 •
edited
Loading

The latest rev seems to work (correctly) like before. Thank you!

@emillon
Copy link
Collaborator Author

emillon commented Oct 18, 2022

Perfect! Thanks a lot for testing.

@emillon emillon merged commit 74304d0 into ocaml:main Oct 18, 2022
@emillon emillon deleted the test-artifact-substitution branch October 18, 2022 08:23
This was referenced Oct 19, 2022
Merged
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Oct 19, 2022
@emillon
[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, du…
2996546 
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0)

CHANGES:

- macOS: Handle unknown fsevents without crashing (ocaml/dune#6217, @rgrinberg)

- Enable file watching on MacOS SDK < 10.13. (ocaml/dune#6218, @rgrinberg)

- Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg)

- Add a `runtime_deps` field in the `cinaps` stanza to specify runtime
  dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg)

- Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg)

- Extend dune describe to include the root path of the workspace and the
  relative path to the build directory. (ocaml/dune#6136, @reubenrowe)

- Allow dune describe workspace to accept directories as arguments.
  The provided directories restrict the worskpace description to those
  directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope)

- Add a terminal persistence mode that attempts to clear the terminal history.
  It is enabled by setting terminal persistence to
  `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg)

- Disallow generating targets in sub direcories in inferred rules. The check to
  forbid this was accidentally done only for manually specified targets (ocaml/dune#6031,
  @rgrinberg)

- Do not ignore rules marked `(promote (until-clean))` when
  `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon)

- Dune no longer considers .aux files as targets during Coq compilation. This
  means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter)

- Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg)

- Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056,
  @rgrinberg)

- Introduce a `dirs` field in the `install` stanza to install entire
  directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg)

- Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg)

- Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes
  ocaml/dune#5945, @rgrinberg)

- Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg)

- Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine)

- Add an `(include <file>)` term to the `include_dirs` field for adding
  directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993,
  @gridbugs)

- Support `(extra_objects ...)` field in `(executable ...)` and `(library
  ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs)

- Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb)

- Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114,
  fixes ocaml/dune#6103, @emillon)

- odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes
  ocaml/dune#1117, @emillon)

- dune install: copy files in an atomic way (ocaml/dune#6150, @emillon)

- Add `%{coq:...}` macro for accessing data about the configuration about Coq.
  For instance `%{coq:version}` (ocaml/dune#6049, @Alizter)

- update vendored copy of cmdliner to 1.1.1. This improves the built-in
  documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon,
  ocaml/dune#6169, @shonfeder)

- The test suite for Coq now requires Coq >= 8.16 due to changes in the
  plugin loading mechanism upstream (which now uses `Findlib`).

- Starting with Coq build language 0.6, theories can be built without importing
  Coq's standard library by including `(stdlib no)`.
  (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek)

- on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, ocaml/dune#6231,
  fixes ocaml/dune#5650, fixes ocaml/dune#6226, @emillon)

- Added an (aliases ...) field to the (rules ...) stanza which allows the
  specification of multiple aliases per rule (ocaml/dune#6194, @Alizter)

- The `(coq.theory ...)` stanza will now ensure that for each declared `(plugin
 ...)`, the `META` file for it is built before calling `coqdep`. This enables
 the use of the new `Findlib`-based loading method in Coq 8.16; however as of
 Coq 8.16.0, Coq itself has some bugs preventing this to work yet. (ocaml/dune#6167 ,
 workarounds ocaml/dune#5767, @ejgallego)

- Allow include statement in install stanza (ocaml/dune#6139, fixes ocaml/dune#256, @gridbugs)

- Handle CSI n K code in ANSI escape codes from commands. (ocaml/dune#6214, fixes ocaml/dune#5528,
  @emillon)

- Add a new experimental feature `mode_specific_stubs` that allows the
  specification of different flags and sources for foreign stubs depending on
  the build mode (ocaml/dune#5649, @voodoos)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.

3.5.0-alpha1: Can't build without codesign on macOS
3 participants
@emillon @rgrinberg @anmonteiro