Repository navigation
Conversation
010c1ae to
f456adc
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8839 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f456adc3e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94a90ef802
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1c8248246
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd0c132ece
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccacd4b908
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cknitt
changed the title
765184e to
a5af682
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5af682777
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a5af682 to
e3ada4d
Compare
cknitt
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3ada4d2cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Rust rewatch (`rescript-rust`) watches each source folder separately, and
source folders may overlap. On a full rebuild it unwatched the paths that
went away and registered only the new ones. With inotify on Linux, notify
keeps one watch per directory, shared by every registered path that reaches
it, so unwatching a removed recursive folder also dropped a folder that is
still configured, and edits there were no longer compiled. Outside macOS,
every configured path is now registered again after the stale ones are
unwatched; registering an already watched directory only updates its watch,
so unchanged watches have no gap. macOS keeps registering only the new
paths, because FSEvents restarts its event stream on every watch change and
its watches don't depend on each other.
Example (text):
rescript.json of a package, while `rescript-rust watch` runs:
"sources": [{"dir": "src", "subdirs": true}, "src/retained"]
changed to
"sources": ["src", "src/retained"]
then src/retained/Retained.res is edited.
Before (text):
The edit isn't compiled (Linux).
After (text):
The edit is compiled.
Source: rewatch/tests/watch/08-watch-overlapping-source-removed.sh, run
against master's rescript-rust (fails 2 of 2) and this branch's (passes).
Changelog entry: - Rewatch: keep watching a source folder on Linux when an overlapping recursive source folder is removed from `rescript.json`. #8839
Review: Is OCaml rewatch (`rescript`) affected? No. It resolves every
watched folder to its canonical directory, keeps one libuv handle per
directory, and on a refresh closes only the directories no longer wanted.
The new test runs against both executables in the shared suite and passes
for `rescript` on master already.
Review: Why not keep the incremental update everywhere? Watches can share
inotify state through overlapping folders, `..` and symlinks, including
directory symlinks inside a recursive folder; working out every way an
unwatch can drop another watch took several review rounds. Registering every
path again needs no such reasoning and costs no gap on inotify.
Review: Why does macOS keep the incremental update? #8730 introduced it
because FSEvents restarts its event stream on every watch or unwatch call
and loses the events in between. FSEvents watches are independent paths, so
an unwatch never drops another watch there.
Area: rewatch tests
Changelog: fix
Output: unchanged
Tests: +rewatch/tests/watch/08-watch-overlapping-source-removed.sh
~rewatch/tests/suite.sh
PR: #8839
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BC5F5QPeQh5cHBjGBnvJ2T
Signed-off-by: Christoph Knittel <ck@cca.io>
e3ada4d to
e24b200
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Rust rewatch (`rescript-rust`) watches each source folder separately, and
source folders may overlap. On a full rebuild it unwatched the paths that
went away and registered only the new ones. With inotify on Linux, notify
keeps one watch per directory, shared by every registered path that reaches
it, so unwatching a removed recursive folder also dropped a folder that is
still configured, and edits there were no longer compiled. Outside macOS,
every configured path is now registered again after the stale ones are
unwatched; registering an already watched directory only updates its watch,
so unchanged watches have no gap. macOS keeps registering only the new
paths, because FSEvents restarts its event stream on every watch change and
its watches don't depend on each other.
Watches are also registered non-recursive first. notify keeps one recursive
flag per directory, set by the last registration that reaches it, so a
non-recursive source folder inside a recursive one could clear the flag, and
folders created there later weren't watched.
Example (text):
rescript.json of a package, while `rescript-rust watch` runs:
"sources": [{"dir": "src", "subdirs": true}, "src/retained"]
changed to
"sources": ["src", "src/retained"]
then src/retained/Retained.res is edited. Before the change, a file is
also created in a new folder src/retained/nested.
Before (text):
The edit isn't compiled (Linux). The new folder's file is missed in most
runs.
After (text):
Both are compiled.
Source: rewatch/tests/watch/08-watch-overlapping-source-removed.sh, run
against master's rescript-rust (the edit fails 2 of 2), this branch without
the registration order (the new folder fails 7 of 8) and this branch (passes
8 of 8).
Changelog entry: - Rewatch: keep watching a source folder on Linux when an overlapping recursive source folder is removed from `rescript.json`. #8839
Review: Is OCaml rewatch (`rescript`) affected? No. It resolves every
watched folder to its canonical directory, keeps one libuv handle per
directory, and on a refresh closes only the directories no longer wanted.
The new test runs against both executables in the shared suite and passes
for `rescript` on master already.
Review: Why not keep the incremental update everywhere? Watches can share
inotify state through overlapping folders, `..` and symlinks, including
directory symlinks inside a recursive folder; working out every way an
unwatch can drop another watch took several review rounds. Registering every
path again needs no such reasoning and costs no gap on inotify.
Review: Can an edit be missed while the watches are updated? Only after a
config change that removes a recursive folder overlapping a kept one: the
kept folder's watch is dropped by the unwatch and restored when every path
is registered again, so an edit in between is picked up only with the next
change. Ordinary full rebuilds unwatch nothing. Closing that window would
need a rescan after registration, which isn't worth it for the Rust
fallback.
Review: Why does macOS keep the incremental update? #8730 introduced it
because FSEvents restarts its event stream on every watch or unwatch call
and loses the events in between. FSEvents watches are independent paths, so
an unwatch never drops another watch there.
Area: rewatch tests
Changelog: fix
Output: unchanged
Tests: +rewatch/tests/watch/08-watch-overlapping-source-removed.sh
~rewatch/tests/suite.sh
PR: #8839
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BC5F5QPeQh5cHBjGBnvJ2T
Signed-off-by: Christoph Knittel <ck@cca.io>
e24b200 to
d3591fd
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This branch has not been deployed
1 participant
rescript watchregisters each source folder as its own watch, and source folders may overlap. In Rust rewatch (rescript-rust), a full rebuild (after arescript.jsonchange, but also when a source file is added, removed or renamed) unwatches the paths that went away and, since #8730, registers only the new ones. With inotify on Linux that loses watches: notify keeps one watch per directory, shared by every registered path that reaches it, so unwatching one path can drop another that is still configured.For example, with a package's sources changed from
[{"dir": "src", "subdirs": true}, "src/retained"]to["src", "src/retained"]while the watcher runs, unwatching the recursivesrcalso droppedsrc/retained, and edits there were no longer compiled (#8825). The same can happen through other spellings of a folder (.., symlinks) and through directory symlinks inside a recursive folder.update_watchesstill unwatches only the stale watches first (paths no longer configured, or whose directory was replaced). After that it depends on the platform:watch/unwatchcall and loses the events in between, which is why Fix watcher missing watch.lock removal during full rebuilds #8730 made updates incremental. Its watches are independent paths, so an unwatch never drops another one.The doc comment of
update_watchesexplains these differences.Watches are also registered non-recursive first, on every platform. notify keeps one recursive flag per directory, set by the last registration that reaches it, and only watches a folder created later if its parent's flag is recursive. Registered in
HashMaporder, a non-recursivesrc/retainedcould clear the flag that the recursivesrcset, so files in folders created insidesrc/retainedwere missed. That already happened on master's first registration; with every path registered again, it could also happen on each full rebuild.One window remains: after a config change that removes a recursive folder overlapping a kept one, the kept folder's watch is dropped by the unwatch and restored when every path is registered again, so an edit in between is picked up only with the next change. Ordinary full rebuilds unwatch nothing. Closing it would need a rescan after registration, which isn't worth it for the Rust fallback.
OCaml rewatch (
rescript), which ships as the default since #8780, isn't affected: it resolves every watched folder to its canonical directory, keeps one libuv handle per directory, and on a refresh closes only the directories that are no longer wanted.New test:
rewatch/tests/watch/08-watch-overlapping-source-removed.shin the shared suite runs against both executables. With the overlapping configuration, it checks that a file in a folder created insidesrc/retainedis compiled. It then reproduces the example above through the CLI and checks that an edit insrc/retainedis compiled after the change. With master,rescript-rustfails the edit check (2 of 2 runs); without the registration order, the new-folder check failed 7 of 8 runs.rescriptpasses both.Fixes #8825
🤖 Generated with Claude Code
https://claude.ai/code/session_01BC5F5QPeQh5cHBjGBnvJ2T