🏗 Persist merge commit for reuse with later CI jobs by jridgewell · Pull Request #33165 · ampproject/amphtml

jridgewell · 2021-03-09T18:59:57Z

This commit is purposefully forked several commits before (768808e) the current master (d110566). I'm going to rebase and merge to see what happens.

jridgewell · 2021-03-09T20:29:02Z

First run forked from d110566, and CircleCI is "merging" it with c6b122b. Results:

Success so far!

build-system/tasks/bundle-size/index.js

jridgewell · 2021-03-09T20:29:57Z

I've now rebased on d876f32, creating 0f8d04b. Let's see how this one goes.

jridgewell · 2021-03-09T20:54:30Z

The rebase failed, but for a surprising reason! After the build, but before bundle-size, a new commit was merged onto master. So we built from c2f0984 (https://app.circleci.com/pipelines/github/ampproject/amphtml/4038/workflows/dfff0a99-8d4a-400a-a39a-f5065b1fdb7b/jobs/51946/parallel-runs/0/steps/0-107), but diffed size against cfe8f2f (https://app.circleci.com/pipelines/github/ampproject/amphtml/4038/workflows/dfff0a99-8d4a-400a-a39a-f5065b1fdb7b/jobs/51966/parallel-runs/0/steps/0-107).

rsimha · 2021-03-09T21:07:53Z

The rebase failed, but for a surprising reason! After the build, but before bundle-size, a new commit was merged onto master.

One way to avoid this is to report bundle sizes immediately after building in the same job (i.e., the old way). This probably means we'll need to merge the module and nomodule size reports on the server side instead of consolidating binaries during CI.

build-system/tasks/bundle-size/index.js

amp-owners-bot · 2021-03-09T22:34:14Z

Hey @rsimha! These files were changed:

.circleci/config.yml
.circleci/fetch_merge_commit.sh

jridgewell · 2021-03-09T23:21:38Z

The new persist logic worked! You can see we consistently used merge commit ba9a167, even though I purposefully merged #33062 after the build started (but before bundle-size started). This is proven by the changed commit log lines:

  *   ba9a1678e Justin Ridgewell - (HEAD -> pull/33165) Merge 0841e3d4929c06f6f0cbfa5938fe7594239e449d into 32a411de53fde79e349c6b636d8134acf070199d (4 minutes ago)
  |\  
  | * 0841e3d49 Justin Ridgewell - (origin/pull/33165) Fetch and store merge_commit at the beginning of the build (4 minutes ago)
- * | 32a411de5 Dima Voytenko - (origin/master, origin/HEAD, master) Multi-version: signal when extensions are known (#33162) (19 minutes ago)
+ * | 32a411de5 Dima Voytenko - Multi-version: signal when extensions are known (#33162) (27 minutes ago)

From that, we generated https://app.circleci.com/pipelines/github/ampproject/amphtml/4060/workflows/e50d7ea6-898d-455e-9c1e-ba6619708486/jobs/52305, and https://github.com/ampproject/amphtml/pull/33165/checks?check_run_id=2071240440.

jridgewell · 2021-03-09T23:27:55Z

I've now merged (using a merge commit) 6fc5bc6, creating 7f4eaee.

jridgewell · 2021-03-09T23:46:02Z

And we have a successful https://app.circleci.com/pipelines/github/ampproject/amphtml/4064/workflows/1aa4cfb0-f652-4920-a78b-09e7dff2d126/jobs/52366/parallel-runs/0/steps/0-108 and https://github.com/ampproject/amphtml/pull/33165/checks?check_run_id=2071429754.

rsimha

Thanks for investigating this and prototyping a solution! You've managed to discover the elusive crux of the problem: a commit sneaking into master between build stages of a CI build.

However, I think the solution can be made more efficient at the minimum, and potentially be done without adding a new CI stage. Posting an initial comment inline, will follow up after doing some homework.

.circleci/config.yml

rsimha · 2021-03-10T00:15:37Z

And we have a successful app.circleci.com/pipelines/github/ampproject/amphtml/4064/workflows/1aa4cfb0-f652-4920-a78b-09e7dff2d126/jobs/52366/parallel-runs/0/steps/0-108 and #33165 (checks).

The "compare" link on this check #33165 (checks) shows code from other PRs, I believe it too needs to use the merge commit SHA instead of gitCommitHash().

jridgewell

The "compare" link on this check #33165 (checks) shows code from other PRs, I believe it too needs to use the merge commit SHA instead of gitCommitHash().

I agree partially. The comparison is definitely wrong, but I think the HEAD and BASE are should be reported as they are. We should instead change the code changes link to MERGE?

.circleci/config.yml

rsimha

I agree partially. The comparison is definitely wrong, but I think the HEAD and BASE are should be reported as they are. We should instead change the code changes link to MERGE?

Today, CI builds only report the base SHA to the bundle-size app (along with the actual sizes).

amphtml/build-system/tasks/bundle-size/index.js

Lines 188 to 191 in c121292

    
           body: { 
        
             baseSha, 
        
             bundleSizes: await getBrotliBundleSizes(), 
        
           },

The app infers the head SHA and constructs the comparison link. We'll likely need to change this API to properly fix the comparison link.

.circleci/config.yml

jridgewell · 2021-03-10T01:51:51Z

Added a mergeSha field to the bundle-size endpoint POST. We just need to update the API to use it.

rsimha

A few comments based on your latest changes.

.circleci/config.yml

.circleci/fetch_merge_commit.sh

rsimha · 2021-03-10T02:06:42Z

build-system/common/ci.js

+/**
+ * Returns the merge commits SHA when running a CI Pull Request build.
+ * @return {string}
+ */
+function ciMergeSha() {
+  // CIRCLE_MERGE_SHA is populated by .circleci/fetch_merge_commit.sh.
+  return isCircleci ? env('CIRCLE_MERGE_SHA') : '';
+}


The thumb-rule for every function in this file is that it should be usable on GH actions if ever required, and that it is based on well-known env vars from each platform.

amphtml/build-system/common/ci.js

Lines 18 to 24 in c121292

/**

* @fileoverview Provides various kinds of CI state.

*

* References:

* GitHub Actions: https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables

* CircleCI: https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables

*/

For CIRCLE_MERGE_SHA, I wonder if we should just add a util function within bundle-size/index.js.

There's not an equivalent GH env.

For CIRCLE_MERGE_SHA, I wonder if we should just add a util function within bundle-size/index.js.

I wanted to avoid tying the setup scripts to the actual JS code.

SG. Let's just rename this function to circleciPrMergeSha to clearly indicate that it's not a generic CI function.

According to https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request, the GITHUB_SHA is actually the merge commit! And we're mixing up the CCI SHA (which is the HEAD SHA) with the GH SHA (which is the merge SHA) in ciCommitSha above.

I'll rename this for now, and leave it to you to cleanup the env vars.

ciCommitSha is only used during push builds, and according to these docs, GITHUB_SHA is the correct variable.

I will admit that ci.js has gotten confusing due to all the individual use cases, and could do with a separate cleanup.

Other cleanup is coming up in #33215.

rsimha

LGTM. Thanks for patiently dealing with all my review comments!

rsimha · 2021-03-10T02:49:47Z

build-system/common/ci.js

+/**
+ * Returns the merge commits SHA when running a CI Pull Request build.
+ * @return {string}
+ */
+function ciMergeSha() {
+  // CIRCLE_MERGE_SHA is populated by .circleci/fetch_merge_commit.sh.
+  return isCircleci ? env('CIRCLE_MERGE_SHA') : '';
+}


SG. Let's just rename this function to circleciPrMergeSha to clearly indicate that it's not a generic CI function.

.circleci/fetch_merge_commit.sh

rsimha · 2021-03-10T03:02:25Z

build-system/common/ci.js

+ */
+function ciMergeSha() {
+  // CIRCLE_MERGE_SHA is populated by .circleci/fetch_merge_commit.sh.
+  return isCircleci ? env('CIRCLE_MERGE_SHA') : '';


Should this condition be isPullRequestBuild() && isCircleciBuild() to guarantee an empty value during push builds?

jridgewell · 2021-03-10T04:42:24Z

Lol, deleting my branch borked master. I restored it until #33176 can be merged.

Uses the [new `mergeSha` field](https://github.com/ampproject/amphtml/pull/33165/files#diff-c141453bcbdbe03ca8414e5b4d425c5b7323e43d524b614455ce13770633509eR191), which resolves ampproject/amphtml#33165 (comment).

* bundle-size: Use mergeSha when showing code changes Uses the [new `mergeSha` field](https://github.com/ampproject/amphtml/pull/33165/files#diff-c141453bcbdbe03ca8414e5b4d425c5b7323e43d524b614455ce13770633509eR191), which resolves ampproject/amphtml#33165 (comment). * Review updates * Add todo

jridgewell requested review from danielrozenberg and rsimha March 9, 2021 18:59

jridgewell force-pushed the build-size-base branch 3 times, most recently from 778cb10 to 0f8d04b Compare March 9, 2021 20:26

danielrozenberg reviewed Mar 9, 2021

View reviewed changes

build-system/tasks/bundle-size/index.js Outdated Show resolved Hide resolved

rsimha reviewed Mar 9, 2021

View reviewed changes

build-system/tasks/bundle-size/index.js Show resolved Hide resolved

jridgewell force-pushed the build-size-base branch 5 times, most recently from e6259bd to fbebb49 Compare March 9, 2021 23:01

Fetch and store merge_commit at the beginning of the build

0841e3d

jridgewell force-pushed the build-size-base branch from fbebb49 to 0841e3d Compare March 9, 2021 23:04

jridgewell changed the title ~~Use the current master as the base point of bundle-size checks~~ 🏗 Persist merge commit for reuse with later CI jobs Mar 9, 2021

jridgewell added 2 commits March 9, 2021 18:24

Log MERGE_SHA during Setup ENV stage

a9a56ae

Merge remote-tracking branch 'upstream/master' into build-size-base

7f4eaee

Fix GET_MERGE_SHA

6b60438

rsimha suggested changes Mar 10, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

jridgewell commented Mar 10, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

Simplify and speed up fetch_merge_commit

6171d55

jridgewell added 2 commits March 9, 2021 19:56

Curl and bash

53e7af2

Fixes

8c98d69

jridgewell mentioned this pull request Mar 10, 2021

WIP: Fetch and store merge_commit at the beginning of the build #33174

Closed

Try Docker image for faster Setup?

d891e84

jridgewell force-pushed the build-size-base branch from d67174f to d891e84 Compare March 10, 2021 01:18

Add comment describing docker image

8786557

rsimha reviewed Mar 10, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

Report merge SHA to bundle-size endpoint

10a19ef

rsimha reviewed Mar 10, 2021

View reviewed changes

jridgewell added 2 commits March 9, 2021 21:20

Review updates

260d2b9

Fix docker executor

5b98694

rsimha approved these changes Mar 10, 2021

View reviewed changes

rsimha reviewed Mar 10, 2021

View reviewed changes

Rename ciMergeSha

546b278

jridgewell merged commit 980f0ef into ampproject:master Mar 10, 2021

jridgewell deleted the build-size-base branch March 10, 2021 03:38

jridgewell restored the build-size-base branch March 10, 2021 04:42

jridgewell deleted the build-size-base branch March 10, 2021 05:16

jridgewell mentioned this pull request Mar 10, 2021

bundle-size: Use mergeSha when showing code changes ampproject/amp-github-apps#1241

Merged

rsimha mentioned this pull request Mar 12, 2021

🏗 Clean up CircleCI merge SHA logic #33215

Merged

jridgewell mentioned this pull request Mar 15, 2021

wg-performance Status Update 2021-03-15 ampproject/wg-performance#73

Closed

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Mar 17, 2021

ampprojectbot added the PR use: In LTS label Apr 12, 2021

	/**
	* @fileoverview Provides various kinds of CI state.
	*
	* References:
	* GitHub Actions: https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables
	* CircleCI: https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables
	*/

Conversation

jridgewell commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jridgewell commented Mar 9, 2021

Uh oh!

jridgewell commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rsimha commented Mar 9, 2021

Uh oh!

Uh oh!

amp-owners-bot bot commented Mar 9, 2021

Uh oh!

jridgewell commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented Mar 9, 2021

Uh oh!

rsimha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rsimha commented Mar 10, 2021

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rsimha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jridgewell commented Mar 10, 2021

Uh oh!

rsimha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rsimha Mar 10, 2021 • 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!

jridgewell Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

rsimha Mar 10, 2021 • 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!

jridgewell Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

rsimha Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

rsimha left a comment

Choose a reason for hiding this comment

Uh oh!

rsimha Mar 10, 2021 • 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!

Uh oh!

rsimha Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

jridgewell commented Mar 10, 2021

Uh oh!

Reviewers

jridgewell commented Mar 9, 2021 •

edited

Loading

jridgewell commented Mar 9, 2021 •

edited

Loading

jridgewell commented Mar 9, 2021 •

edited

Loading

jridgewell commented Mar 9, 2021 •

edited

Loading

jridgewell commented Mar 9, 2021 •

edited

Loading

rsimha Mar 10, 2021 •

edited

Loading

rsimha Mar 10, 2021 •

edited

Loading

rsimha Mar 10, 2021 •

edited

Loading