Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc(build): give build-tracker a shared git history on PRs #11449

Merged
merged 10 commits into from
Nov 20, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ jobs:
- run: yarn i18n:checks
- run: yarn dogfood-lhci

# buildtracker runs `git merge-base HEAD origin/master` which needs more history than depth=1. https://github.com/paularmstrong/build-tracker/issues/106
- name: Deepen git fetch (for buildtracker)
run: git fetch --deepen=100
# buildtracker needs history and a common merge commit.
Copy link
Collaborator

@connorjclark connorjclark Sep 21, 2020

Choose a reason for hiding this comment

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

Could we drop the entire script if we just skip this step if the current branch isn't master? so it only runs on commits merged to master.

Copy link
Member Author

Choose a reason for hiding this comment

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

this script is mostly necessary for PRs.

though on master we need we need just 1 command... the git fetch --deepen.
the bash script early exists (line 26) in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we want it tracking PR? It doesn't ever fail, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

we want master for timeseries historical data

we want prs for budgets. once bt is running successfully i can add bundle size budgets, which is the last remaining item to complete the move from the old bundlesize module to this.
buildtracker CLI won't have a non-zero exit code, even with failing budgets.. but i'll likely post to the status API to express the failure state

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to check against a budget w/o uploading the data for the PR revision (and thus, no need to do this complex git stuff)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If build tracker UI can't filter to just master commits, I'm hesitant to mix the two data sources.

Copy link
Member Author

Choose a reason for hiding this comment

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

If build tracker UI can't filter to just master commits, I'm hesitant to mix the two data sources.

totally. the UI filters to just master/default branch by default. so they're not mixed.

Is there a way to check against a budget w/o uploading the data for the PR revision (and thus, no need to do this complex git stuff)?

nope. while you can set 3 types of budgets (SIZE, DELTA, PERCENT_DELTA), the type we want is DELTA, which needs to know the baseline size, and only the server knows that. so in BT, the budgets are configured against the server instead of the client who's doing the build.

i asked the buildtracker maintainer about uploading non-master stuff and he said at twitter they upload all branches. BT may at some point allow switching the UI to look at a diff branch but its not implemented yet.

- name: Fixup git history (for buildtracker)
run: bash $GITHUB_WORKSPACE/lighthouse-core/scripts/git-get-shared-history.sh
- name: Store in buildtracker
# TODO(paulirish): Don't allow this to fail the build. https://github.com/paularmstrong/build-tracker/issues/200
run: yarn bt-cli upload-build || true
Expand Down
75 changes: 75 additions & 0 deletions lighthouse-core/scripts/git-get-shared-history.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#!/usr/bin/env bash

##
# @license Copyright 2020 The Lighthouse Authors. All Rights Reserved.
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
##

set -euo pipefail
Copy link
Collaborator

Choose a reason for hiding this comment

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

wanna add x?


# Overview:
# - build-tracker relies on a common commit that's shared between HEAD and master.
# - Lighthouse runs on pull_request, not push, so the checkout is not the branch with shared history, but the result of a merge.
# - checkout@v2 uses a merge remote (eg. remotes/pull/9605/merge) that often has just a single commit.
# - This script creates a new branch that matches the current checkout, but does have a shared history.

# See also
# - https://github.com/paularmstrong/build-tracker/issues/106
# - https://github.com/paularmstrong/build-tracker/issues/200

# We can always use some more history
git -c protocol.version=2 fetch --deepen=100
echo "History is deepened."

if git merge-base HEAD origin/master > /dev/null; then
echo "We have a common commit w/ origin/master. Skipping this script…";
exit 0
else
echo "We don't have a common commit w/ origin/master. We'll checkout the associated branch, merge master, and then we'll be good"
fi

# get the human readable remote name
checkout_name=$(git describe --all --exact-match HEAD)


# We only want to keep going if it's a 'remotes/pull/{*}/merge'
if [[ $checkout_name != remotes/pull/*/merge ]]; then
echo "Don't know how to handle this checkout ($checkout_name). 🤔 Bailing.";
exit 0;
fi

# Extract 9605 from 'remotes/pull/9605/merge'
pr_num=${checkout_name//[!0-9]/}

lsremote_hash=$(git ls-remote origin "refs/pull/$pr_num/head" | cut -f1)

if [ -z "$lsremote_hash" ]; then
echo "ls-remote failed to find this PR's branch. 🤔 Bailing.";
exit 0;
fi

# Checkout our PR branch
git checkout --progress --force "$lsremote_hash"

# Branch off, for safekeeping
mergebranch_name="_bt_mergebranch-$pr_num"
git checkout -b "$mergebranch_name"


git merge --no-verify -m "Merge remote-tracking branch 'origin/master' into $mergebranch_name" origin/master

# If there's a diff aginst where we started.. we fucked up
if git --no-pager diff --color=always --exit-code "$checkout_name" > /dev/null; then
echo "No diff, good!"
else
echo "Unexpected difference between $mergebranch_name and $checkout_name. Bailing";
exit 0;
fi

# Lastly, now we should definitely have a merge-base.
if git merge-base HEAD origin/master > /dev/null; then
echo "Merge-base found. Perfect. 👌"
else
echo "No diff, but still no merge-base. Very unexpected. 🤔"
fi