Add GitHub actions #838

dantti · 2023-09-22T19:58:45Z

No description provided.

winterz · 2023-09-22T20:11:22Z

@dantti can you somehow fold this together with #768

dantti · 2023-10-01T21:19:24Z

@winterz so PR #768 did not enable tests, and had a disabled AppImage step, that uses the old linuxdeploy (not the one we currently use for Qt6 stuff), also I don't think GitHub actions should create Gammaray binaries because gammaray is quite tied to the same Qt version the app is running.

A full run takes quite some time so that perhaps we would only want github actions to build packages for tagged versions.

This PR also enables all tests that passes, I added a comment on each configuration that I skip tests, as they depend on Qt version and OS.

IMO it's good to get this merged now, and we can fix tests as we manage to, will we retire buildbot for this one?

ferdnyc

A few notes.

Regarding #768, the only reason I included things like the AppImage build (disabled, as it can't work, for reasons I explained in the comments/PR) was because I figured resurrecting the old Travis CI script was as good a starting point as any.

I'd have happily removed that, or made any other changes if requested. But the PR's been there since March without any indication that it'd even been noticed. A comment on this PR is the very first time it's ever been acknowledged or mentioned.

ferdnyc · 2023-10-02T23:39:19Z

.github/workflows/build.yml

+        qt:
+          - version: "5.15.2"
+            requested: "5.15"
+          - version: "6.5.3"
+            requested: "6.5.*"
+            modules: qtshadertools


I would caution against those version keys -- if there's a 6.5.4 release, who's going to file the PR to update the version number?

(It might be more useful to just store "5" and "6" for the ${{ matrix.qt.version }}, then you can avoid all of those startsWith() calls later on. But there's no reason the exact version is needed. Especially when it could be wrong, as install-qt-action might actually install a different version.)

If you really want the exact version to display, it's probably easiest to run qtpaths --qt-version in a step after installing it, and store the output. e.g.

steps: - name: "Get Qt version" id: qt_version run: | echo "QT_VERSION=$(qtpaths --qt-version)" >> "$GITHUB_OUTPUT" [...later...] - name: "Test on Qt ${{ steps.qt_version.outputs.QT_VERSION }}"

Or, if you only need it in the scripting it can be done as an envvar:

steps: - name: "Set Qt version" run: | echo "QT_VERSION=$(qtpaths --qt-version)" >> "$GITHUB_ENV" [...later...] - name: "Run tests" run: | echo "Testing on Qt $QT_VERSION"

(Actually it can be done as an envvar either way, thanks to the ${{ env.QT_VERSION }} context that would let you reference it in the YAML metadata as well.)

Come to think of it, I don't think that will quite work for Windows PowerShell, but this will:

- run: > ${{ runner.os == 'windows' && '"QT_VERSION=$(qtpaths --qt-version)" >> $env:GITHUB_ENV' }} || 'echo "QT_VERSION=$(qtpaths --qt-version)" >> "$GITHUB_ENV"' }}

AppImage can be created just fine by using qt plugin of Linux deploy, still it's too slow for regular builds.

Qt version is perfectly fine this way, as we might want to have more versions to test regressions or behavior changes. The version text is just text, what is used is the requested one, this is from the Qt git hub actions page, which I might change in future to look a bit more like KDDockWidget, but there I create a much more complex setup so for now this is enough.

ferdnyc · 2023-10-02T23:40:32Z

.github/workflows/build.yml

+  build:
+    runs-on: ${{ matrix.os }}
+    strategy:
+      fail-fast: false #true


The #true comment at the end there is a bit confusing.

Was left there to remember to return to true, and it worked ;)

ferdnyc · 2023-10-02T23:52:02Z

.github/workflows/build.yml

+        run: >
+          cmake -S . -B ./build -G Ninja
+          -DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
+          -DGAMMARAY_CLIENT_ONLY_BUILD=${{ matrix.client_only.client_only }}


...Huh, does that actually work? Kind of cool, I didn't realize you could test members like that, instead of an explicit boolean test like ${{ matrix.client_only == 'client_only' }}.

Oh — actually, this doesn't work. All of the configured jobs have -DGAMMARAY_CLIENT_ONLY_BUILD= on the CMake command line, which is why your tests didn't fail due to the missing probes that it would otherwise have caused.

(Also, Windows client only builds are currently broken due to some more bad target-creation logic I didn't catch (because it doesn't affect Linux) in plugins/quickinspector/CMakeLists.txt. I'll have a PR submitted in a couple of minutes to fix that.)

right, I initially had true and false but was not readable on the github matrix, do forgot to update it

ferdnyc · 2023-10-03T04:26:01Z

.github/workflows/build.yml

+        # Exclude
+        # quicktexturetest
+        # bindinginspectortest
+      - name: Qt5 Run tests on Windows
+        if: ${{ matrix.build_type == 'Debug' && runner.os == 'Windows' && startsWith(matrix.qt.version, '5.') }}
+        run: >
+          ctest --test-dir ./build -C ${{ matrix.build_type }} --output-on-failure
+          --exclude-regex "quicktexturetest|bindinginspectortest"
+
+        # Exclude
+        # quicktexturetest
+        # bindinginspectortest
+        # modelinspectortest fails in Qt6/CI passes locally
+      - name: Qt6 Run tests on Windows
+        if: ${{ matrix.build_type == 'Debug' && runner.os == 'Windows' && startsWith(matrix.qt.version, '6.') }}
+        run: >
+          ctest --test-dir ./build -C ${{ matrix.build_type }} --output-on-failure
+          --exclude-regex "quicktexturetest|launchertest|bindinginspectortest|modelinspectortest"
+


This will work, no question. But except for the exclusion list, the test instances are basically identical. (99%, anyway — just the Linux envvars.)

IMHO it's cleaner to just use one definition, and break out the selection of exclusion list into a previous step. I got it to work like this, which is a little clunky but pretty readable...

jobs: build: # [...] env: Windows_5: 'quicktexturetest|bindinginspectortest' Windows_6: 'quicktexturetest|launchertest|bindinginspectortest|modelinspectortest' macOS_5: 'connectiontest-*|probeabidetectortest|launchertest|quickinspectortest2' macOS_6: 'connectiontest-*|probeabidetectortest|launchertest|clientconnectiontest|modelinspectortest|quickinspectortest2' Linux_5: 'quickmaterialtest|quicktexturetest|connectiontest-style-filter' Linux_6: 'quickmaterialtest|quicktexturetest|connectiontest-style-filter|bindinginspectortest|quickinspectortest|quickinspectortest2|modelinspectortest' QT_QPA_PLATFORM: ${{ contains('ubuntu', matrix.os) && 'offscreen' || '' }} QT_QUICK_BACKEND: ${{ contains('ubuntu', matrix.os) && 'software' || '' }} steps: - name: "Select exclusions" env: CTEST_EXCLUSIONS: > ${{ (runner.os == 'Windows' && matrix.qt.version == 5) && env.Windows_5 || '' }} ${{ (runner.os == 'Windows' && matrix.qt.version == 6) && env.Windows_6 || '' }} ${{ (runner.os == 'Linux' && matrix.qt.version == 5) && env.Linux_5 || '' }} ${{ (runner.os == 'Linux' && matrix.qt.version == 6) && env.Linux_6 || '' }} ${{ (runner.os == 'macOS' && matrix.qt.version == 5) && env.macOS_5 || '' }} ${{ (runner.os == 'macOS' && matrix.qt.version == 6) && env.macOS_6 || '' }} run: > ${{ runner.os == 'windows' && '"CTEST_EXCLUSIONS=$env:CTEST_EXCLUSIONS" >> $env:GITHUB_ENV' || 'echo "CTEST_EXCLUSIONS=$CTEST_EXCLUSIONS" >> "$GITHUB_ENV"' }}

It defines all of the right envvars for each system, so they can be dropped in later (Including --exclude-regex "${{ env.CTEST_EXCLUSIONS }}" for the CTest invocation.)

Though now I'm also seeing that I have to restrict the tests to non-client-only builds. A bunch of them complain if they're missing a probe for the platform.

(The above also relies on a matrix.qt setup of the form I suggested earlier:)

matrix: qt: - version: 5 request: 5.15.* - version: 6 request: 6.4.* modules: qtshadertools

(If you don't want to pollute the process environment, the exclusions could also be baked directly into the selection step, e.g.:)

# [...] env: QT_QPA_PLATFORM: ${{ contains('ubuntu', matrix.os) && 'offscreen' || '' }} QT_QUICK_BACKEND: ${{ contains('ubuntu', matrix.os) && 'software' || '' }} steps: - name: "Select exclusions" env: CTEST_EXCLUSIONS: > ${{ (runner.os == 'Windows' && matrix.qt.version == 5) && 'quicktexturetest|bindinginspectortest' || '' }} ${{ (runner.os == 'Windows' && matrix.qt.version == 6) && 'quicktexturetest|launchertest|bindinginspectortest|modelinspectortest' || '' }} ${{ (runner.os == 'Linux' && matrix.qt.version == 5) && 'quickmaterialtest|quicktexturetest|connectiontest-style-filter' || '' }} ${{ (runner.os == 'Linux' && matrix.qt.version == 6) && 'quickmaterialtest|quicktexturetest|connectiontest-style-filter|bindinginspectortest|quickinspectortest|quickinspectortest2|modelinspectortest' || '' }} ${{ (runner.os == 'macOS' && matrix.qt.version == 5) && 'connectiontest-*|probeabidetectortest|launchertest|quickinspectortest2' || '' }} ${{ (runner.os == 'macOS' && matrix.qt.version == 6) && 'connectiontest-*|probeabidetectortest|launchertest|clientconnectiontest|modelinspectortest|quickinspectortest2' || '' }} run: > ${{ runner.os == 'windows' && '"CTEST_EXCLUSIONS=$env:CTEST_EXCLUSIONS" >> $env:GITHUB_ENV' || 'echo "CTEST_EXCLUSIONS=$CTEST_EXCLUSIONS" >> "$GITHUB_ENV"' }}

That'll have the same effect, without defining all the Linux_5, Windows_6, etc. envvars that'd otherwise persist through the entire job run. The comments from the individual steps in the current workflow could also, I believe, be included between each condition without affecting the final result.

Sorry but IMO it's a lot easier to have multiple steps that are way easier to read.
This workaround GitHub actions lack of ternary options is not obvious so I prefer to avoid it.

ferdnyc · 2023-10-03T04:30:47Z

.github/workflows/build.yml

+      - name: Create build directory
+        run: mkdir build
+


It's really unnecessary to waste a step on this, CMake does it for you.

ferdnyc · 2023-10-03T04:34:37Z

.github/workflows/build.yml

+      - name: Install Qt with options and default aqtversion
+        uses: jurplel/install-qt-action@v3
+        with:
+          aqtversion: null # use whatever the default is


I'm trying to understand why this is here.

If it's not specified at all, the default will be used, right? That's why it's the default. Why would it be necessary to explicitly ask for a default?

We can remove ofcourse, but I feel like this might be needed in future, also helps one knowing that it uses aqt behind the scenes

That's fair.

ferdnyc · 2023-10-03T19:22:49Z

So, some of my review comments on the exclusion structure aren't entirely accurate.

I did eventually get it working, like this:

env:
  Windows_5: 'quicktexturetest|bindinginspectortest'
  Windows_6: 'quicktexturetest|launchertest|bindinginspectortest|modelinspectortest'
  macOS_5: 'connectiontest-*|probeabidetectortest|launchertest|quickinspectortest2'
  macOS_6: 'connectiontest-*|probeabidetectortest|launchertest|clientconnectiontest|bindinginspectortest|modelinspectortest|quickinspectortest2' 
  Linux_5: 'quickmaterialtest|quicktexturetest|connectiontest-style-filter'
  Linux_6: 'quickmaterialtest|quicktexturetest|connectiontest-style-filter|bindinginspectortest|quickinspectortest|quickinspectortest2|modelinspectortest'

jobs:
  build:
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-latest, macos-latest, windows-latest]
        build_type: [Debug]
        client_only: [ON, OFF] 
        qt:
          - { version: 5, request: 5.15.*, modules: }
          - { version: 6, request: 6.5.*, modules: qtshadertools }
    runs-on: ${{ matrix.os }}
    steps:
      - name: "Select exclusions"
        env:
          CTEST_EXCLUSIONS: ${{ (runner.os == 'Windows' && matrix.qt.version == 6) && env.Windows_6 || '' }}${{ (runner.os == 'Windows' && matrix.qt.version == 5) && env.Windows_5 || '' }}${{ (runner.os == 'Linux' && matrix.qt.version == 5) && env.Linux_5 || '' }}${{ (runner.os == 'Linux' && matrix.qt.version == 6) && env.Linux_6 || '' }}${{ (runner.os == 'macOS' && matrix.qt.version == 5) && env.macOS_5 || '' }}${{ (runner.os == 'macOS' && matrix.qt.version == 6) && env.macOS_6 || '' }}
        run: >
          ${{ runner.os == 'windows'
          && '"CTEST_EXCLUSIONS=$env:CTEST_EXCLUSIONS" | Out-File -FilePath $env:GITHUB_ENV -Append'
          || 'echo "CTEST_EXCLUSIONS=$CTEST_EXCLUSIONS" >> "$GITHUB_ENV"' }}

      - name: "Configure Linux test environment"
        if: runner.os == 'Linux' && matrix.client_only == 'OFF'
        run: |
          echo "QT_QPA_PLATFORM=offscreen" >> "$GITHUB_ENV"
          echo "QT_QUICK_BACKEND=software" >> "$GITHUB_ENV"

The envvar definition has to be all on one line like that, or the value ends up with spaces in it that throw off the expression matching. (GitHub really needs to add a trim() function to their ${{ }} expression syntax.)

Which admittedly makes it kind of ugly. Accusations that I'm being too pointlessly "clever" graciously accepted.

I should've mentioned, BTW: the latest (still in-progress, as of this moment) run of my version of the workflow can be seen here: ~~https://github.com/ferdnyc/GammaRay/actions/runs/6397369854~~

I had to add the bindinginspectortest to the macOS 6 exclusions, as well, and I'm still waiting to see what the final outcome of the Windows attempts will be.

The full workflow itself is: https://github.com/ferdnyc/GammaRay/blob/master/.github/workflows/test-trickery.yml

(Edit: Corrected. (The Windows Qt 6 exclusions weren't being set.) Previous run stopped, new one is... well, whichever one is at the top of https://github.com/ferdnyc/GammaRay/actions/workflows/test-trickery.yml at any given moment.)

dantti · 2023-10-03T21:31:15Z

Well IMO this huge one line is quite unreadable, plus, this should be temporary, we need these tests either permanently skipped or fixed

dantti force-pushed the dantti/gha_build branch from f4cacba to 41a94e3 Compare September 22, 2023 20:02

dantti force-pushed the dantti/gha_build branch from 41a94e3 to 8e61197 Compare September 22, 2023 20:27

dantti force-pushed the dantti/gha_build branch 10 times, most recently from dc0d289 to ce84688 Compare October 1, 2023 20:17

ferdnyc reviewed Oct 3, 2023

View reviewed changes

dantti force-pushed the dantti/gha_build branch from ce84688 to e2029e1 Compare October 3, 2023 17:29

dantti force-pushed the dantti/gha_build branch from e2029e1 to 39eec8d Compare October 3, 2023 21:36

dantti force-pushed the dantti/gha_build branch 2 times, most recently from 4c215a8 to 323f5bd Compare November 15, 2023 22:10

Add GitHub actions

c0bc122

dantti force-pushed the dantti/gha_build branch from 323f5bd to c0bc122 Compare November 15, 2023 22:25

dantti merged commit 68e6f5b into master Nov 16, 2023
14 checks passed

dantti deleted the dantti/gha_build branch November 16, 2023 12:06

dantti mentioned this pull request Nov 16, 2023

Add Github Actions CI builds #768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitHub actions #838

Add GitHub actions #838

dantti commented Sep 22, 2023

winterz commented Sep 22, 2023

dantti commented Oct 1, 2023

ferdnyc left a comment

ferdnyc Oct 2, 2023

ferdnyc Oct 3, 2023

dantti Oct 3, 2023

ferdnyc Oct 2, 2023

dantti Oct 3, 2023

ferdnyc Oct 2, 2023

ferdnyc Oct 3, 2023

dantti Oct 3, 2023

ferdnyc Oct 3, 2023

ferdnyc Oct 3, 2023

ferdnyc Oct 3, 2023

dantti Oct 3, 2023

ferdnyc Oct 3, 2023

ferdnyc Oct 3, 2023

dantti Oct 3, 2023

ferdnyc Oct 3, 2023

ferdnyc commented Oct 3, 2023 •

edited

Loading

dantti commented Oct 3, 2023

Add GitHub actions #838

Add GitHub actions #838

Conversation

dantti commented Sep 22, 2023

winterz commented Sep 22, 2023

dantti commented Oct 1, 2023

ferdnyc left a comment

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

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

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

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

Choose a reason for hiding this comment

ferdnyc commented Oct 3, 2023 • edited Loading

dantti commented Oct 3, 2023

ferdnyc commented Oct 3, 2023 •

edited

Loading