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.

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

Use latest watchman #21546

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Use latest watchman #21546

wants to merge 2 commits into from

Conversation

clauxx
Copy link
Member

@clauxx clauxx commented Oct 31, 2024

Summary

fixes #21545

Watchman was locked at an older version due to #16341. After the latest upgrade though, metro requires the latest watchman version and falls back to the "node watcher" (slower and buggier). This ends up (for me) with random timeouts when starting metro, since it takes the node watcher quite a bit of time to go through all the files, and we have a lot of them.

Need to make sure the issue from the latest Watchman, which was mentioned above, doesn't appear for anyone.

Also, I started getting a weird issue with metro's node process not dying after getting closed/killed. Sometimes it lost connection to the device and neither CTRL-C nor SIGKILL could get rid of the rogue node process. Inspecting the process' strace, I noticed that it tried to access non-existent files/directories inside node-modules, right at the moment node became non-responsive. Not sure how that's related to the unkillable process, but re-creating the yarn.lock seems to have fixed it (for now).

The strace:

statx(AT_FDCWD, "/home/cl/dev/work/status-mobile/node_modules", AT_STATX_SYNC_AS_STAT, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0700, stx_size=32768, ...}) = 0
statx(AT_FDCWD, "/home/cl/dev/work/status-mobile/node_modules/@react-native-community/cli-tools", AT_STATX_SYNC_AS_STAT, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0755, stx_size=4096, ...}) = 0
statx(AT_FDCWD, "/home/cl/dev/work/status-mobile/node_modules/@react-native-community/cli-tools.js", AT_STATX_SYNC_AS_STAT, STATX_ALL, 0x7fffa7794d20) = -1 ENOENT (No such file or directory)
statx(AT_FDCWD, "/home/cl/dev/work/status-mobile/node_modules/@react-native-community/cli-tools.json", AT_STATX_SYNC_AS_STAT, STATX_ALL, 0x7fffa7794d20) = -1 ENOENT (No such file or directory)
statx(AT_FDCWD, "/home/cl/dev/work/status-mobile/node_modules/@react-native-community/cli-tools.node", AT_STATX_SYNC_AS_STAT, STATX_ALL, 0x7fffa7794d20) = -1 ENOENT (No such file or directory)
statx(AT_FDCWD, "/home/cl/dev/work/status-mobile/node_modules/@react-native-community/cli-tools/build/index.js", AT_STATX_SYNC_AS_STAT, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0644, stx_size=6722, ...}) = 0
write(26, "\33[33m\33[1mwarn\33[22m\33[39m \33[0mNo a"..., 181) = 181
write(23, "\33[36m\33[1minfo\33[22m\33[39m \33[0mRelo"..., 62) = 62

status: ready

@clauxx
Copy link
Member Author

clauxx commented Oct 31, 2024

@vkjr @smohamedjavid can you test if you still get the watchman bug from #16341 on this branch?

@status-im-auto
Copy link
Member

status-im-auto commented Oct 31, 2024

Jenkins Builds

Click to see older builds (18)
Commit #️⃣ Finished (UTC) Duration Platform Result
c702280 #1 2024-10-31 21:30:40 ~10 min tests 📄log
c702280 #1 2024-10-31 21:30:43 ~10 min android 📄log
c702280 #1 2024-10-31 21:30:44 ~10 min android-e2e 📄log
✔️ c702280 #1 2024-10-31 21:33:05 ~13 min ios 📱ipa 📲
c702280 #2 2024-10-31 22:28:21 ~24 sec android 📄log
c702280 #2 2024-10-31 22:28:52 ~22 sec android-e2e 📄log
c702280 #2 2024-10-31 22:37:43 ~10 min tests 📄log
c702280 #3 2024-11-01 03:32:24 ~4 min tests 📄log
✔️ c702280 #3 2024-11-01 03:42:08 ~14 min android-e2e 🤖apk 📲
✔️ c702280 #3 2024-11-01 03:42:39 ~15 min android 🤖apk 📲
c702280 #4 2024-11-01 05:10:26 ~3 min tests 📄log
aa99828 #5 2024-11-01 08:48:47 ~10 min tests 📄log
✔️ aa99828 #2 2024-11-01 08:49:52 ~11 min ios 📱ipa 📲
✔️ c64f2c6 #3 2024-11-01 09:00:50 ~10 min ios 📱ipa 📲
c64f2c6 #6 2024-11-01 09:01:02 ~10 min tests 📄log
✔️ c64f2c6 #5 2024-11-01 09:01:36 ~11 min android-e2e 🤖apk 📲
✔️ c64f2c6 #5 2024-11-01 09:02:19 ~12 min android 🤖apk 📲
✔️ c64f2c6 #7 2024-11-01 09:06:26 ~4 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 9b4fe71 #8 2024-11-01 21:50:12 ~4 min tests 📄log
✔️ 9b4fe71 #4 2024-11-01 21:57:01 ~11 min ios 📱ipa 📲
✔️ 9b4fe71 #6 2024-11-01 21:57:25 ~11 min android-e2e 🤖apk 📲
✔️ 9b4fe71 #6 2024-11-01 21:57:52 ~12 min android 🤖apk 📲
✔️ f380670 #9 2024-11-05 15:00:43 ~4 min tests 📄log
✔️ f380670 #7 2024-11-05 15:03:11 ~7 min android 🤖apk 📲
✔️ f380670 #7 2024-11-05 15:03:33 ~7 min android-e2e 🤖apk 📲
✔️ f380670 #5 2024-11-05 15:04:52 ~8 min ios 📱ipa 📲

@siddarthkay
Copy link
Contributor

siddarthkay commented Nov 1, 2024

Looks good to me, metro didn't crash for me locally.
Podfile changes are missing though, I'll push.
CI failures aren't related to your code. I'll re-run the pipelines.

@siddarthkay
Copy link
Contributor

component tests do fail though : Test Suites: 18 failed, 106 passed, 124 total
https://ci.status.im/blue/organizations/jenkins/status-mobile%2Fprs%2Ftests/detail/PR-21546/3/pipeline/29

@siddarthkay
Copy link
Contributor

@clauxx : component tests pass for me locally after I revert your changes to yarn.lock file

@siddarthkay
Copy link
Contributor

I tested this change on Android and iOS and it works well locally.

Screenshot 2024-11-01 at 3 06 06 PM

@smohamedjavid
Copy link
Member

@vkjr @smohamedjavid can you test if you still get the watchman bug from #16341 on this branch?

Can confirm that the bug is NOT present anymore, and file changes (hot reload) work well as expected. Thanks 🙏

Comment on lines 2 to 7
"ignore_dirs": [
"android/build",
"target"
"target",
".clj-kondo",
".shadow-cljs"
]
Copy link
Member Author

Choose a reason for hiding this comment

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

while debugging metro, found that watchman is keeping track of these two directories (e.g. .clj-kondo has ~12000 files usually and .shadow-cljs ~3000). On top of that, working in the editor/IDE causes constant changes to .clj-kondo's cache, which re-trigger watchman making it do extra work for no reason (and also hogging up my CPU at times).

Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

Why not just upgrade the on in nix/pkgs?

The latest metro version requires watchman capabilities that are not
available in the version we locked, so it was not used. Instead we
used the "node watcher", which is slower and buggier (at least for me
metro was timing out quite often)
@clauxx
Copy link
Member Author

clauxx commented Nov 5, 2024

Why not just upgrade the on in nix/pkgs?

@jakubgs do you mean upgrade watchman from the nix/pkgs file? Could do, but I'm not as versed in nix, so it seemed easier & less time consuming to just use the latest version by removing it tbh.

On a sidenote, I removed the yarn.lock/podfile changes as it seems like it wasn't necessary for my issue. It seems like the problem with the un-killable node process was the linux kernel, where the node process was stuck in the "uninterruptible sleep" state, so that should explain why SIGKILL didn't do much. Updating the kernel version fixed the issue so far.

cc: @siddarthkay

Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

I think we used Nix for the package because - as the README states - we simply wanted to downgrade.

What I don't get is why is the fb-watchman in yarn.lock even older than the 4.9.0 that we provide via Nix right now?

status-mobile/yarn.lock

Lines 6053 to 6058 in ca22b67

fb-watchman@^2.0.0:
version "2.0.1"
resolved "https://registry.yarnpkg.com/fb-watchman/-/fb-watchman-2.0.1.tgz#fc84fb39d2709cf3ff6d743706157bb5708a8a85"
integrity sha512-DkPJKQeY6kKwmuMretBhr7G6Vodr7bFwDYTXIkfG1gjvNpaxBTQV3PbXg6bR1c1UP4jPOX0jHUbbHANL9vRjVg==
dependencies:
bser "2.1.1"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: REVIEW
Development

Successfully merging this pull request may close these issues.

Watchman isn't used after latest metro upgrade
6 participants