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(fsevents): correctly handle unknown events #6217

Merged
merged 2 commits into from
Oct 17, 2022
Merged

fix(fsevents): correctly handle unknown events #6217

merged 2 commits into from
Oct 17, 2022

Conversation

rgrinberg
Copy link
Member

We should apply our action mask before converting it into an OCaml
variant. Otherwise, we'll get events that we don't understand.

Signed-off-by: Rudi Grinberg [email protected]

ps-id: 204d7599-9ffb-4e95-8cff-871f39b4dc68

@rgrinberg rgrinberg requested a review from emillon October 11, 2022 17:09
@rgrinberg
Copy link
Member Author

@samoht could you give this PR a try?

@samoht
Copy link
Member

samoht commented Oct 11, 2022 •
edited
Loading

Should it be on top of the other one? (if so could you make a branch with all the patches - it's easier to pin/test for me)

@samoht
Copy link
Member

samoht commented Oct 11, 2022

I still have a Error: exception Failure("fsevents: unexpected event action") with just this PR.

@rgrinberg rgrinberg force-pushed the ps/rr/fix_fsevents___correctly_handle_unknown_events branch from 5430112 to 8ad9aca Compare October 11, 2022 18:21
@rgrinberg
Copy link
Member Author

Should it be on top of the other one? (if so could you make a branch with all the patches - it's easier to pin/test for me)

There's no need. That PR just improved the error message.

I still have a Error: exception Failure("fsevents: unexpected event action") with just this PR.

What about now?

@samoht
Copy link
Member

samoht commented Oct 12, 2022 •
edited
Loading

Same same:

Error: exception Failure("fsevents: unexpected event action")
Raised by primitive operation at Fsevents.Event.action in file
  "src/fsevents/fsevents.ml", line 204, characters 17-31
Called from Dune_file_watcher.fsevents_standard_event in file
  "src/dune_file_watcher/dune_file_watcher.ml", line 517, characters 15-42
Called from Stdune__list.filter_map.loop in file "otherlibs/stdune/list.ml",
  line 19, characters 12-15
Called from Dune_engine__scheduler.Run_once.iter in file
  "src/dune_engine/scheduler.ml", line 1053, characters 19-25
Called from Fiber.run.loop in file "src/fiber/fiber.ml", line 15, characters
  51-60
Called from Fiber.run.(fun) in file "src/fiber/fiber.ml" (inlined), line 17,
  characters 17-47
Called from Dune_engine__scheduler.Run_once.run in file
  "src/dune_engine/scheduler.ml", line 1131, characters 10-50
Re-raised at Stdune__exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml" (inlined), line 36, characters 27-56
Called from Dune_engine__scheduler.Run.go.(fun) in file
  "src/dune_engine/scheduler.ml", line 1367, characters 30-61
Called from Cmdliner_term.app.(fun) in file
  "vendor/cmdliner/src/cmdliner_term.ml", line 24, characters 19-24
Called from Cmdliner_eval.run_parser in file
  "vendor/cmdliner/src/cmdliner_eval.ml", line 34, characters 37-44
Called from Cmdliner_eval.eval_value in file
  "vendor/cmdliner/src/cmdliner_eval.ml", line 202, characters 14-39
Called from Main in file "bin/main.ml", line 90, characters 10-41

@rgrinberg
Copy link
Member Author

@emillon could you review the PR? This PR fixes one part of the bug, the other part will be fixed in a subsequent PR.

@emillon emillon added this to the 3.5.0 milestone Oct 13, 2022
@rgrinberg rgrinberg force-pushed the ps/rr/fix_fsevents___correctly_handle_unknown_events branch 4 times, most recently from 4a62006 to 0331c6b Compare October 14, 2022 00:54
@rgrinberg
Copy link
Member Author

@emillon one more round of review please.

@samoht could you test again?

emillon
emillon requested changes Oct 14, 2022
View reviewed changes
src/fsevents/fsevents_stubs.c Outdated Show resolved Hide resolved
src/fsevents/fsevents_stubs.c Outdated Show resolved Hide resolved
@samoht
Copy link
Member

samoht commented Oct 14, 2022 •
edited
Loading

No more crashes!

@rgrinberg rgrinberg force-pushed the ps/rr/fix_fsevents___correctly_handle_unknown_events branch 3 times, most recently from e11e256 to 17d917d Compare October 14, 2022 14:11
@rgrinberg
fix(fsevents): correctly handle unknown events
9f69d0d 
We should apply our action mask before converting it into an OCaml
variant. Otherwise, we'll get events that we don't understand.

Signed-off-by: Rudi Grinberg <[email protected]>

ps-id: 204d7599-9ffb-4e95-8cff-871f39b4dc68
@rgrinberg rgrinberg force-pushed the ps/rr/fix_fsevents___correctly_handle_unknown_events branch from 17d917d to 9f69d0d Compare October 15, 2022 19:59
@rgrinberg rgrinberg merged commit 401bb68 into main Oct 17, 2022
@rgrinberg rgrinberg deleted the ps/rr/fix_fsevents___correctly_handle_unknown_events branch October 17, 2022 23:37
@emillon emillon mentioned this pull request Oct 19, 2022
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

@emillon emillon emillon approved these changes

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 participants
@rgrinberg @samoht @emillon