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

Setup GitHub Actions #2512

Merged
merged 15 commits into from Mar 28, 2020
Merged

Setup GitHub Actions #2512

merged 15 commits into from Mar 28, 2020

Conversation

cortinico
Copy link
Member

This is a proposal to replace Travis + AppVeyor with Github Actions (as suggested in #2507). Having GH Actions will allow to run a matrix with [win, mac, linux] x [jdk8, jdk11, jdk13].

I've also removed Travis and AppVeyor configuration but we can keep it if we want to have them run alongside for some time.

I've adapted the .buildscript/deploy_snapshot.sh to work although I wasn't able to test it. I anyway think it's better to have a separate workflow that runs only on one agent + only on push to master.

Happy to get some input/feedbacks/ideas.

@codecov-io
Copy link

codecov-io commented Mar 26, 2020

Codecov Report

No coverage uploaded for pull request base (master@cec2d62). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2512   +/-   ##
=========================================
  Coverage          ?   80.85%           
  Complexity        ?     2266           
=========================================
  Files             ?      376           
  Lines             ?     7348           
  Branches          ?     1202           
=========================================
  Hits              ?     5941           
  Misses            ?      809           
  Partials          ?      598           
Impacted Files Coverage Δ Complexity Δ
...rturbosch/detekt/rules/empty/EmptyFunctionBlock.kt 93.33% <0.00%> (ø) 12.00% <0.00%> (?%)
...bosch/detekt/rules/complexity/LabeledExpression.kt 92.00% <0.00%> (ø) 14.00% <0.00%> (?%)
...ab/arturbosch/detekt/extensions/DetektExtension.kt 26.31% <0.00%> (ø) 2.00% <0.00%> (?%)
...n/io/gitlab/arturbosch/detekt/core/DetektFacade.kt 47.61% <0.00%> (ø) 3.00% <0.00%> (?%)
...lin/io/gitlab/arturbosch/detekt/api/ConfigAware.kt 60.00% <0.00%> (ø) 0.00% <0.00%> (?%)
.../detekt/generator/collection/MultiRuleCollector.kt 73.43% <0.00%> (ø) 4.00% <0.00%> (?%)
...ab/arturbosch/detekt/rules/empty/EmptyElseBlock.kt 100.00% <0.00%> (ø) 3.00% <0.00%> (?%)
...ch/detekt/rules/coroutines/GlobalCoroutineUsage.kt 90.00% <0.00%> (ø) 3.00% <0.00%> (?%)
...b/arturbosch/detekt/rules/style/FileParsingRule.kt 100.00% <0.00%> (ø) 2.00% <0.00%> (?%)
...ain/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt 89.47% <0.00%> (ø) 13.00% <0.00%> (?%)
... and 366 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cec2d62...dfdea40. Read the comment docs.

.buildscript/deploy_snapshot.sh Outdated Show resolved Hide resolved
Co-Authored-By: Artur Bosch <arturbosch@gmx.de>
.buildscript/deploy_snapshot.sh Outdated Show resolved Hide resolved
.github/workflows/pre-merge.yaml Outdated Show resolved Hide resolved
Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

Super cool, thanks for the work @cortinico !

.github/workflows/pre-merge.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

Don't we need these lines?

  - rm -f  $HOME/.gradle/caches/modules-2/modules-2.lock
  - rm -fr $HOME/.gradle/caches/*/plugin-resolution/

.github/workflows/pre-merge.yaml Show resolved Hide resolved
.github/workflows/pre-merge.yaml Outdated Show resolved Hide resolved
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks! I very much welcome this change, since I invested quite some time to re-trigger the flaky builds on TravisCI. Good job @cortinico !
But we have to be careful here, before we remove the TravisCI and AppVeyor scripts. The current GitHub action script misses codecov.io code coverage reports.
I think it can be added by using a corresponding GitHub action.

@arturbosch
Copy link
Member

arturbosch commented Mar 26, 2020

Thanks! I very much welcome this change, since I invested quite some time to re-trigger the flaky builds on TravisCI. Good job @cortinico !
But we have to be careful here, before we remove the TravisCI and AppVeyor scripts. The current GitHub action script misses codecov.io code coverage reports.
I think it can be added by using a corresponding GitHub action.

I think we have it. It's the last action?

 - name: Publish Coverage
      uses: codecov/codecov-action@v1

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

@arturbosch Oh sry, I oversaw this command. I take everything back.

@cortinico
Copy link
Member Author

cortinico commented Mar 27, 2020

So far I've addressed all the comments. There are still two open points to address: caching and snapshots.

Don't we need these lines?

  • rm -f $HOME/.gradle/caches/modules-2/modules-2.lock
  • rm -fr $HOME/.gradle/caches/*/plugin-resolution/

I was playing around with those in a06c017. Those are Travis specific configuration commands. Looks like caching is working fine on the Ubuntu worker (build time ~6 minutes). That's working also without those two commands.
MacOS and Win worker are instead failing to cache properly. I'm still investigating what's going on.

As for the Snapshot setup, is currently hard to test as it's configured to publish a snapshot on push to the master branch. This mirrors the former Travis setup. I'm currently concerned as the current setup will run the deploy_snapshot.sh script 3 times (JDK8 for win, mac, linux). Looks like this was the case also before on Travis, right? (JDK8 for mac and linux).

Should I limit this to run only on JDK8/Linux? Or should it be a separate action?

@arturbosch
Copy link
Member

As for the Snapshot setup, is currently hard to test as it's configured to publish a snapshot on push to the master branch. This mirrors the former Travis setup. I'm currently concerned as the current setup will run the deploy_snapshot.sh script 3 times (JDK8 for win, mac, linux). Looks like this was the case also before on Travis, right? (JDK8 for mac and linux).

Should I limit this to run only on JDK8/Linux? Or should it be a separate action?

Lets go with just JDK8/Linux.
A separate action which just runs on the actual merge to master would be cool too.

@schalkms
Copy link
Member

Do we know why the Publish Coverage task lasts so long on MacOS?

@cortinico
Copy link
Member Author

The cache miss issue should be solved now. Funny story, @3flex had the same issue we were facing here: actions/cache#144

I've moved the logic of the publish script to its own Action.

Do we know why the Publish Coverage task lasts so long on MacOS?

I noticed that the Publish Coverage goes sometimes in timeout. Apparently the external GH Action is doing some retry. I could eventually switch to the good old bash <(curl -s https://codecov.io/bash) approach.

@schalkms
Copy link
Member

schalkms commented Mar 27, 2020

I could eventually switch to the good old bash <(curl -s https://codecov.io/bash) approach.

That would be my preferred approach until the referenced issue is fixed. Thanks again for the awesome research work in this PR! @cortinico

@schalkms
Copy link
Member

AppVeyor is still triggered even though there's no config anymore. Therefore, the failed build on this platform can be ignored.

.github/workflows/pre-merge.yaml Outdated Show resolved Hide resolved
Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
@cortinico
Copy link
Member Author

We should be good to go.
Although I noticed that the cache on the macos workers is significantly bigger (2+ Gb vs an average of 400Mb for win & linux). I don't exactly know why.

I don't think this is a blocker as for now as the overall time is anyway lower than it was before with AppVeyor/Travis so I'd love to followup on a later PR on that.

@arturbosch arturbosch merged commit 4f76887 into detekt:master Mar 28, 2020
@arturbosch arturbosch added this to the 1.7.2 milestone Mar 28, 2020
@cortinico cortinico deleted the setup-gh-actions branch March 28, 2020 14:34
@3flex
Copy link
Collaborator

3flex commented Mar 29, 2020

Funny story, @3flex had the same issue we were facing here: actions/cache#144

Because I was trying to introduce Actions for this project myself :)

Glad you worked it out, thanks! And I saw the same regarding cache size, and agree it shouldn't be a blocker - if the cache grows too large it will be pruned automatically, though we'd lose some caching benefits. Overall though I see this as a win especially as Windows builds run in parallel which wasn't possible with AppVeyor.

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.

None yet

6 participants