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

Experiment with GitHub Actions #940

Closed
wants to merge 34 commits into from
Closed

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Sep 23, 2019

GitHub Actions (https://github.com/features/actions) offer a promising alternative to Travis CI. This PR sets up automatic CI releases for this build via GitHub Actions so that we can disable the Travis, Appveyor and Azure CIs.

@olafurpg olafurpg force-pushed the actions branch 5 times, most recently from 3ee0602 to fed03fd Compare September 23, 2019 13:26
@olafurpg
Copy link
Member Author

It looks like it's not yet possible to re-run individual job failures that are caused by flakyness https://gh.neting.ccmunity/t5/GitHub-Actions/quot-This-check-suite-has-disabled-re-running-individual-check/m-p/31696

@olafurpg
Copy link
Member Author

@tgodzik do these error ring a bell?

INFO  running '/tmp/metals1573090658375376626/mvnw ch.epfl.scala:maven-bloop_2.10:1.3.2:bloopInstall -DdownloadSources=true'
ERROR /tmp/metals1573090658375376626/mvnw: 186: /tmp/metals1573090658375376626/mvnw: cannot open ./.mvn/wrapper/maven-wrapper.properties: No such file
ERROR ./.mvn/wrapper/maven-wrapper.jar: No such file or directory
ERROR Error: Could not find or load main class org.apache.maven.wrapper.MavenWrapperMain

From https://github.com/scalameta/metals/pull/940/checks?check_run_id=233844637

@olafurpg
Copy link
Member Author

Looks like we should try to split the unit/test job into multiple jobs. It's currently the bottleneck on the Ubuntu job and it's also hitting on OOM errors in Windows.

strategy:
matrix:
scala:
- "2.13.0"
Copy link
Member

@gabro gabro Sep 24, 2019

Choose a reason for hiding this comment

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

Also, 2.13.1

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'll need to add a sbt command to auto-generate this matrix to ensure it's in sync with the build. I don't expect to merge this PR for now because GitHub Actions don't support re-running individual checks yet. I'm just trying to get a feel for how much we can push it 😄

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 will also need to ensure that the unit/test cross-matrix covers all the packages under tests

Copy link
Member

Choose a reason for hiding this comment

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

@@ -191,7 +191,7 @@ object PackageIndex {
Classpath(v).entries
}.toList
entry <- entries
if entry.isFile
if entry.isFile && !entry.toNIO.endsWith("jfr.jar")
Copy link
Member

Choose a reason for hiding this comment

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

what's jfr.jar? Maybe this is worth a 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.

Good point, added (but not pushed yet).

@olafurpg olafurpg force-pushed the actions branch 3 times, most recently from d6896ef to 0ec502f Compare September 24, 2019 15:19
"Mtags" -> githubActionsJob(
"Run tests",
"csbt ++${{ matrix.scala }} cross/test",
githubActionsMatrix("scala", V.supportedScalaVersions)
Copy link
Member Author

Choose a reason for hiding this comment

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

@milessabin One option for syncing the CI with crossScalaVersions is to automatically generate .travis.yml from build.sbt. In this PR we're using GitHub Actions but the same approach should work for Travis CI.

Choose a reason for hiding this comment

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

@olafurpg you must be pulling my leg ... @djspiewak told me this is "fundamentally impossible"! ;-)

But seriously ... thanks for the pointer :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't even seem necessary to print YAML, you can print JSON as long as the filename stays travis.yml

Copy link

@djspiewak djspiewak Sep 24, 2019

Choose a reason for hiding this comment

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

In this PR we're using GitHub Actions but the same approach should work for Travis CI.

It doesn't work with travisci unless you commit the results. If Github Actions has multi-stage builds without a need to hard-commit results and allowing configuration shifts between stages, then it would be capable of this, but Travis' architecture doesn't support that.

Choose a reason for hiding this comment

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

It doesn't work with travisci unless you commit the results.

When did I ever say I didn't want to commit the results!?!

- incorrect resource paths
- cleaner build matrix
- support multiple steps in the same job
@olafurpg
Copy link
Member Author

I'm gonna close this PR for now as an interesting experiment.

  • I think it's a good idea to generate the build matrix from build.sbt.
  • I think it's a good idea to split the codebase into smaller packages so that we can more easily run tests in parallel
  • I think it's best to wait until we switch the CI to GitHub Actions, primarily the feature that is missing IMO is the ability to re-run an individual job that failed for some reason (flaky download, etc). Lack of caching doesn't seem to be a blocker.

There are too many unrelated changes in this PR mixed together for me to feel comfortable merging it. I think it's best that we consider switching by incrementally enabling parts of the build.

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

Successfully merging this pull request may close these issues.

4 participants