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

fix(codesign): run hook for promoted executables #10263

Merged
merged 4 commits into from
Mar 14, 2024
Merged

fix(codesign): run hook for promoted executables #10263

merged 4 commits into from
Mar 14, 2024

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Mar 13, 2024

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.

emillon added 3 commits March 13, 2024 15:53
@emillon
fix(codesign): run hook for promoted executables
d82161c 
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
simplify test
c62b95a 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
changelog
1761e95 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon marked this pull request as ready for review March 13, 2024 15:00
@emillon
Copy link
Collaborator Author

emillon commented Mar 13, 2024

In the future I'd like to turn this thing around and selectively call Artifact_substitution.copy_file only for the files that require it, but in the meantime this is a fairly conservative change that fixes the issue. It adds a stat call but on a path that calls an external process so no performance impact.

@emillon emillon requested a review from rgrinberg March 13, 2024 15:35
@rgrinberg rgrinberg requested a review from anmonteiro March 13, 2024 16:18
anmonteiro
anmonteiro approved these changes Mar 13, 2024
View reviewed changes
Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in hindsight maybe the ?executable argument shouldn't have been optional, and it would have given us all the locations that we needed to audit.

The only question I have is whether the stating should be done a background thread?

@rgrinberg
Copy link
Member

in hindsight maybe the ?executable argument shouldn't have been optional, and it would have given us all the locations that we needed to audit.

Indeed. We need to get rid of all of these pointless optional variables.

@anmonteiro
Copy link
Collaborator

I also kicked off a nix-overlays CI run with this patch here: nix-ocaml/nix-overlays#1320

@emillon
Copy link
Collaborator Author

emillon commented Mar 14, 2024

The only question I have is whether the stating should be done a background thread?

There's probably a way to avoid it. The setup_copy call knows the mode because that's how it creates the file. I suppose we could return it as well but that seems a bit awkward as an API. I don't think it matters for performance.

@emillon emillon merged commit 40ea41f into ocaml:main Mar 14, 2024
26 of 27 checks passed
@emillon emillon deleted the fix-9272 branch March 14, 2024 13:10
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Mar 26, 2024
Closed
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 26, 2024
@Leonidas-from-XIV
[new release] dune (15 packages) (3.15.0~alpha1)
68b4763 
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 pull request Apr 3, 2024
@Leonidas-from-XIV
[new release] dune (15 packages) (3.15.0)
950c529 
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 Leonidas-from-XIV mentioned this pull request Apr 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmonteiro anmonteiro anmonteiro approved these changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

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

Successfully merging this pull request may close these issues.

Dune 3.11 introduces regression on macOS with regarding to executable promotion
3 participants
@emillon @rgrinberg @anmonteiro