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

CI with GitHub Actions #11142

Merged
merged 44 commits into from Apr 24, 2022
Merged

CI with GitHub Actions #11142

merged 44 commits into from Apr 24, 2022

Conversation

ihostage
Copy link
Member

@ihostage ihostage commented Feb 2, 2022

Notes by @mkurz:
Using coursier/cache-action without arguments will build a hash from the content of these files and uses that hash as part of the key to store the values (paths/files).
That means to "invalidate" the cache the build.sbt, project/[*.scala|*.sbt], etc. has to change.

When extracing the cache, existing files in a path will be kept, except for files which also exist in the stored cache, then it will be overwritten. In the background a tar command is used: tar -xf <archive.tar> -P -C <dir>

Right now, the cache content is not updated. That means e.g. once the .cache/coursier folder is cached, it will never be updated anymore if there was a cache hit (with the primary key): actions/cache#342
Also see here: https://github.com/actions/cache/blob/8f1e2e02865c42348f9baddbbaafb1841dce610a/src/save.ts#L38 - when the primary key is hit, the cache will never be updated.

The cache-hit output will only be set to true, if there was an exact match: https://github.com/actions/cache/blob/8f1e2e02865c42348f9baddbbaafb1841dce610a/src/restore.ts#L52 (there could also be a partial match, but then still the output will be false)
BTW: Here is the underlying restoreCache method used: https://github.com/actions/toolkit/blob/91f9153ca87feb260480640429822005380ea9de/packages/cache/src/cache.ts#L65 (the saveCache is below it as well)

coursier/cache-action and coursier/setup-action play well together to cache a java installation. That's because cs uses ~/.cache/coursier/, e.g. ~/.cache/coursier/https/github.com/adoptium/temurin11-binaries/releases/download/jdk-11.0.14.1%252B1/OpenJDK11U-jdk_x64_linux_hotspot_11.0.14.1_1.tar.gz/jdk-11.0.14.1+1, see https://github.com/playframework/playframework/runs/6110556466?check_suite_focus=true#step:6:49
Problem: When using different JVM, only one will cached by default when the first jobs runs, because like written above the cache is not going to be updated. Fixed in the prefetch-for-caching job.

Stupid workaround to clear the cache: actions/cache#2 (comment) (Can be combined with inputs: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch). Added the workaround for the Play repo here: #11247

A travis config we used to test different sbt and scala versions, including running some of the jobs in a cron job:
https://github.com/playframework/playframework/blob/f18c239c9186b5b65af291817cd75918f5d39ff9/.travis.yml

@mkurz
Copy link
Member

mkurz commented Apr 24, 2022

I am very confident we are finally good to go here. If @ihostage or someone else wants to review, I recommend to do that commit-after-commit, since that should explain what I did (including the commit messages).

The tests are all looking good. I also temporary simulated a cron job run by switched the conditions to check for pull_request instead of schedule and everything looked well too.

It also turns out that we do not need the script folder anymore.

There are probably things that could still be improved, however lets move on, we can improve along the way.

@mkurz mkurz marked this pull request as ready for review April 24, 2022 18:28
Copy link
Member

@mkurz mkurz left a comment

Choose a reason for hiding this comment

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

Finally.

@mkurz mkurz merged commit 877ddc5 into playframework:main Apr 24, 2022
prefetch-for-caching:
name: Prefetch dependencies and JVMs for caching
uses: playframework/.github/.github/workflows/sbt.yml@v1
# if: steps.coursier-cache.outputs.cache-hit-coursier != 'true' # Waiting for https://github.com/coursier/cache-action/pull/296
Copy link
Member

@mkurz mkurz Apr 24, 2022

Choose a reason for hiding this comment

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

Actually this condition here will be very important to speed up the build. Right now this prefetch job will always run all sbt commands below, however once that feature is available that job will be run only one time for each unique file hash of the build.sbt and project/[*.sbt|*.scala]
That means as long as that sbt files do not change, we will always reuse the same cache which is quite nice.

@mkurz
Copy link
Member

mkurz commented Apr 24, 2022

FYI: It seems the workaround removed in this commit 5f38e1d will not be necessary for when using GitHub actions 👍 It seems GHA is smarter than travis was and will always check out the same commit, even when a pull request gets merged e.g. between the first job and the last job (for example after the publish local job finished and before a scripted test jobs starts. In Travis CI this caused problems because we used dynver and now the commit sha changed between the two jobs, the dynver play version changed as well, so the published artifacts could not be used in the scripted tests).
I just tested this with #11231, where I cancelled the scripted test jobs, merged a pull request, and then again started the jobs. The jobs worked correctly and also the "Checkout" job still used the same commit sha like the publish local one. Nice.

@mkurz
Copy link
Member

mkurz commented Apr 25, 2022

The first nightly failed: https://github.com/playframework/playframework/actions/runs/2217931578
I will investigate...

@ihostage ihostage deleted the github-actions branch April 25, 2022 11:38
@ihostage
Copy link
Member Author

It also turns out that we do not need the script folder anymore.

Not quite 😄 local-pr-validation.sh is used by contributors and is mentioned in CONTRIBUTING.md 😉
I think we can create the same simple script without using scripts/scriptLib.

@mkurz
Copy link
Member

mkurz commented Apr 25, 2022

local-pr-validation.sh is now replace by simply running sbt validateCode, I think we should just update the docs and tell people to make use of that sbt task instead of adding scripts for that. (Mentioned that in commit message of 5af610b)

@mkurz
Copy link
Member

mkurz commented Apr 25, 2022

@ihostage see #11250. Thanks for reviewing 👍

@PromanSEW
Copy link
Contributor

This PR was not correctly squashed and "littered" main branch a lot :-/

@mkurz
Copy link
Member

mkurz commented May 4, 2022

🤔 Hmm, that depends, some people like to squash and rebase, some people like to merge and keep the whole history. It's a matter of taste and what someone prefers.

@PromanSEW
Copy link
Contributor

Other PRs were always squashed and merged as 1 commit with PR link, weren't are?

@mkurz
Copy link
Member

mkurz commented May 4, 2022

That was a long time ago, since at least 2 or I think even 3 years we do not squash anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants