Skip to content

Update content hash item limit to prevent Watchman crawl failed errors#2629

Merged
chipsnyder merged 2 commits intodevelopfrom
mobile/watchmanCache
Sep 18, 2020
Merged

Update content hash item limit to prevent Watchman crawl failed errors#2629
chipsnyder merged 2 commits intodevelopfrom
mobile/watchmanCache

Conversation

@chipsnyder
Copy link
Copy Markdown
Contributor

@chipsnyder chipsnyder commented Sep 16, 2020 •
edited
Loading

Related-PR WordPress/gutenberg#25385

Fixes:

While running the metro server locally we'll see the error:

Loading dependency graph...jest-haste-map: Watchman crawl failed. Retrying once with node crawler.
  Usually this happens when watchman isn't running. Create an empty `.watchmanconfig` file in your project's root folder or initialize a git or hg repository in your project.
  Error: Watchman error: too many pending cache jobs. Make sure watchman is running for this project. See https://facebook.github.io/watchman/docs/troubleshooting.html.

This error will occur when our file size has outgrown the default cache size of metro (more info in this SO answer) This increases the content hash size limit to prevent the error.

To test:

  1. Run npm run clean
  2. Run npm start --reset-cache

Expect The Metro server to start without displaying the cache failure error

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@chipsnyder chipsnyder mentioned this pull request Sep 16, 2020
Merged
6 tasks
chipsnyder
chipsnyder commented Sep 16, 2020
View reviewed changes
.watchmanconfig
@@ -1 +1,3 @@
{}
{
"content_hash_max_items": 400000
Copy link
Copy Markdown
Contributor Author

@chipsnyder chipsnyder Sep 16, 2020

Choose a reason for hiding this comment

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

This number was obtained by running find . | wc -l from the gutenberg-mobile root

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Sep 16, 2020 •
edited
Loading

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@chipsnyder
Copy link
Copy Markdown
Contributor Author

chipsnyder commented Sep 18, 2020

Hey @hypest Would I be able to get you to take a look at this PR as well? It's the same change as WordPress/gutenberg#25385 but handles it for the GB-mobile side.

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Sep 18, 2020

Hey @hypest Would I be able to get you to take a look at this PR as well?

👋 Chip, sure!

I still am not able to reproduce the issue, and this time I tried lowering the content_hash_max_items all the way down to just 40 and still can't reproduce. The default for that setting seems to be 131072 and I'd expect to see the issue if I have it too low. What do you think?

@chipsnyder
Copy link
Copy Markdown
Contributor Author

chipsnyder commented Sep 18, 2020

What do you think?

That seems odd to me that a clean and a reset isn't able to fix the issue for you. Do you happen to have a file in /etc/watchman.json? (https://facebook.github.io/watchman/docs/config.html)

I would think lowering the number to 40 would cause the error but perhaps there is a lower bound as well. 🤷‍♂️

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Sep 18, 2020

isn't able to fix the issue for you.

To be clear, it's not that the fix doesn't work for me but rather, I don't see the original issue so can't verify that the fix actually works.

The PR doesn't seem to break the build on my side anyway though so, I'm OK with merging since it fixes it for you. If the issue ends up not being fixed anyway we can revisit. How's that sound?

@chipsnyder
Copy link
Copy Markdown
Contributor Author

chipsnyder commented Sep 18, 2020

If the issue ends up not being fixed anyway we can revisit. How's that sound?

I think that's a good plan :) It's an easy enough line to remove if it does cause trouble.

hypest
hypest approved these changes Sep 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

LGTM!

Also, reminder to update to resolve the merge conflict.

@chipsnyder chipsnyder merged commit cca732f into develop Sep 18, 2020
@chipsnyder chipsnyder deleted the mobile/watchmanCache branch September 18, 2020 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pinarol pinarol Awaiting requested review from pinarol

1 more reviewer

@hypest hypest hypest approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chipsnyder chipsnyder

Labels

Tooling

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chipsnyder @hypest