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

Ignore insignificant file system changes by default #5953

Merged
merged 5 commits into from
Jul 14, 2022
Merged

Ignore insignificant file system changes by default #5953

merged 5 commits into from
Jul 14, 2022

Conversation

rgrinberg
Copy link
Member

But leave an option to restore the old behavior for benchmarking.

@rgrinberg rgrinberg added this to the 3.4.0 milestone Jul 11, 2022
@rgrinberg rgrinberg requested a review from snowleopard July 11, 2022 19:46
@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch from 2691e9f to 0f86651 Compare July 11, 2022 19:54
snowleopard
snowleopard reviewed Jul 11, 2022
View reviewed changes
bin/common.ml Outdated Show resolved Hide resolved
snowleopard
snowleopard reviewed Jul 11, 2022
View reviewed changes
bin/common.ml Outdated Show resolved Hide resolved
@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch from 0f86651 to 684d210 Compare July 11, 2022 20:54
bobot
bobot reviewed Jul 12, 2022
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch 2 times, most recently from aceb742 to 11016d7 Compare July 12, 2022 16:14
snowleopard
snowleopard reviewed Jul 12, 2022
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
@snowleopard
Copy link
Collaborator

snowleopard commented Jul 12, 2022 •
edited
Loading

@rgrinberg Have you tested that this actually works? I think I tried a similar change but it didn't work for me. It would also be nice to have an automated test for this (say, touching a source file while dune -w is running), so we don't regress.

@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch 2 times, most recently from 76131b6 to 21916e4 Compare July 13, 2022 16:40
@rgrinberg
Copy link
Member Author

@snowleopard i've fixed the issues and verified that everything works.

I was trying to write an automated test for this, but I'm having a hard time with writing something portable.

snowleopard
snowleopard approved these changes Jul 13, 2022
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks! I could barely follow the new logic though -- it's quite complicated.

Could you add at least a Linux-only test if making a portable one is tricky? I'd love to do some follow-up refactoring to simplify things but without a test, it's pretty scary to touch this code. Also, we might accidentally introduce a regression in future.

@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch 2 times, most recently from 531271b to 1f8798c Compare July 13, 2022 21:28
@rgrinberg
Copy link
Member Author

@snowleopard I pushed a couple of refactoring commits that should simplify things.

Let me try adding a more direct unit test. Even if I stick to inotify, it's hard to write something that isn't prone to races. To test this thing is working, I need to verify that a new built isn't being triggered. I'll see if I can write a unit test with the help of Scheduler.inject_memo_invalidation

@snowleopard
Copy link
Collaborator

@snowleopard I pushed a couple of refactoring commits that should simplify things.

Cool, thanks.

To test this thing is working, I need to verify that a new built isn't being triggered.

Indeed. That's the exact reason we sometimes want to react to insignificant changes -- that makes it easier to write deterministic tests/benchmarks.

@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch 2 times, most recently from 7a4bea1 to 26b27c7 Compare July 13, 2022 23:06
@rgrinberg
Copy link
Member Author

Alrighty, we have some unit tests going.

@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch from 26b27c7 to 5a47dd8 Compare July 13, 2022 23:26
snowleopard
snowleopard approved these changes Jul 14, 2022
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test! It looks good to me. Alas, there are some CI failures...

@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch from 5a47dd8 to 00695ef Compare July 14, 2022 15:08
rgrinberg added 3 commits July 14, 2022 11:25
@rgrinberg
test(scheduler): empty invalidation
fe6d5b2 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
refactor(scheduler): significant_changes flag
bf9e08c 
Introduce a flag in the scheduler config to control how we handle
insignifacnt changes

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

ps-id: 6AD7A5D9-C093-4442-935D-9377E8D8E146
@rgrinberg
fix: ignore insignificant_changes by default
efbc6f0 
but introduce a --react-insignificant-changes to restore the old
behavior for anyone that needs it (to benchmark)

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg added 2 commits July 14, 2022 11:25
@rgrinberg
refactor(scheduler): input handling in own function
f59c843 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
refactor(scheduler): simplify fs input management
89a8913 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg force-pushed the ps/rr/refactor_scheduler___significant_changes_flag branch from 00695ef to 89a8913 Compare July 14, 2022 16:25
@rgrinberg
Copy link
Member Author

CI is happy!

@rgrinberg rgrinberg merged commit 3031ac7 into main Jul 14, 2022
@rgrinberg rgrinberg deleted the ps/rr/refactor_scheduler___significant_changes_flag branch July 14, 2022 19:17
@emillon emillon mentioned this pull request Jul 20, 2022
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 20, 2022
@emillon
[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, du…
95b4730 
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.4.0)

CHANGES:

- Make `dune describe` correctly handle overlapping implementations
  for virtual libraries (ocaml/dune#5971, fixes ocaml/dune#5747, @esope)

- Building the `@check` alias should make sure the libraries and executables
  don't have dependency cycles (ocaml/dune#5892, @rgrinberg)

- [ctypes] Add support for the `errno` parameter using the `errno_policy` field
  in the ctypes settings. (ocaml/dune#5827, @droyo)

- Fix `dune coq top` when it is invoked on files from a subdirectory of the
  directory containing the associated stanza (ocaml/dune#5784, fixes ocaml/dune#5552, @ejgallego,
  @rlepigre, @Alizter)

- Fix hint when an invalid module name is found. (ocaml/dune#5922, fixes ocaml/dune#5273, @emillon)

- The `(cat)` action now supports several files. (ocaml/dune#5928, fixes ocaml/dune#5795, @emillon)

- Dune no longer uses shimmed `META` files for OCaml 5.x, solely using the ones
  installed by the compiler. (ocaml/dune#5916, @dra27)

- Fix handling of the `(deps)` field in `(test)` stanzas when there is an
  `.expected` file. (ocaml/dune#5952, ocaml/dune#5951, fixes ocaml/dune#5950, @emillon)

- Ignore insignificant filesystem events. This stops RPC in watch mode from
  flashing errors on insignificant file system events such as changes in the
  `.git/` directory. (ocaml/dune#5953, @rgrinberg)

- Fix parsing more error messages emitted by the OCaml compiler. In
  particular, messages where the excerpt line number started with a blank
  character were skipped. (ocaml/dune#5981, @rgrinberg)

- env stanza: warn if some rules are ignored because they appear after a
  wildcard rule. (ocaml/dune#5898, fixes ocaml/dune#5886, @emillon)

- On Windows, XDG_CACHE_HOME is taken to be the `FOLDERID_InternetCache` if
  unset, and XDG_CONFIG_HOME and XDG_DATA_HOME are both taken to be
  `FOLDERID_LocalAppData` if unset.  (ocaml/dune#5943, fixes ocaml/dune#5808, @nojb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobot bobot bobot left review comments

@snowleopard snowleopard snowleopard approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@rgrinberg @snowleopard @bobot