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

nix: downgrade watchman to 4.9.0 #16406

Merged
merged 1 commit into from
Jul 2, 2023
Merged

nix: downgrade watchman to 4.9.0 #16406

merged 1 commit into from
Jul 2, 2023

Conversation

yakimant
Copy link
Member

@yakimant yakimant commented Jun 27, 2023

fixes #16341

Summary

watchman was upgraded significantly during the last #14944 (4.9.0 (Aug 16, 2017) to 2023.01.30.00 - 6 years between):
status-im/nixpkgs@4e9c02b

Probably causing developers to have "too many files open" issue #16341

This PR is an attempt to fix the issue by downgrading the watchman

Testing notes

"too many files open" issue of watchman might be difficult to catch.
So let's test, that the downgrade doesn't break the flow rather.

Steps to test

  • run-clojure/run-metro/run-... flow shouls continue to work and update the app as before

status: wip

@yakimant yakimant requested a review from jakubgs as a code owner June 27, 2023 09:13
@yakimant
Copy link
Member Author

@jakubgs, please have a look

@yakimant yakimant mentioned this pull request Jun 27, 2023
@status-im-auto
Copy link
Member

status-im-auto commented Jun 27, 2023

Jenkins Builds

Click to see older builds (18)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 9e266c3 #1 2023-06-27 09:24:11 ~10 min android 🤖apk 📲
✔️ 9e266c3 #1 2023-06-27 09:24:21 ~10 min android-e2e 🤖apk 📲
✔️ 9e266c3 #1 2023-06-27 09:25:44 ~11 min tests 📄log
✔️ 9e266c3 #1 2023-06-27 09:25:50 ~11 min ios 📱ipa 📲
✔️ 78d6983 #2 2023-06-27 13:58:30 ~8 min android-e2e 🤖apk 📲
✔️ 78d6983 #2 2023-06-27 13:59:26 ~9 min tests 📄log
✔️ 78d6983 #2 2023-06-27 13:59:56 ~9 min android 🤖apk 📲
✔️ 78d6983 #2 2023-06-27 13:59:56 ~9 min ios 📱ipa 📲
✔️ 8f4f413 #3 2023-06-27 15:50:53 ~5 min android 🤖apk 📲
✔️ 8f4f413 #3 2023-06-27 15:51:18 ~6 min android-e2e 🤖apk 📲
✔️ 8f4f413 #3 2023-06-27 15:53:10 ~8 min ios 📱ipa 📲
✔️ 8f4f413 #3 2023-06-27 15:56:55 ~11 min tests 📄log
98e62e9 #4 2023-06-30 09:10:38 ~2 min tests 📄log
✔️ 98e62e9 #4 2023-06-30 09:13:14 ~5 min android 🤖apk 📲
✔️ 98e62e9 #4 2023-06-30 09:13:56 ~6 min android-e2e 🤖apk 📲
98e62e9 #5 2023-06-30 09:14:36 ~2 min tests 📄log
✔️ 98e62e9 #4 2023-06-30 09:17:38 ~9 min ios 📱ipa 📲
98e62e9 #6 2023-06-30 10:33:11 ~2 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c1026c2 #5 2023-06-30 13:35:26 ~6 min android 🤖apk 📲
✔️ c1026c2 #5 2023-06-30 13:35:26 ~6 min android-e2e 🤖apk 📲
✔️ c1026c2 #7 2023-06-30 13:36:53 ~7 min tests 📄log
✔️ c1026c2 #5 2023-06-30 13:42:09 ~13 min ios 📱ipa 📲
✔️ 07d037f #7 2023-07-02 06:53:18 ~6 min android 🤖apk 📲
✔️ 07d037f #9 2023-07-02 06:54:17 ~7 min tests 📄log
✔️ 07d037f #7 2023-07-02 06:56:23 ~9 min android-e2e 🤖apk 📲
✔️ 07d037f #7 2023-07-02 06:59:38 ~12 min ios 📱ipa 📲
✔️ 07d037f #8 2023-07-02 07:10:09 ~9 min ios 📱ipa 📲

nix/overlay.nix Outdated Show resolved Hide resolved
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.

Looks good, and appears to work. Nice.

Maybe we could add a README.md in nix/pkgs/watchman giving a short explanation of the issue this pinning solves and an example of error.

@jakubgs
Copy link
Member

jakubgs commented Jun 27, 2023

And you have context in the PR description but not in commit body.

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.

Double approved for great justice.

@siddarthkay
Copy link
Contributor

Tested your branch with these settings

❯ sysctl kern.maxfiles
kern.maxfiles: 524288

❯ sysctl kern.maxfilesperproc
kern.maxfilesperproc: 262144

❯ ulimit -n
262144

A lot of files were open at the same time because i ran make run-ios right after make run-android.
All this while the metro server was steady and did not crash.

This PR does solve the problem I think.
Good work @yakimant !

@yakimant yakimant force-pushed the nix_downgrade_watchman branch 2 times, most recently from 98e62e9 to c1026c2 Compare June 30, 2023 13:28
watchman was upgraded significantly during the last #14944 (4.9.0 (Aug 16, 2017) to 2023.01.30.00 - 6 years between):
status-im/nixpkgs@4e9c02b

Probably causing developers to have "too many files open" issue #16341

This PR is an attempt to fix the issue by downgrading the watchman

Signed-off-by: Jakub Sokołowski <jakub@status.im>
@jakubgs jakubgs merged commit 07d037f into develop Jul 2, 2023
@jakubgs jakubgs deleted the nix_downgrade_watchman branch July 2, 2023 07:11
@yakimant
Copy link
Member Author

yakimant commented Jul 3, 2023

Notes for future upgrades:

  • Try to reproduce the "too many files open" issue (lower the os limit first)
  • consider ignore in watchman config, node_module folder has too many files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Metro server crashes
5 participants