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

Switch from Travis to GHA #1073

Merged
merged 3 commits into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
77 changes: 77 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
name: CI

on:
push:
branches:
- master
pull_request:
schedule:
# Run at 18:00 UTC every day
- cron: '0 18 * * *'
Comment on lines +8 to +10
Copy link
Member

Choose a reason for hiding this comment

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

Is this the link-check cronjob?

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand, all the steps in the file are executed when master or a PR is pushed to, and at a regular interval (cron job). Is that correct?

Copy link
Member

Choose a reason for hiding this comment

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

I think you're partially correct, but there are several instances of #1073 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this the link-check cronjob?

Yeah, it's the same as what we have on travis.

image

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to not have this in a separate yml file inside of workflows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason to not have this in a separate yml file inside of workflows?

I don't think it's worth having duplicate workflows with multiple files.


jobs:
ci:
runs-on: ubuntu-latest
env:
MDBOOK_VERSION: 0.4.7
MDBOOK_LINKCHECK_VERSION: 0.7.2
MDBOOK_TOC_VERSION: 0.6.1
DEPLOY_DIR: book/html
BASE_SHA: ${{ github.event.pull_request.base.sha }}
steps:
- uses: actions/checkout@v2
with:
# linkcheck needs the base commit.
fetch-depth: 0

- name: Cache binaries
id: mdbook-cache
uses: actions/cache@v2
with:
path: |
~/.cargo/bin
key: ${{ runner.os }}-${{ env.MDBOOK_VERSION }}--${{ env.MDBOOK_LINKCHECK_VERSION }}--${{ env.MDBOOK_TOC_VERSION }}

- name: Cache linkcheck
uses: actions/cache@v2
with:
path: |
~/book/linkcheck
key: ${{ runner.os }}-${{ hashFiles('./book/linkcheck') }}

- name: Check line lengths
if: github.event_name != 'push'
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't this run on push?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we should block the deployment by the line length failure. It will be caught by the cron job anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Will the line-length check run for PRs? I was confused about whether push was just for pushes to master, or if it included PR pushes as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will the line-length check run for PRs? I was confused about whether push was just for pushes to master, or if it included PR pushes as well.

push here means just for master as we restrict it here: https://github.com/JohnTitor/rustc-dev-guide/blob/4c70da4512e1019a4deb0de9160791965b73a6a8/.github/workflows/ci.yml#L4-L6

For instance, I pushed this branch but the workflow wasn't triggered.
https://github.com/JohnTitor/rustc-dev-guide/tree/doesnt-trigger-event

run: |
shopt -s globstar
MAX_LINE_LENGTH=100 bash ci/check_line_lengths.sh src/**/*.md

- name: Install latest nightly Rust toolchain
if: steps.mdbook-cache.outputs.cache-hit != 'true'
uses: actions-rs/toolchain@v1
with:
toolchain: nightly
override: true

- name: Install Dependencies
if: steps.mdbook-cache.outputs.cache-hit != 'true'
run: |
cargo install mdbook --version ${{ env.MDBOOK_VERSION }}
cargo install mdbook-linkcheck --version ${{ env.MDBOOK_LINKCHECK_VERSION }}
cargo install mdbook-toc --version ${{ env.MDBOOK_TOC_VERSION }}
Comment on lines +58 to +60
Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't come up with a good way to use --version ^x.y.z while using actions/cache.

Copy link
Member

Choose a reason for hiding this comment

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

This seems fine to me.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that means we'll have to manually update these tools more often?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth it to introduce some automation to update those dependencies?

Copy link
Member

@camelid camelid Mar 2, 2021

Choose a reason for hiding this comment

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

Well, I guess we might be able to get dependabot setup, but I'm not sure if it's worth doing it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK dependabot only updates Cargo.toml or Cargo.lock so it doesn't help us. But an automation job or something else would be great as follow-up work.

I guess that means we'll have to manually update these tools more often?

Yes, we can still use --version ^x.y.z specifying but it doesn't update the mdbook and their tools version due to the cache key.


- name: Check build
run: mdbook build

- name: Deploy to gh-pages
if: github.event_name == 'push' && github.repository == 'rust-lang/rustc-dev-guide'
LeSeulArtichaut marked this conversation as resolved.
Show resolved Hide resolved
run: |
touch "${{ env.DEPLOY_DIR }}/.nojekyll"
cp CNAME "${{ env.DEPLOY_DIR }}"
cd "${{ env.DEPLOY_DIR }}"
rm -rf .git
git init
git config user.name "Deploy from CI"
git config user.email ""
git add .
git commit -m "Deploy ${GITHUB_SHA} to gh-pages"
git push --quiet -f "https://x-token:${{ secrets.GITHUB_TOKEN }}@github.com/${GITHUB_REPOSITORY}" HEAD:gh-pages
39 changes: 0 additions & 39 deletions .travis.yml

This file was deleted.

12 changes: 6 additions & 6 deletions ci/linkcheck.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,22 @@
set -e
set -o pipefail

# https://docs.travis-ci.com/user/environment-variables/#default-environment-variables
if [ "$TRAVIS_EVENT_TYPE" = "cron" ] ; then # running in cron job
# https://docs.github.com/en/actions/reference/environment-variables
if [ "$GITHUB_EVENT_NAME" = "schedule" ] ; then # running in scheduled job
FLAGS=""

echo "Doing full link check."
set -x
elif [ "$CI" = "true" ] ; then # running in PR CI build
if [ -z "$TRAVIS_COMMIT_RANGE" ]; then
echo "error: unexpected state: TRAVIS_COMMIT_RANGE must be non-empty in CI"
if [ -z "$BASE_SHA" ]; then
echo "error: unexpected state: BASE_SHA must be non-empty in CI"
exit 1
fi

CHANGED_FILES=$(git diff --name-only $TRAVIS_COMMIT_RANGE | tr '\n' ' ')
CHANGED_FILES=$(git diff --name-only $BASE_SHA... | tr '\n' ' ')
FLAGS="--no-cache -f $CHANGED_FILES"

echo "Checking files changed in $TRAVIS_COMMIT_RANGE: $CHANGED_FILES"
echo "Checking files changed since $BASE_SHA: $CHANGED_FILES"
set -x
else # running locally
COMMIT_RANGE=master...
Expand Down