-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add kotlin problem matchers #1775
Conversation
7ad5bff
to
2dfb9a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome speed up. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor 👍 minor comments
"severity": "error", | ||
"pattern": [ | ||
{ | ||
"regexp": "(.+\\.kt):(\\d+):(\\d+):\\s(.*)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add kts
Gradle scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is required. We don't run detekt against our build scripts.
"severity": "error", | ||
"pattern": [ | ||
{ | ||
"regexp": "^e\\:\\s(?:(?:(.*):(?:\\s\\()?(\\d+)(?:(?:\\,\\s)|\\:)(\\d+))(?:\\))?\\:\\s)?(.*)$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you come up with all these regexp? or are they already provided somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with them. Because we use two kotlin versions in our project (buildSrc, and regular project) we have two kinds of logs for kotlin compilation errors.
if: always() && !cancelled() && steps.jvm-cache.outputs.cache-hit == 'true' | ||
with: | ||
name: packages-jvm-${{ steps.find-library-version.outputs.label }} | ||
name: packages-jvm-${{ needs.checksum.outputs.version-label }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the name be packages-m2-jvm-sync
? it should match the usage in pr.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, packages-m2-jvm-sync
is to define a cache, whereas packages-jvm-
is to define an artifact.
- name: Check JNI Linux lib cache | ||
id: jni-linux-lib-cache | ||
uses: cmelchior/cache@main | ||
uses: actions/cache@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we need anything from CM fork? or v4 already has everything we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fork had a CACHE_SKIP_SAVE
env parameter to skip saving the cache, I guess that it was an optimization, but after splitting the check cache in multiple jobs is no longer required.
path: ./packages/cinterop/build/realmLinuxBuild | ||
path: ./packages/cinterop/build/realmWindowsBuild | ||
key: jni-windows-lib-${{ needs.checksum.outputs.packages-sha }} | ||
enableCrossOsArchive: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is unnecessary for Linux (enableCrossOsArchive
) since we build and retrieve on Ubuntu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so.
with: | ||
cmake-version: ${{ vars.VERSION_CMAKE }} | ||
|
||
- name: Setup ninja | ||
uses: cmelchior/setup-ninja@master | ||
uses: clementetb/setup-ninja@master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still based on https://github.com/ashutoshvarma/setup-ninja ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is based in cmelchior/setup-ninja that it is based on ashutoshvarma's
checkstyle-github-action
with problem matchers.