From 4534750ab7ca52f7b38204f976b5dccd180ed4c5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 2 Feb 2021 19:18:58 -0600 Subject: [PATCH] Migrate sizebot to combined workflow Replaces the two separate sizebot jobs (one for each channel, stable and experimental) with a single combined job that outputs size information for all bundles in a single GitHub comment. I didn't attempt to simplify the output at all, but we should. I think what I would do is remove our custom Rollup sizes plugin, and read the sizes from the filesystem instead. We would lose some information about the build configuration used to generate each artifact, but that can be inferred from the filepath. For example, the filepath `fb-www/ReactDOM-dev.classic.js` already tells us everything we need to know about the artifact. Leaving this for a follow up. --- .circleci/config.yml | 84 ++++++-------------- dangerfile.js | 185 +++++++++++++++---------------------------- 2 files changed, 90 insertions(+), 179 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 1c049c8e6b2d2..d0913051bd1c2 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -170,17 +170,9 @@ jobs: - *restore_node_modules - run: yarn build-combined - persist_to_workspace: - root: build2 + root: . paths: - - facebook-www - - facebook-react-native - - facebook-relay - - oss-stable - - oss-experimental - - react-native - - dist - - sizes-stable - - sizes-experimental + - build2 get_base_build: docker: *docker @@ -195,21 +187,14 @@ jobs: # branch. Once they do, replace with `merge-base HEAD origin/master` command: | git fetch origin master + echo "$(merge-base HEAD origin/master)" cd ./scripts/release && yarn && cd ../../ scripts/release/download-experimental-build.js --commit=6b8d4ca mv ./build2 ./base-build - persist_to_workspace: - root: base-build + root: . paths: - - facebook-www - - facebook-react-native - - facebook-relay - - oss-stable - - oss-experimental - - react-native - - dist - - sizes-stable - - sizes-experimental + - base-build process_artifacts_combined: docker: *docker @@ -217,7 +202,7 @@ jobs: steps: - checkout - attach_workspace: - at: build2 + at: . - run: yarn workspaces info | head -n -1 > workspace_info.txt - *restore_node_modules - run: echo "<< pipeline.git.revision >>" >> build2/COMMIT_SHA @@ -226,6 +211,19 @@ jobs: - store_artifacts: path: ./build2.tgz + sizebot: + docker: *docker + environment: *environment + steps: + - checkout + - attach_workspace: + at: . + - run: echo "<< pipeline.git.revision >>" >> build2/COMMIT_SHA + - run: yarn workspaces info | head -n -1 > workspace_info.txt + - *restore_node_modules + - run: + command: node ./scripts/tasks/danger + build_devtools_and_process_artifacts: docker: *docker environment: *environment @@ -292,38 +290,6 @@ jobs: process_artifacts: *process_artifacts process_artifacts_experimental: *process_artifacts - sizebot_stable: - docker: *docker - environment: *environment - steps: - - checkout - - attach_workspace: *attach_workspace - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - # This runs in the process_artifacts job, too, but it's faster to run - # this step in both jobs instead of running the jobs sequentially - - run: node ./scripts/rollup/consolidateBundleSizes.js - - run: - environment: - RELEASE_CHANNEL: stable - command: node ./scripts/tasks/danger - - sizebot_experimental: - docker: *docker - environment: *environment - steps: - - checkout - - attach_workspace: *attach_workspace - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - # This runs in the process_artifacts job, too, but it's faster to run - # this step in both jobs instead of running the jobs sequentially - - run: node ./scripts/rollup/consolidateBundleSizes.js - - run: - environment: - RELEASE_CHANNEL: experimental - command: node ./scripts/tasks/danger - yarn_lint_build: docker: *docker environment: *environment @@ -372,7 +338,7 @@ jobs: steps: - checkout - attach_workspace: - at: build2 + at: . - run: yarn workspaces info | head -n -1 > workspace_info.txt - *restore_node_modules - run: yarn test --build <> --ci @@ -425,9 +391,6 @@ workflows: - process_artifacts: requires: - RELEASE_CHANNEL_stable_yarn_build - - sizebot_stable: - requires: - - RELEASE_CHANNEL_stable_yarn_build - RELEASE_CHANNEL_stable_yarn_lint_build: requires: - RELEASE_CHANNEL_stable_yarn_build @@ -444,9 +407,6 @@ workflows: - process_artifacts_experimental: requires: - yarn_build - - sizebot_experimental: - requires: - - yarn_build - yarn_lint_build: requires: - yarn_build @@ -529,6 +489,10 @@ workflows: - get_base_build: requires: - setup + - sizebot: + requires: + - get_base_build + - yarn_build_combined fuzz_tests: triggers: - schedule: diff --git a/dangerfile.js b/dangerfile.js index 86e7e422a5b40..209d7a284926d 100644 --- a/dangerfile.js +++ b/dangerfile.js @@ -26,34 +26,10 @@ // `DANGER_GITHUB_API_TOKEN=[ENV_ABOVE] yarn danger pr https://github.com/facebook/react/pull/11865 const {markdown, danger, warn} = require('danger'); -const fetch = require('node-fetch'); const {generateResultsArray} = require('./scripts/rollup/stats'); -const {existsSync, readFileSync} = require('fs'); -const {exec} = require('child_process'); - -// This must match the name of the CI job that creates the build artifacts -const RELEASE_CHANNEL = - process.env.RELEASE_CHANNEL === 'experimental' ? 'experimental' : 'stable'; -const artifactsJobName = - process.env.RELEASE_CHANNEL === 'experimental' - ? 'process_artifacts_experimental' - : 'process_artifacts'; - -if (!existsSync('./build/bundle-sizes.json')) { - // This indicates the build failed previously. - // In that case, there's nothing for the Dangerfile to do. - // Exit early to avoid leaving a redundant (and potentially confusing) PR comment. - warn( - 'No bundle size information found. This indicates the build ' + - 'job failed.' - ); - process.exit(0); -} - -const currentBuildResults = JSON.parse( - readFileSync('./build/bundle-sizes.json') -); +const {readFileSync, readdirSync} = require('fs'); +const path = require('path'); /** * Generates a Markdown table @@ -100,99 +76,23 @@ function setBoldness(row, isBold) { } } -/** - * Gets the commit that represents the merge between the current branch - * and master. - */ -function git(args) { - return new Promise(res => { - exec('git ' + args, (err, stdout, stderr) => { - if (err) { - throw err; - } else { - res(stdout.trim()); - } - }); - }); -} - -(async function() { - // Use git locally to grab the commit which represents the place - // where the branches differ - const upstreamRepo = danger.github.pr.base.repo.full_name; - if (upstreamRepo !== 'facebook/react') { - // Exit unless we're running in the main repo - return; - } - - markdown(`## Size changes (${RELEASE_CHANNEL})`); - - const upstreamRef = danger.github.pr.base.ref; - await git(`remote add upstream https://github.com/facebook/react.git`); - await git('fetch upstream'); - const baseCommit = await git(`merge-base HEAD upstream/${upstreamRef}`); - - let previousBuildResults = null; - try { - let baseCIBuildId = null; - const statusesResponse = await fetch( - `https://api.github.com/repos/facebook/react/commits/${baseCommit}/status` - ); - const {statuses, state} = await statusesResponse.json(); - if (state === 'failure') { - warn(`Base commit is broken: ${baseCommit}`); - return; - } - for (let i = 0; i < statuses.length; i++) { - const status = statuses[i]; - if (status.context === `ci/circleci: ${artifactsJobName}`) { - if (status.state === 'success') { - baseCIBuildId = /\/facebook\/react\/([0-9]+)/.exec( - status.target_url - )[1]; - break; - } - if (status.state === 'pending') { - warn(`Build job for base commit is still pending: ${baseCommit}`); - return; - } - } +function getBundleSizes(pathToSizesDir) { + const filenames = readdirSync(pathToSizesDir); + let bundleSizes = []; + for (let i = 0; i < filenames.length; i++) { + const filename = filenames[i]; + if (filename.endsWith('.json')) { + const json = readFileSync(path.join(pathToSizesDir, filename)); + bundleSizes.push(...JSON.parse(json).bundleSizes); } - - if (baseCIBuildId === null) { - warn(`Could not find build artifacts for base commit: ${baseCommit}`); - return; - } - - const baseArtifactsInfoResponse = await fetch( - `https://circleci.com/api/v1.1/project/github/facebook/react/${baseCIBuildId}/artifacts` - ); - const baseArtifactsInfo = await baseArtifactsInfoResponse.json(); - - for (let i = 0; i < baseArtifactsInfo.length; i++) { - const info = baseArtifactsInfo[i]; - if (info.path.endsWith('bundle-sizes.json')) { - const resultsResponse = await fetch(info.url); - previousBuildResults = await resultsResponse.json(); - break; - } - } - } catch (error) { - warn(`Failed to fetch build artifacts for base commit: ${baseCommit}`); - return; - } - - if (previousBuildResults === null) { - warn(`Could not find build artifacts for base commit: ${baseCommit}`); - return; } + return {bundleSizes}; +} +async function printResultsForChannel(baseResults, headResults) { // Take the JSON of the build response and // make an array comparing the results for printing - const results = generateResultsArray( - currentBuildResults, - previousBuildResults - ); + const results = generateResultsArray(baseResults, headResults); const packagesToShow = results .filter( @@ -281,15 +181,62 @@ function git(args) {
Details of bundled changes. -

Comparing: ${baseCommit}...${danger.github.pr.head.sha}

- - ${allTables.join('\n')}
`; - markdown(summary); + return summary; } else { - markdown('No significant bundle size changes to report.'); + return 'No significant bundle size changes to report.'; } +} + +(async function() { + // Use git locally to grab the commit which represents the place + // where the branches differ + + const upstreamRepo = danger.github.pr.base.repo.full_name; + if (upstreamRepo !== 'facebook/react') { + // Exit unless we're running in the main repo + return; + } + + let headSha; + let headSizesStable; + let headSizesExperimental; + + let baseSha; + let baseSizesStable; + let baseSizesExperimental; + + try { + headSha = (readFileSync('./build2/COMMIT_SHA') + '').trim(); + headSizesStable = getBundleSizes('./build2/sizes-stable'); + headSizesExperimental = getBundleSizes('./build2/sizes-experimental'); + + baseSha = (readFileSync('./base-build/COMMIT_SHA') + '').trim(); + baseSizesStable = getBundleSizes('./base-build/sizes-stable'); + baseSizesExperimental = getBundleSizes('./base-build/sizes-experimental'); + } catch { + warn( + "Failed to read build artifacts. It's possible a build configuration " + + 'has changed upstream. Try pulling the latest changes from the ' + + 'main branch.' + ); + return; + } + + markdown(` +## Size changes + +

Comparing: ${baseSha}...${headSha}

+ +### Stable channel + +${await printResultsForChannel(baseSizesStable, headSizesStable)} + +### Experimental channel + +${await printResultsForChannel(baseSizesExperimental, headSizesExperimental)} +`); })();