Skip to content

CI: Add build using Qt6 on Arch Linux#4428

Merged
Be-ing merged 1 commit intomixxxdj:qt6from
Holzhaus:qt6-ci
Oct 17, 2021
Merged

CI: Add build using Qt6 on Arch Linux#4428
Be-ing merged 1 commit intomixxxdj:qt6from
Holzhaus:qt6-ci

Conversation

@Holzhaus
Copy link
Copy Markdown
Member

@Holzhaus Holzhaus commented Oct 15, 2021

Do not merge until qt6 build works.

Comment thread .github/workflows/build.yml Outdated
container: holzhaus/mixxx-ci:20211015-qt6
# QTKEYCHAIN needs to be disabled until this commit has been part
# of a release and the docker image has been updated:
# https://github.com/frankosterfeld/qtkeychain/commit/ae1f9a479001d519d209d19da1b9319bf4d05a28
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add this commit as a patch to the Arch package? I have done that for vcpkg: microsoft/vcpkg#20185

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I'll look into it. It would be much easier if they just tagged a release that includes that patch :-/

I also opened a bug report at the arch linux bug tracker: https://bugs.archlinux.org/task/72446
Maybe they will also patch the package.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dvzrv maybe you could help us with this qtkeychain package?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Already patched it in the docker container: Holzhaus/mixxx-ci-docker@2ce7e5d

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ooof, GitHub CI needs root privs in the docker container, pushed a new commit: Holzhaus/mixxx-ci-docker@b4b601b

@Holzhaus Holzhaus force-pushed the qt6-ci branch 2 times, most recently from 8266e58 to 2cb293a Compare October 16, 2021 22:07
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

Merge?

@uklotzde
Copy link
Copy Markdown
Contributor

Let's merge or rebase on main and see the results before merging.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

Why? We know the Qt6 jobs will fail for now.

@Holzhaus
Copy link
Copy Markdown
Member Author

Then we shouldn't merge yet IMHO. I dont want to lose the green checkmark on non-qt6 PRs.

Maybe we cna make a Qt6 branch that we can merge this to?

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I dont want to lose the green checkmark on non-qt6 PRs.

I don't think we should hold back merging for this. It's still easy to see that everything else passed.

Maybe we cna make a Qt6 branch that we can merge this to?

Please no. That would only bring superfluous complexity.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I am not comfortable only building QML for Qt6 without having Qt6 CI setup. Let's merge this now.

@uklotzde
Copy link
Copy Markdown
Contributor

Or a qt6-ci branch dedicated to performing CI builds after merging main periodically? All Qt6 development will continue on main. The only drawback is that new CI build failures are detected after merging PRs into main.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I don't think it's a good idea to make another branch. IMO we need to tolerate some temporary imperfection to move forward on the long path ahead for Qt6.

@uklotzde
Copy link
Copy Markdown
Contributor

As long as the Qt6 builds don't result in a usable executable we can tolerate some delay between causing and noticing regressions. The qt6-ci branch could be deleted after the first Qt6 build passed.

This extra branch doesn't cause too much hassle as long as we only merge main into it, i.e. qt6-ci = main + this CI patch.

@uklotzde
Copy link
Copy Markdown
Contributor

Let's try it. If it doesn't work we can decide to drop it at any time.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I'm not going to bother setting up extra complexity to only run CI jobs periodically on some other branch and I think this would be pointless. What will most likely happen if someone wastes time on that is that the CI jobs on that other branch will be ignored entirely.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

It's not hard to scroll to the bottom of a pull request and see which checks failed and passed. It's really not a big deal to have a red X on https://github.com/mixxxdj/mixxx/pulls

@uklotzde
Copy link
Copy Markdown
Contributor

Disturbing the CI checks for everyone on main for an unforeseeable time is a strong burden.

I recommend the extra branch as proposed. One step after another.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

More Git branches only creates extra work. Let's get everything into main ASAP.

@uklotzde
Copy link
Copy Markdown
Contributor

I am not going to open PRs one by one and scroll through all test results before deciding if it is ready for review.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

Then the only route forward I see is piling all changes needed for Qt6 into one giant pull request including starting Qt6 CI.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

That or tolerate having entire source code files that aren't built on CI for some time. I would much rather tolerate seeing a red X on https://github.com/mixxxdj/mixxx/pulls

@Be-ing Be-ing marked this pull request as ready for review October 17, 2021 19:23
@Holzhaus
Copy link
Copy Markdown
Member Author

Holzhaus commented Oct 17, 2021

Then the only route forward I see is piling all changes needed for Qt6 into one giant pull request including starting Qt6 CI.

We can also just make a qt6 branch with CI, then merge that branch into main and revert the commit that adds Qt6 CI on main.

Then all Qt6 PRs can be opened against the qt6 branch. We can regularily merge the qt6 branch into main.

Because the CI commit was reverted on main, CI on main will still pass. We can drop the qt6 branch when Qt6 CI starts to pass. Then we can also re-add Qt6 CI on main.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

Creating big branches is a setup for disaster. I will stop working on Qt6 support if this is done just for the sake of not seeing a red X.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I am already very frustrated having to juggle so many unmerged branches.

@Holzhaus
Copy link
Copy Markdown
Member Author

This is not much different to what we do with the 2.3 branch vs. main.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I will not keep working on Qt6 support if unnecessary hassles are put in my way, especially not for something as trivial as an icon.

@Holzhaus
Copy link
Copy Markdown
Member Author

Creating big branches is a setup for disaster. I will stop working on Qt6 support if this is done just for the sake of not seeing a red X.

Have you read my comment here? #4428 (comment)

There would be no big branch, because the qt6 branch will be regularily merged into main. The only difference between the qt6 branch and main would be that qt6 contains a commit that adds Qt6 CI and main additionally contains a commit that reverts that commit (so that Qt6 CI is not added to main when we merge qt6 into main).

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I am highly doubtful anyone will bother paying attention to some superfluous branch's CI results.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

I see two choices:

  1. Tolerate not building some files on CI for a while.
  2. Tolerate seeing a red X for a while.

2 seems much better IMO.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

A branch solely for CI that nobody pays attention to is no more useful than running builds locally.

@Holzhaus
Copy link
Copy Markdown
Member Author

I am highly doubtful anyone will bother paying attention to some superfluous branch's CI results.

What do you mean? The qt6 PRs go against the qt6 branch, which means that we can look at those CI results on every Qt6 PR.

I see two choices:

  1. Tolerate not building some files on CI for a while.
  2. Tolerate seeing a red X for a while.

The issue with (2) is that we accustom ourselves to ignoring the CI check mark. And because the Qt6 CI check always fails, what difference does it make compared to not having that check at all? So I think we should have a qt6 branch that can be used to resolve Qt6 build issues. As soon as it builds we can drop the branch and use main instead.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

The qt6 PRs go against the qt6 branch

I will not continue working on Qt6 this way. This is a setup for trouble.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

There would be no big branch, because the qt6 branch will be regularily merged into main.

Sorry, I overlooked this in frustration. If the qt6 branch is merged into main every time we merge a PR to the qt6 branch then I think this could work.

@Be-ing Be-ing changed the base branch from main to qt6 October 17, 2021 19:59
@Be-ing Be-ing merged commit f12da9d into mixxxdj:qt6 Oct 17, 2021
Holzhaus added a commit that referenced this pull request Oct 17, 2021
This reverts commit f12da9d, reversing
changes made to f83869b.
@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

How will we maintain the qt6 branch? Keep merging main -> qt6, then merge branches into qt6, then merge qt6 -> main? This seems messy. Alternatively we can rebase PRs onto main right before merging.

@daschuer
Copy link
Copy Markdown
Member

Merging the qt6 branch into main does not work because the failing CI will move along with it.
The only workaround I have in mind that avoids a Giant Qt6 merge is this:
We can treat this branch as sandbox and merge this PR into a private repro branch, to verify a PR. Once it is working we may add a regular PR to main.

@daschuer
Copy link
Copy Markdown
Member

Uh, the reverted commit 12019eb in main prevents that.
Can we create another Qt branch from main and re-revert the commit?

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 17, 2021

None of this cross-cross merge mess or Git tricks would be needed if we just looked at the individual CI checks.

@uklotzde
Copy link
Copy Markdown
Contributor

Let's target main for all PRs as originally proposed and simply merge main into qt6 periodically. No big deal. CI failures that only affect Qt6 can be fixed in the aftermath.

@Holzhaus
Copy link
Copy Markdown
Member Author

Can we create another Qt branch from main and re-revert the commit?

Why? The qt6 branch has the CI. We can re-revert the commit when Qt6 builds properly and we stop using the qt6 branch.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 18, 2021

Criss-cross merges and repeated reversions get confusing and messy. How about we keep the matrix condition in the workflow file in main but commented out? Then if you want to run a Qt6 job in a PR, temporarily uncomment it.

@Holzhaus
Copy link
Copy Markdown
Member Author

I don't see any problem with the proposed method and think it's far easier and less messy than uncommenting the workflow all the time. But If you like it better go ahead.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Oct 18, 2021

I will make a PR after #4432 is merged.

JaviVilarroig pushed a commit to JaviVilarroig/mixxx that referenced this pull request Oct 26, 2021
This reverts commit f12da9d, reversing
changes made to f83869b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants