don’t use watchman when not watching by robrichard · Pull Request #1966 · facebook/relay

robrichard · 2017-07-15T03:50:33Z

Fixes #1644

This PR will update relay-compiler to not use watchman unless watching files.

robrichard · 2017-07-15T04:05:30Z

@wincent @kassens let me know if you think this is the right approach?

robrichard · 2017-07-17T15:44:12Z

The failing check here is also failing on master

wincent · 2017-07-17T16:02:34Z

let me know if you think this is the right approach?

Yeah this looks pretty good, although I would suggest first trying to use Watchman even in the non-watch code-path (it will be much faster on a very large repo) before falling back to a non-Watchman alternative.

robrichard · 2017-07-17T16:12:29Z

I actually found fast-glob to be about 25% faster than watchman on my projects, but I can switch it if you prefer.

sibelius · 2017-07-17T16:29:30Z

can we have this as an option?

so we can use fast-glob or watchman based on this

wincent · 2017-07-17T16:39:20Z

I actually found fast-glob to be about 25% faster than watchman on my projects, but I can switch it if you prefer.

How large are your projects? Watchman was built to support repos with well over a million files in them: in these large scenarios, querying an always-up-to-date, in-memory index that has been prepared ahead of time is always going to be faster than actually stat-ing the filesystem, even when the OS's filesystem caches are hot.

robrichard · 2017-07-17T19:08:01Z

@wincent @sibelius I pushed another commit that uses watchman by default to query files. It will fall back to fast-glob if either watchman is not installed or the --noWatchman option is passed.

junosuarez · 2017-07-19T22:59:29Z

we got this working on Travis by adding:

before_install:
  # dirty ugly watchman hack https://github.com/facebook/relay/issues/1644#issuecomment-315998313
  - if [[ ! -e watchman ]]; then git clone https://github.com/facebook/watchman.git && cd watchman/ && git checkout v4.7.0 && ./autogen.sh && ./configure && make  && sudo make install; fi
  - sudo sysctl fs.inotify.max_user_watches=524288
  - sudo sysctl -p
  - cd ~/build/$TRAVIS_REPO_SLUG
  # end dirty watchman hack```

leebyron · 2017-07-28T18:30:50Z

      describe: 'More verbose logging',
      type: 'boolean',
    },
+    'noWatchman': {


Let's avoid double-negative options. How about just a watchman flag which defaults to true, so --watchman=false to disable?

leebyron · 2017-07-28T18:32:21Z

-  client.end();
-  return updateFiles(new Set(), baseDir, filter, resp.files);
+  const {baseDir, include, extensions, exclude} = filterOptions;
+  if (


Let's refactor this into two functions since this is getting a bit complicated. How about queryFilesWithWatchman and queryFilesWithGlob and you can call them from this condition?

leebyron · 2017-07-28T18:33:39Z

+      const client = new RelayWatchmanClient();
+      client.on('error', () => {
+        resolve(false);
+      });


minor note: this could be syntactically simplified slightly to:

client.on('error', () => resolve(false));

leebyron · 2017-07-28T18:36:59Z

+      });
+      client.hasCapability('relative_root')
+        .then(() => {
+          client.end();


Should we really be creating a client, and then ending it like this? Only to create it again in this success case?

Perhaps a better API would be something like createIfAvailable() which promises to resolve to an instance of RelayWatchmanClient if "available" (has a relative_root) or otherwise returns null, which could then be detected in the code above

robrichard · 2017-07-28T21:03:50Z

@leebyron thanks for taking the time to review. I pushed up another commit to address your comments. Please let me know if there is anything else you think I should change.

leebyron · 2017-07-28T21:53:22Z

+      const client = new RelayWatchmanClient();
+      client.on('error', () => resolve(null));
+      client.hasCapability('relative_root')
+        .then(() => resolve(client));


Right now this is always resolving to the client, regardless of the return value of relative_root - is that intended? Do you mean for this to be

.then(hasRelativeRoot => hasRelativeRoot ? client : null)

antmdvs · 2017-07-28T21:55:16Z

-    'noWatchman': {
-      describe: 'Do not use watchman when not in watch mode',
+    'watchman': {
+      describe: 'Use watchman when not in watch mode',


I'm confused. Is watchman still used when not in watch mode?

Watchman will always be used in watch mode. Watchman will also still be used when not in watch mode unless --watchman=false is passed or we detect watchman is not installed.

I agree that it's confusing that you could specify that watchman shouldn't be used but also that you want to use watch mode. I think in that scenario it's better to stop and report an error instead of disregarding wishes to not use watchman

@leebyron I added this check now

leebyron · 2017-07-28T21:56:03Z

+  static createIfAvailable(): Promise<?RelayWatchmanClient> {
+    return new Promise((resolve) => {
+      const client = new RelayWatchmanClient();
+      client.on('error', () => resolve(null));


I'm curious if there are certain kinds of errors which should cause this to be determined as "unavailable" as opposed to considering all errors? I'm interested in what you saw during investigating writing this PR that led you to this

I based it on the node watchman docs here where they check capabilities for relative_root.

Invoking client.hasCapability when watchman is not installed will cause the on('error') callback to be invoked (or a Unhandled 'error' event to be thrown if not implemented) and never execute the function in the then callback. I'm not able to catch this error by chaining a .catch or rewriting as an async function and wrapping in a try/catch. I'm investigating why that's the case now.

I could check the error in on('error') to make sure it's ENOENT before resolving null.

@leebyron It seems like fb-watchman does not work as documented in the link above. The only way I found to handle the error from watchman not being installed is through the on('error') callback. I opened an issue on the watchman repo with a minimal example (facebook/watchman#509). How do you think I should proceed?

Let's try out your current implementation and adjust from there if anything goes wrong. Thanks for opening the issue on watchman, hopefully someone who knows more about it than I do can help us out.

facebook-github-bot · 2017-07-29T00:18:54Z

@leebyron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

leebyron

See my inline comment, this PR will need a new approach to be mergeable. My suggestion is to revert all changes to RelayCodegenWatcher and RelayCodegenRunner and create only additive changes to RelayCodegenRunner instead and focus the majority of the PR on RelayCompilerBin

Right now RelayCodegenRunner expects a watchmanExpression, however perhaps it could expect that or an array of filenames (via the output of fast-glob, called when --watchman=false is provided). Since that only adds capabilities without removing others, it wouldn't be a breaking change.

leebyron · 2017-07-29T02:47:47Z

+  extensions: Array<string>,
+  include: Array<string>,
+  exclude: Array<string>,
+  watchman: boolean,


I was working on merging this, but unfortunately this breaking change will prevent me from doing so.

This RelayCodegenRunner is public API, and it's used extensively by Relay projects internally at Facebook which provide complex watchmanExpression that cannot be replicated simply with include/exclude.

leebyron · 2017-07-29T02:53:43Z

          expression: [
            'allof',
-            config.watchmanExpression,
+            RelayCodegenWatcher.buildWatchExpression(config),


I think this change here won't be enough - if watchman is disabled, then calling getDirtyWriters will fail here.

robrichard · 2017-07-31T15:16:25Z

@leebyron can you take another look? I updated the PR to incorporate your suggestions. There are no longer any breaking changes to any modules.

leebyron · 2017-08-01T21:32:24Z

This is really nice, thanks @robrichard - let me work on merging this

facebook-github-bot · 2017-08-01T21:32:38Z

@leebyron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Hey leebyron, I noticed after #1966 was merged the code was amended to create a watchman client solely for purposes of testing if watchman is available. Unfortunately this client is never closed so the process will hang indefinitely. Closes #2010 Reviewed By: alangenfeld Differential Revision: D5548547 Pulled By: leebyron fbshipit-source-id: 69b164558f6121e80d68b9579834adc55264ce55

The unneeded dependency should be fixed by newer Relay: facebook/relay#1966

facebook-github-bot added the CLA Signed label Jul 15, 2017

robrichard force-pushed the no-watchman branch 3 times, most recently from 7f799b3 to fd34428 Compare July 15, 2017 18:59

robrichard mentioned this pull request Jul 17, 2017

Make Watchman a non-hard dependency #1644

Closed

robrichard force-pushed the no-watchman branch 2 times, most recently from 7f282a4 to ebd49af Compare July 17, 2017 21:16

robrichard force-pushed the no-watchman branch from ebd49af to bfb0171 Compare July 28, 2017 17:50

leebyron reviewed Jul 28, 2017

View reviewed changes

antmdvs reviewed Jul 28, 2017

View reviewed changes

leebyron reviewed Jul 28, 2017

View reviewed changes

leebyron suggested changes Jul 29, 2017

View reviewed changes

Fallback to glob when watchman is not available

a4b87a2

robrichard force-pushed the no-watchman branch from dfd50ed to a4b87a2 Compare July 31, 2017 14:58

sibelius mentioned this pull request Aug 1, 2017

[relay-compiler] watchman is required even if watch option is not passed #2001

Closed

facebook-github-bot closed this in 4f80e02 Aug 2, 2017

robrichard mentioned this pull request Aug 2, 2017

relay-compiler hangs when watchman is installed #2010

Closed

kmjennison added a commit to gladly-team/tab that referenced this pull request Nov 16, 2017

Do not install Watchman on TravisCI.

7d7fed8

The unneeded dependency should be fixed by newer Relay: facebook/relay#1966

pranaygp mentioned this pull request Jan 11, 2018

relay-compiler --watchman false option fails with FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. when --src is current directory #2042

Open

robrichard deleted the no-watchman branch November 3, 2020 18:48

Conversation

robrichard commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robrichard commented Jul 15, 2017

Uh oh!

robrichard commented Jul 17, 2017

Uh oh!

wincent commented Jul 17, 2017

Uh oh!

robrichard commented Jul 17, 2017

Uh oh!

sibelius commented Jul 17, 2017

Uh oh!

wincent commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robrichard commented Jul 17, 2017

Uh oh!

junosuarez commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrichard commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 29, 2017

Uh oh!

leebyron left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrichard commented Jul 31, 2017

Uh oh!

leebyron commented Aug 1, 2017

Uh oh!

facebook-github-bot commented Aug 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

robrichard commented Jul 15, 2017 •

edited

Loading

wincent commented Jul 17, 2017 •

edited

Loading

junosuarez commented Jul 19, 2017 •

edited

Loading

leebyron left a comment •

edited

Loading