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

Update Linux-GitHub runner to Ubuntu 24.04.01 LTS #13781

Merged
merged 13 commits into from
Nov 11, 2024

Conversation

JoergAtGithub
Copy link
Member

According our https://github.com/mixxxdj/mixxx/wiki/Minimum-requirements-policy Ubuntu 24.04.01 LTS is the earliest version, that developers need to support in new code.
I'm working on some code that fullfills this requirement, but does not build on CI, because the version of the Linux GitHub runner is lagging behind the minimum requirement. This update ensures that the CI will still pass.

I found out, that clazy version 1.11, that Ubuntu ships with 24.04.01 LTS is incompatible with the included Qt 6.4. Therfore we can't use the clazy package from the Ubuntu distribution. I added the steps to download and build it as described on the clazy homepage.

@acolombier
Copy link
Member

acolombier commented Oct 19, 2024

Overall, I'm not sure this is a good idea to drop support for Ubuntu 22.04/Qt 6.2 LTS. A few reasons for that:

  • Ubuntu 24.04 doesn't rely on a LTS version of Qt
  • PopOS (which I appear to be using for full disclosure) hasn't released a 24.04 LTS due to some delay on their end

Appreciate that last point is strongly biased, but as PopOS is still fairly popular among the Linux community, so I think it would be wise to support at least, at least till we provide support for the next Qt LTS version on Linux.
Another point is, I know this is the established strategy in our wiki, but is it wise to drop support for Ubuntu LTS after only 2 years? I understand we may not want to follow precisely Canonical's strategy, but many of our Linux community relies on Ubuntu and derivatives and usually pick LTS version to ensure they won't have to break and reinstall their system every year or 2.

TL;DR is, could we support both 22.04 and 24.04, at least till we move to support the next Qt LTS version?

@JoergAtGithub
Copy link
Member Author

* Ubuntu 24.04 doesn't rely on a LTS version of Qt

That make no difference, because 6.2 LTS and 6.4 are both out of maintenance. 6.5 is the oldes Qt6 release where bugfixes will be developed for: https://endoflife.date/qt

but is it wise to drop support for Ubuntu LTS after only 2 years?

We decided to introduce these rules, to not hinder development. Ubuntu has already very old packages, at the time of release. So we have a delay of more than 2 years until we can use upstream features.

@acolombier
Copy link
Member

could we support both 22.04 and 24.04, at least till we move to support the next Qt LTS version?

How about this?

@JoergAtGithub
Copy link
Member Author

could we support both 22.04 and 24.04, at least till we move to support the next Qt LTS version?

How about this?

This would prevent us to use new functionality.

We introduced this rule based system to give all devs predictability, and not to depend on case by case decisions with long discussions.

@daschuer
Copy link
Member

We have established our Minimum-requirements-policy after exchanging quite some arguments. One major argument for me was, the desire of contributors to gain experience with bleeding etch features soon after they are available. This is "feel good" issue is important IMHO. So there is no reason to repeat this discussion.

It does on the other hand not stop us from maintaining a back port branch if there is a need for it. I am for example still at Ubuntu Focal with one of my devices to test the 2.4. branch (and urgently need to update it btw., we are beyond the deadline) and maintain such a branch: https://github.com/daschuer/mixxx/tree/focal_main.

I will do a similar branch for Ubuntu Jammy as well, that you may use as well. We may consider to make it more accessible and move it to "mixxxdj/mixxx". This is controversial of cause.

@@ -81,6 +97,7 @@ jobs:
-DMAD=ON \
-DMODPLUG=ON \
-DWAVPACK=ON \
-DCMAKE_CXX_FLAGS="-Wno-nan-infinity-disabled" \
Copy link
Member

Choose a reason for hiding this comment

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

We should use our fpclassify.cpp instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a regression of this PR, as the check hasn't existed before. Therefore it's out of scope of this PR!

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but let's keep the warning until the issue is fixed in the other PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

But than all CI runs fail?

Copy link
Member

Choose a reason for hiding this comment

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

Opened a new issue, so that we don't we forget about it: #13837

Problem solved?

name: ${{ matrix.name }}
steps:
- name: Check out repository
uses: actions/checkout@v4.2.1
- name: Install build dependencies
run: tools/debian_buildenv.sh setup
- name: Build clazy v1.12
if: matrix.name == 'clazy'
# Ubuntu2404 comes with v1.11, which is too old for the Qt6.4 shipped with Ubuntu 2404
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a reference for this, a Mixxx and a Ubuntu bug? This sounds like an issue we should solve in our https://github.com/daschuer/mixxx/blob/focal_main/tools/debian_buildenv.sh instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could move it to debian_buildenv.sh, but than clazy would be always build and not only for the clazy CI action.

Copy link
Member

Choose a reason for hiding this comment

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

The goal should be to have a working clazy locally by default. I looked around to find a PPA for that but not yet found. Maybe we add this to our Launchpad PPA? What is the issue you trying to solve btw.?

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 can not compile the Qt6.4 headers, because of unsupported C++ features

Copy link
Member

Choose a reason for hiding this comment

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

This takes 5 min. A bit long IMHO.
With a bit of luck the Ocular version will work: https://packages.ubuntu.com/oracular/clazy

Copy link
Member

@daschuer daschuer Nov 5, 2024

Choose a reason for hiding this comment

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

using the Oracular build works

          echo "deb http://archive.ubuntu.com/ubuntu oracular main universe" | sudo tee /etc/apt/sources.list.d/oracular-temp.list
          echo -e "Package: *\nPin: release n=oracular\nPin-Priority: 100" | sudo tee /etc/apt/preferences.d/oracular.pref
          sudo apt update
          sudo apt install -t oracular clazy

Penalty 46s

Copy link
Member

Choose a reason for hiding this comment

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

The Oracular hack updates a lot of dependency. Nothing I would like to do at home.

The macOS solution sounds promising. It looks like macOS builds are in general faster so it is probably a win-win.

Copy link
Member Author

Choose a reason for hiding this comment

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

macOS is not the way to go, the Homebrew installation of Clazy took 1h 50m 50s because it builds the whole llvm package as dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's bad. So we have two remaining options. Push the clazy build to our PPA, or make use of the GitHub cache. I will look in the PPA solution. It should be straight forward using the Oracular rule file. This has the benefit that Ubuntu users can use it at home as well.

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've also the docker container solution proposed by Jan as third option. But I'm also in favor to trying the PPA solution first, as it covers not only CI.

.github/workflows/build-checks.yml Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
#pragma once
#include <QAbstractListModel>
#include <QtQml>
#include <QQmlEngine>
Copy link
Member

Choose a reason for hiding this comment

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

This is also out of scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary, that Clazy passes, should I move it to an own PR?

Copy link
Member

Choose a reason for hiding this comment

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

It is not very important. I actually prefere to have everything fixed either in one go or before doing the switch. This as well as the nan-infinity-disabled issue. We need to do it anyway at one point, let's avoid intermediate workarounds.

@daschuer
Copy link
Member

daschuer commented Nov 8, 2024

JoergAtGithub#2
Contains the new clazy and the infinity fix.
It was not the upload no brainer I have first thought because of failing test with the older Qt version. But now everything is in shape. .. hopefully.

@JoergAtGithub
Copy link
Member Author

This is ready for merge now:

@@ -81,6 +90,7 @@ jobs:
-DMAD=ON \
-DMODPLUG=ON \
-DWAVPACK=ON \
-DCMAKE_CXX_FLAGS="-Wno-nan-infinity-disabled" \
Copy link
Member

@Swiftb0y Swiftb0y Nov 10, 2024

Choose a reason for hiding this comment

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

could you add a comment at the issue locations so we don't forget to remove this line once fixed?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you.

@daschuer
Copy link
Member

Unfortunately we have a conflict now.

…Wno-nan-infinity-disabled when the issue is fixed
@JoergAtGithub
Copy link
Member Author

I fixed the merge-conflict and added the comments suggested by Niko.

@daschuer
Copy link
Member

Thank you.

@daschuer daschuer merged commit c39cc5f into mixxxdj:main Nov 11, 2024
13 checks passed
@JoergAtGithub JoergAtGithub deleted the ubuntu_240401 branch November 11, 2024 18:43
rev: 37c2513b1b8275a475a160ed2f5b044910335d5f # No release tag yet including #6 fix
rev: latest # No release tag yet including #6 fix
Copy link
Member

Choose a reason for hiding this comment

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

this should be reverted... pre-commit does that automatically unfortunately when running autoupdate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened PR #13880, which reverts this unintended change.

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

Successfully merging this pull request may close these issues.

5 participants