Configuring GitHub actions #111

ftomassetti · 2023-12-06T16:16:01Z

The GitHub actions should now run for every commit and every PR.
We just perform some basic tasks, like compiling for the JVM and running tests on the JVM. Later one we could enable more tasks.

Fix #108

ftomassetti · 2023-12-06T16:17:04Z

@lppedd would this work for you?

lppedd · 2023-12-06T16:40:24Z

.gitignore

@@ -138,3 +138,4 @@ Desktop.ini
 *~
 .directory
 .merge_file*
+.idea


The idea of committing a subset of .idea was to streamline inspections and formatting, mostly. I generally tend to do this everywhere, so a dev can jump in and have the environment ready with no additional processes needed.

But if you feel like it doesn't suit the repo, feel free to remove those files.

I tend to have all the .idea configuration generated by gradle. I just thought these files were committed by mistake

I tend to have all the .idea configuration generated by Gradle

A part of those files are indeed fine as generated at project import time, but others like codeStyles/* or inspectionProfiles/* are not generated, and when opening the project you'll find the "Default" settings applied. This means everyone will add his own custom formatting.

For formatting, I think we can add ktlint and .editorconfig, which should help with consistency with different IDEs. Perhaps I can add an issue about that

I would avoid KTLint and stick with editorconfig then. I can generate the .editorconfig file in case.
KTLint configuration is a PITA and it doesn't have a reliable IDE plugin.

Great, let's go with .editorconfig then!
Should I consider this a follow up task and proceed with this merge or should we add this to this PR?

Not sure if I can add it, but you can generate it:

Restore the files you deleted

Open up the IntelliJ Settings (checking it says "Project" as scheme) and export as EditorConfig

Select the root of the repository as export target

Delete the .idea files again

ftomassetti · 2023-12-06T16:43:50Z

I changed this to have two separate actions: one for building and one for running tests

Each of them runs both on java 11 and java 17. Java 8 cannot be supported as ANTLR 4.13 requires at least Java 11

lppedd · 2023-12-06T16:45:35Z

Studying how Actions are set up right now.

@ftomassetti

as ANTLR 4.13 requires at least Java 11

Is this related to the generation process or also to the runtime?
If this impacts the Java runtime, we can also move up the generated bytecode version in our build scripts.

ftomassetti · 2023-12-06T16:54:23Z

Studying how Actions are set up right now.

@ftomassetti

as ANTLR 4.13 requires at least Java 11

Is this related to the generation process or also to the runtime?

For sure this affects the build process, so I need to run GitHub actions with Java 11. But it is true that we can still setup the build to compile for Java 8. However that would be unrelated from the GitHub actions configuration

lppedd · 2023-12-06T16:54:54Z

.github/workflows/build.yml

+        distribution: 'adopt'
+        java-version: ${{ matrix.java }}
+    - name: Test
+      run: ./gradlew ciBasicBuild


Looking at those two tasks (ciTest and ciBasicBuild) it seems we're just wasting CI time. Building is done two times, so I guess an unified task is enough, it will just say "Build" and we know it also contains tests.

Instead of creating custom tasks you can just invoke:

gradlew build

And it will take care of building and testing what it can actually build and test.
Under Ubuntu it's going to be JVM, JS, and linuxX64.

It is true that there is overlap between the two tasks. I originally add one, but following your comment in an issue I thought you preferred to have them split.

Normally I would use build or check, but at least on Mac they require to have Xcode installed to run also the native builds. I am not sure what kind of configuration it would require on GitHub actions, perhaps configuring GCC or other compilers, so I was thinking that we could get away with testing on the JVM for these frequently running actions. We could then have more complete actions later, to build on all platforms, and run those before a release or trigger them manually.

but following your comment in an issue I thought you preferred to have them split.

Yeah I had guessed you did it because of this, sorry! In Jenkins I'm used to publish custom "check points" back to my GitHub Enterprise instance, but I guess GitHub Actions are not that advanced yet.

so I was thinking that we could get away with testing on the JVM for these frequently running actions

I'd test both JVM and JS. JS is a risky platform and it should be always tested imo (I know this by experience).

I'd say you can use gradlew build -Ptarget.is.native=false.
target.is.native=false disables all the Kotlin Native targets, I had this one prepared for this exact occasion.

Thank you, I will run ./gradlew check -Ptarget.is.native=false.target.is.native=false

frett · 2023-12-06T17:19:12Z

.github/workflows/build.yml

+    - name: Test
+      run: ./gradlew ciBasicBuild


There is a gradle-build-action action that will automatically cache various gradle build files between subsequent actions.

Suggested change

- name: Test

run: ./gradlew ciBasicBuild

- name: Test

uses: gradle/gradle-build-action@v2

with:

arguments: ciBasicBuild

relevant action repo for reference: https://github.com/gradle/gradle-build-action

Interesting, but for now I will keep things simple and avoid caches. For now I think we can have a single job and step and just look at logs if something fails

frett · 2023-12-06T17:22:34Z

on my fork of the repo I had setup a GHA workflow for tests and deploying to our JFrog repo. feel free to grab anything you want from that action file, or ask questions about it.

https://github.com/CruGlobal/antlr-kotlin/blob/build/v0.0.7/.github/workflows/publish-fork.yml

lppedd · 2023-12-06T17:25:04Z

@frett looks pretty neat! I've got a question, what's up with the Cache Konan step?

frett · 2023-12-06T17:26:44Z

@frett looks pretty neat! I've got a question, what's up with the Cache Konan step?

Kotlin Multiplatform uses a Konan runtime to actually do the Kotlin/Native compilation. That binary was automatically downloaded and setup everytime a build would run. Unfortunately that isn't captured by the gradle-build-action caching, so I set it to manually cache it

frett · 2023-12-06T17:29:58Z

the publish action on my GHA workflow runs on macos to have access to XCode in order to get the ios targets.

running it on macos does also support jvm & js, but there is potential additional costs for a macos runner vs a normal ubuntu runner. We have a enterprise account so I'm not sure if there are any costs or restrictions in place for an open source project

lppedd · 2023-12-06T17:33:26Z

Oh I see, makes sense!

So (pardon my inexperience on Actions), splitting up the YAML file, we could have:

on:
  push:
    branches: ['master']
  pull_request:
    branches: ['*']

And:

jobs:
  build_and_test:
    name: Build & Test
    runs-on: ubuntu-latest
    steps:
      - name: Checkout code
        uses: actions/checkout@v3
      - name: Cache Konan
        uses: actions/cache@v3
        with:
          path: ~/.konan
          key: ${{ runner.os }}-konan-${{ github.sha }}
          restore-keys: ${{ runner.os }}-konan-
      - name: Run Build and Tests
        uses: gradle/gradle-build-action@v2
        with:
          arguments: build -Ptarget.is.native=false

Would this work?

frett · 2023-12-06T17:35:00Z

I would do:

on:
  push:
    branches: ['master']
  pull_request:
    branches: ['master']

the pull_request triggers are based on the branch you are merging into

lppedd · 2023-12-06T17:37:21Z

Thank you!
We also have a matrix now:

strategy:
  matrix:
    java: [ '11', '17' ]

I guess this would mean two JS builds too. Maybe running on Java 11 only is sufficient?

frett · 2023-12-06T17:41:23Z

it's probably sufficient, but the GHA workflow is only taking 2-3 mins to run currently, so running it on both toolchains vs 1 wouldn't have a huge impact. plus those 2 jobs would run concurrently.

lppedd · 2023-12-06T17:46:27Z

Sounds reasonable. So, looking at the overall picture, we are left with:

name: Build

on:
  push:
    branches: ['master']
  pull_request:
    branches: ['master']

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

jobs:
  build_and_test:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        java: ['11', '17']
    name: Build & Test (Java ${{ matrix.Java }})
    steps:
      - name: Setup Java
        uses: actions/setup-java@v3
        with:
          distribution: 'adopt'
          java-version: ${{ matrix.java }}
      - name: Checkout code
        uses: actions/checkout@v3
      - name: Cache Konan
        uses: actions/cache@v3
        with:
          path: ~/.konan
          key: ${{ runner.os }}-konan-${{ github.sha }}
          restore-keys: ${{ runner.os }}-konan-
      - name: Run Build and Tests
        uses: gradle/gradle-build-action@v2
        with:
          arguments: build -Ptarget.is.native=false

I'm starting to understand how all of this work finally.

frett · 2023-12-06T17:47:28Z

yeah, although you missed the setup-java step :)

lppedd · 2023-12-06T17:50:25Z

Damn, thanks! Added back in the comment.

frett · 2023-12-06T17:52:24Z

that looks good and should work as a starting point, the workflow can evolve over time as needs arise :)

lppedd · 2023-12-06T19:13:49Z

@frett so had a bit of docs reading and came up with:

Edit: updated with a working script

name: Build

on:
  push:
    branches: [ 'master' ]
  pull_request:
    branches: [ 'master' ]

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

jobs:
  build:
    strategy:
      matrix:
        include:
          - java: '11'
            target: JVM
            task: jvmTest
          - java: '17'
            target: JVM
            task: jvmTest
          - java: '17'
            target: JS
            task: jsTest
    runs-on: ubuntu-latest
    name: Build (${{ matrix.java }} / ${{ matrix.target }})
    steps:
      - name: Checkout code
        uses: actions/checkout@v3

      - name: Setup Java
        uses: actions/setup-java@v3
        with:
          distribution: 'adopt'
          java-version: ${{ matrix.java }}

      - name: Cache Konan
        uses: actions/cache@v3
        with:
          path: ~/.konan
          key: ${{ runner.os }}-konan-${{ github.sha }}
          restore-keys: ${{ runner.os }}-konan-

      - name: Setup Gradle
        uses: gradle/gradle-build-action@v2

      - name: Run Build and Tests (${{ matrix.target }})
        run: ./gradlew ${{ matrix.task }}
        working-directory: ${{ github.workspace }}
        continue-on-error: false

This allows you to execute three jobs:

- JDK 11 + JVM target
- JDK 17 + JVM target
- JDK 17 + JS target

frett · 2023-12-06T19:19:48Z

I'm not sure without giving it a try

lppedd · 2023-12-06T19:26:13Z

I've actually found out we can just do the following to define the matrix runs:

matrix:
  include:
    - java: '11'
      target: JVM
      task: jvmTest
    - java: '17'
      target: JVM
      task: jvmTest
    - java: '17'
      target: JS
      task: jsTest

Easier and probably more understandable.

ftomassetti · 2023-12-07T09:45:50Z

thank you @frett and @lppedd , I learned something very useful thanks to you. I just applied your suggestion as-is, and that works great. Let's get it merged

lppedd · 2023-12-07T09:50:47Z

Thanks! Another step towards easier maintenance is done 😎

ftomassetti added 6 commits December 6, 2023 13:25

updating badges

f01f42a

defining ciCheck task

6ad9ed4

add GitHub actions configuration

fc81707

remove idea configurations from committed files

df1f6ba

do not run CI on Java 8

9df27ab

change jobs names

034e319

ftomassetti marked this pull request as ready for review December 6, 2023 16:16

ftomassetti mentioned this pull request Dec 6, 2023

Correctly read bytes from a ReadableByteChannel #110

Merged

separate GitHub actions for build and test

a194c07

lppedd reviewed Dec 6, 2023

View reviewed changes

frett reviewed Dec 6, 2023

View reviewed changes

ftomassetti added 3 commits December 7, 2023 10:34

adding .editorconfig

a2a4da1

restore a single workflow

479d0a8

moving to approach supporting Native format (by frett and lppedd)

52f4761

ftomassetti merged commit 9ad8927 into master Dec 7, 2023
3 checks passed

ftomassetti deleted the githubActions branch December 7, 2023 09:48

sschuberth mentioned this pull request Jan 25, 2024

Add a linter #24

Open

Configuring GitHub actions #111

Configuring GitHub actions #111

Conversation

ftomassetti commented Dec 6, 2023 • edited Loading

ftomassetti commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lppedd Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftomassetti commented Dec 6, 2023

lppedd commented Dec 6, 2023 • edited Loading

ftomassetti commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frett Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frett commented Dec 6, 2023

lppedd commented Dec 6, 2023

frett commented Dec 6, 2023

frett commented Dec 6, 2023

lppedd commented Dec 6, 2023

frett commented Dec 6, 2023

lppedd commented Dec 6, 2023

frett commented Dec 6, 2023

lppedd commented Dec 6, 2023 • edited Loading

frett commented Dec 6, 2023

lppedd commented Dec 6, 2023

frett commented Dec 6, 2023

lppedd commented Dec 6, 2023 • edited Loading

frett commented Dec 6, 2023

lppedd commented Dec 6, 2023 • edited Loading

ftomassetti commented Dec 7, 2023

lppedd commented Dec 7, 2023

ftomassetti commented Dec 6, 2023 •

edited

Loading

lppedd Dec 6, 2023 •

edited

Loading

lppedd commented Dec 6, 2023 •

edited

Loading

frett Dec 6, 2023 •

edited

Loading

lppedd commented Dec 6, 2023 •

edited

Loading

lppedd commented Dec 6, 2023 •

edited

Loading

lppedd commented Dec 6, 2023 •

edited

Loading