Enable building of DevTools and scheduling profiler in CI #19691

taneliang · 2020-08-25T13:57:32Z

Summary

This PR makes 4 separate changes to the CI config:

Re-enables building of main DevTools in CI. This was previously disabled in Disable DevTools build job to unblock master #19012. I'm not sure why builds were failing, but I'm guessing it's due to the build issue that Upgrade all @babel/* packages to fix DevTools builds #19647 fixed.
I've disabled parallelism in the DevTools build job because it looked like it was just doing the same thing 20 times across 20 executors 😆 I don't think this job is worth parallelizing as it isn't the longest one, but I can look into parallelizing this if we really wanted to.

Speed up yarn installs in circle builds #19566 replaced yarn install commands in every job with a central cache of the root node_modules folder. However, the node_modules folders in each package were not cached. Because yarn install was no longer run in each job, these node_modules folders were no longer generated.

If we SSH into a CI executor after the setup job:

circleci@a41afdc807a2:~/project$ ls packages/react-devtools-scheduling-profiler
README.md  package.json  src  webpack.config.js
circleci@a41afdc807a2:~/project$ ls packages/*/node_modules
ls: cannot access 'packages/*/node_modules': No such file or directory

This is a truncated list of the contents of all project-specific node_modules folders on my dev computer:

$ ls packages/*/node_modules
packages/eslint-plugin-react-hooks/node_modules:

packages/jest-mock-scheduler/node_modules:

packages/jest-react/node_modules:

packages/react-art/node_modules:

packages/react-client/node_modules:

packages/react-devtools-core/node_modules:
ws/

packages/react-devtools-extensions/node_modules:
ansi-styles/      browserify-zlib/        crc32-stream/        ...

packages/react-devtools-inline/node_modules:
ansi-styles/  '@babel'/   babel-eslint/   chalk/   ...

packages/react-devtools/node_modules:

packages/react-devtools-scheduling-profiler/node_modules:
acorn/               css-loader/            loader-utils/                       ...

packages/react-devtools-shared/node_modules:
semver/

packages/react-devtools-shell/node_modules:
ansi-styles/  '@babel'/   babel-eslint/   chalk/   ...

packages/react-dom/node_modules:

packages/react-interactions/node_modules:

packages/react-native-renderer/node_modules:
scheduler/

packages/react/node_modules:

packages/react-reconciler/node_modules:

packages/react-server/node_modules:

packages/react-transport-dom-relay/node_modules:
scheduler/

packages/react-transport-dom-webpack/node_modules:

packages/scheduler/node_modules:

Because of this, the scheduling profiler could not be built in CI. ~~This PR adds paths to all packages' node_modules folders (even if they don't exist) to the CI cache.~~ This PR calls yarn install during the DevTools build job as suggested by Enable building of DevTools and scheduling profiler in CI #19691 (comment). More details in this discussion thread: Enable building of DevTools and scheduling profiler in CI #19691 (comment).

Adds the scheduling profiler to the DevTools build script. We can eventually add another CI job to deploy the artifact to Vercel. Question (to @bvaughn): should I also compress the scheduling-profiler folder in the devtools.tgz artifact? UPDATE: As suggested in Enable building of DevTools and scheduling profiler in CI #19691 (comment), the scheduling profiler build is stored in a new devtools-scheduling-profiler.tgz artifact.

Test Plan

CI
Inspecting the devtools-scheduling-profiler.tgz artifact and serving the scheduling-profiler.

$ tar -xvzf devtools-scheduling-profiler.tgz
./
./app.js
./import.worker.worker.js.map
./static/
./static/media/
./static/media/profilerBrowser.8d782287.png
./import.worker.worker.js
./index.html
./app.js.map

This reverts commit 9273e65.

taneliang · 2020-08-25T13:59:03Z

.circleci/config.yml

+      # Store node_modules for all jobs in this workflow so that they don't
+      # need to each run a yarn install for each job. This will speed up all
+      # jobs run on this branch with the same lockfile. Split into multiple
+      # caches as there's a limited number of paths per cache.


This is the specific error we get if we put everything in one cache: https://support.circleci.com/hc/en-us/articles/360037695934-save-cache-step-fails-with-Error-uploading-archive-MetadataTooLarge-Your-metadata-headers-exceed-the-maximum-allowed-metadata-size-

codesandbox-ci · 2020-08-25T14:01:26Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ffcdff1:

Sandbox	Source
React	Configuration

sizebot · 2020-08-25T14:04:03Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against ffcdff1

sizebot · 2020-08-25T14:05:57Z

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against ffcdff1

Currently, the script simply does the same work 20 times. Seems like some work is needed to actually parallelize the work across executors, but since the job is already very short it doesn't make sense to parallelize it.

bvaughn · 2020-09-01T13:43:02Z

This is in my queue to review. I was on PTO last week though and I have a lot of catch up to do. I'll try to get to it sometime in the next couple of days. 😄

bvaughn · 2020-09-03T13:21:57Z

Question (to @bvaughn): should I also compress the scheduling-profiler folder in the devtools.tgz artifact?

Can we actually put this into a separate TGZ file? devtools.tgz is for things being deployed to Chrome/Firefox/Edge stores. This new thing gets deployed elsewhere, during a different workflow.

bvaughn · 2020-09-03T13:23:07Z

.circleci/config.yml

+    restore_cache:
+      name: Restore other packages node_modules cache
+      keys:
+        - v2-node-{{ arch }}-{{ .Branch }}-{{ checksum "yarn.lock" }}-package-node-modules


cc @rickhanlonii for a second set of eyes on the Circle CI changes.

bvaughn · 2020-09-03T13:25:00Z

.circleci/config.yml

+            - packages/react-transport-dom-webpack/node_modules
+            - packages/scheduler/node_modules
+            - packages/shared/node_modules
+            - packages/use-subscription/node_modules


This is something that will be easy to forget to update when we next add a new package. Will the (CI) failure behavior be obvious? (Will the error message reasonably direct folks to this bit of the config?)

Nope, there's no obvious failure behavior. You'll just get some missing packages, or some packages that are on the wrong version (which may cause silent errors). That's also why it took me some time to track down the build failure to these missing node_modules folders.

But I agree that this list is not a good idea. Here's another idea: maybe it'll be better if we had a script that cp -r packages/*/node_modules to a single folder, and cached that folder instead?

That sounds more robust.

Can we run a yarn install for only the tests that can't use the global cache? Is that only the devtools test?

Yeah we could do that. I'm just concerned that these missing folders may also cause problems in the future, so it would seem less risky to me if we just cached all node_modules folders. react-native-renderer and react-transport-dom-relay currently do have non-empty node_modules too (even though they only contain scheduler).

What do you think? I'll be happy to change this PR to either approach.

If we ever have 2+ nested deps that are different versions, this consolidated approach would cause problems.

Hmm, I was thinking of copying all the packages' node_modules folders into individual folders. So the cache will contain multiple package_node_modules/<package name>/node_modules folders alongside the root node_modules folder. This will allow us to restore all the packages' node_modules folders back to where yarn install created them. I think this should be pretty safe, if a little complicated.

My concern with running yarn install only for the new test is that other tests may suffer from silent problems if their conflicting deps aren't restored/installed. The set of conflicting deps may change over time as well.

Sorry then. I misunderstood what you were suggesting. That sounds fine. 👍

I think the consolidated cache idea seems problematic if we ever have conflicts between packages

Yarn workspaces hoist all of the dependencies in a package up to the root node_modules already unless there's a conflict. So to prevent conflicts, we could enforce all packages use the same version and add a test to compare the package.json files. What do you think about that?

We already have several conflicts. Some of them probably arbitrary due to transient deps, others perhaps intentional (like react-native-renderer + scheduler).

Btw this is what we do internally in xplat/js.

bvaughn · 2020-09-03T13:27:24Z

.circleci/config.yml

@@ -350,7 +452,9 @@ jobs:
    steps:
      - checkout
      - attach_workspace: *attach_workspace
-      - *restore_node_modules
+      - *restore_root_node_modules
+      - *restore_devtools_node_modules


I don't think we need to restore DevTools modules for most of these CI tasks, do we?

Just build_devtools_and_process_artifacts and yarn_test_build_devtools?

(And maybe we don't need to restore_package_node_modules for those two tasks either?)

Hmm, I wasn't sure what was and wasn't needed by the various CI tasks, so I figured it'll be safest to just restore everything. I'd expect e.g. the build, test, flow tasks to need everything restored?

Either way, the devtools and package caches are really cheap to restore (~0s) so maybe it won't be worth checking if they're necessary?

In any case, if we use the consolidated node_modules cache approach in my comment above we can go back to having a single cache :D

Single cache sounds nice :)

taneliang · 2020-09-03T15:20:51Z

Can we actually put this into a separate TGZ file? devtools.tgz is for things being deployed to Chrome/Firefox/Edge stores. This new thing gets deployed elsewhere, during a different workflow.

Okay! I'll do that tomorrow.

Regarding deployment, do you have any workflow for that in mind? Seems like we won't be able to integrate Vercel's bot normally because it doesn't have Java and so it can't build React. The best idea I've come up with so far is this: write a script to upload the compiled app to Vercel, and run that in a master-only CI job. Wondering if you have other ideas 😆

bvaughn · 2020-09-03T16:10:13Z

Regarding deployment, do you have any workflow for that in mind?

Haven't had the time to give it a lot of thought yet. Open for suggestions!

rickhanlonii · 2020-09-03T19:27:44Z

@taneliang instead of caching all the package node modules, could we run a yarn install for only the new test? The install should be fast enough because we're caching the global yarn cache, we just don't want to do it in the other jobs because of the parallelization.

bvaughn · 2020-09-14T13:01:06Z

Did this stall out or is it waiting on another review? Just noticed it had been a few days since anyone commented on it.

taneliang · 2020-09-14T14:26:29Z

Oops! Been swamped by schoolwork.

I'm also not sure how I should proceed. Should I do what @rickhanlonii originally suggested, I.e. remove the new caches and run yarn install in the devtools jobs? We can add that package.json test to prevent conflicting deps in a future PR.

bvaughn · 2020-09-14T14:42:57Z

No worries @taneliang.

Should I do what @rickhanlonii originally suggested

I don't have any objections to that.

taneliang · 2020-09-14T15:41:55Z

Sounds good! I'll do this over the coming weekend.

This reverts commit 9c4eaf8.

taneliang · 2020-09-21T10:57:23Z

All done!

Unfortunately it looks like this build_devtools_and_process_artifacts job is now the longest one. I'm not sure what we should do about this. The bulk of the time is spent building main DevTools stuff (see logs), so moving the scheduling profiler build into a separate job won't make it that much faster but can probably reduce the total time by ~20-30s. Should I do that?

taneliang · 2020-09-21T11:35:24Z

Actually, I'll split the scheduling profiler build into a separate job, if everyone's okay with that.

I intend to add a deploy job in a future PR, and it'll be dependent on the build job. Although we don't have to split the build job, it'll allow the deployment to be done in parallel with the main DevTools build.

This is what the final job tree will look like:

... -> yarn_build -> build_devtools_scheduling_profiler -> deploy_devtools_scheduling_profiler (I'll add this in a future PR, and it'll only be run on master)

bvaughn · 2020-09-21T13:37:00Z

@taneliang I'm looking at the most recent CI run for this branch:
https://app.circleci.com/pipelines/github/taneliang/react?branch=scheduling-profiler-ci

Looks like it just stopped running things midway through?

Am I misreading things maybe?

taneliang · 2020-09-21T13:46:48Z

Interesting. Looks like CircleCI is showing the CI runs from the project on my CircleCI account instead of Facebook's. Most of the jobs on my account aren't running because my free account doesn't have 20x parallelism.

Let me finish working on my deployment PR and I'll deactivate the project on my account, which should hopefully fix this.

bvaughn · 2020-09-21T13:47:53Z

Oh that is interesting.

Okay! :) Just ping me again when this is ready for a look. I'll work on fixing the Babel thing now.

taneliang · 2020-09-21T14:29:40Z

Disabled the project on my CircleCI account. Closing and reopening to kick CI.

taneliang · 2020-09-21T14:42:41Z

@bvaughn this is ready for review!

bvaughn · 2020-09-21T15:08:23Z

Excellent! Thanks 🙇 Reviewing now.

bvaughn · 2020-09-21T15:19:15Z

.circleci/config.yml

+      - attach_workspace: *attach_workspace
+      - *restore_yarn_cache
+      - *restore_node_modules
+      - run:


Shouldn't this be tagged with RELEASE_CHANNEL: experimental?

The scheduling profiler uses unstable_createRoot. The stable yarn_build command doesn't build a version of ReactDOM that exposes this API.

I don't think so, unless I'm missing something. The yarn_build CI job builds everything with the experimental channel. Because we depend on those packages, I don't think the scheduling profiler's build will be affected by a RELEASE_CHANNEL tag.

I've also confirmed that the built artifact works:

Oh, huh. I guess I'm a little out of date on our how Circle CI config is setup. build_devtools_and_process_artifacts specifies RELEASE_CHANNEL: experimental, but maybe it's unnecessary since, as you say, yarn_build is in the experimental channel (and RELEASE_CHANNEL_stable_yarn_build is stable)

Yeah, there's a good chance that that's unnecessary, but I didn't investigate that as it was already there 😆

…9691) Re-enables building of main DevTools in CI and add new CI target for building the scheduling profiler.

taneliang added 3 commits August 25, 2020 21:32

Revert "Disable DevTools build job to unblock master (facebook#19012)"

9c4fd01

This reverts commit 9273e65.

Add package/*/node_modules to CI cache

9c4eaf8

Enable scheduling profiler builds in CI

629e08c

taneliang commented Aug 25, 2020

View reviewed changes

facebook-github-bot added the CLA Signed label Aug 25, 2020

Disable parallelism for devtools build job

c8cabc4

Currently, the script simply does the same work 20 times. Seems like some work is needed to actually parallelize the work across executors, but since the job is already very short it doesn't make sense to parallelize it.

bvaughn self-requested a review August 31, 2020 14:54

bvaughn self-assigned this Aug 31, 2020

bvaughn reviewed Sep 3, 2020

View reviewed changes

bvaughn requested a review from rickhanlonii September 3, 2020 13:28

taneliang added 5 commits September 21, 2020 17:50

Merge branch 'master' into scheduling-profiler-ci

19fd7c3

Revert "Add package/*/node_modules to CI cache"

5b81533

This reverts commit 9c4eaf8.

Call yarn install in build DevTools job

0b1dada

Move scheduling profiler into separate artifact

e4c210d

Fix cd path

99620d4

taneliang added 2 commits September 21, 2020 20:10

Split scheduling profiler build into separate job

7a88935

Fix long build command name

ffcdff1

taneliang mentioned this pull request Sep 21, 2020

Add scheduling profiler deployment CI job #19874

Merged

4 tasks

taneliang closed this Sep 21, 2020

taneliang reopened this Sep 21, 2020

bvaughn reviewed Sep 21, 2020

View reviewed changes

bvaughn approved these changes Sep 21, 2020

View reviewed changes

bvaughn merged commit 6d73063 into facebook:master Sep 21, 2020

taneliang deleted the scheduling-profiler-ci branch September 22, 2020 08:23

koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021

Enable building of DevTools and scheduling profiler in CI (facebook#1…

2e1a149

…9691) Re-enables building of main DevTools in CI and add new CI target for building the scheduling profiler.

Enable building of DevTools and scheduling profiler in CI #19691

Enable building of DevTools and scheduling profiler in CI #19691

Conversation

taneliang commented Aug 25, 2020 • edited Loading

Summary

Test Plan

Choose a reason for hiding this comment

codesandbox-ci bot commented Aug 25, 2020 • edited Loading

sizebot commented Aug 25, 2020 • edited Loading

Size changes (experimental)

sizebot commented Aug 25, 2020 • edited Loading

Size changes (stable)

bvaughn commented Sep 1, 2020

bvaughn commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taneliang commented Sep 3, 2020

bvaughn commented Sep 3, 2020

rickhanlonii commented Sep 3, 2020

bvaughn commented Sep 14, 2020

taneliang commented Sep 14, 2020

bvaughn commented Sep 14, 2020

taneliang commented Sep 14, 2020

taneliang commented Sep 21, 2020

taneliang commented Sep 21, 2020

bvaughn commented Sep 21, 2020

taneliang commented Sep 21, 2020

bvaughn commented Sep 21, 2020 • edited Loading

taneliang commented Sep 21, 2020

taneliang commented Sep 21, 2020

bvaughn commented Sep 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taneliang commented Aug 25, 2020 •

edited

Loading

codesandbox-ci bot commented Aug 25, 2020 •

edited

Loading

sizebot commented Aug 25, 2020 •

edited

Loading

sizebot commented Aug 25, 2020 •

edited

Loading

bvaughn commented Sep 21, 2020 •

edited

Loading