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

Don't use deprecated Qt5 functions #5156

Closed
wants to merge 8 commits into from

Conversation

lukas-w
Copy link
Member

@lukas-w lukas-w commented Aug 27, 2019

Continuation of @noahb01's #5086. I rebased on master, removed whitespace changes, squashed related commits and removed the remaining usages of QSignalMapper.

@PhysSong
Copy link
Member

As I mentioned in #5086 (comment), e39d6f3 requires Qt 5.6, whereas Travis Windows builds use 5.4.

@lukas-w
Copy link
Member Author

lukas-w commented Aug 29, 2019

@PhysSong Should we just remove the Travis Windows builds? We already test with Ubuntu 18.04 Qt 5.9 on CircleCI and I don't see a reason to support Qt versions older than that.

@PhysSong
Copy link
Member

Should we just remove the Travis Windows builds?

I think it will be fine once I finish working on #5142.

We already test with Ubuntu 18.04 Qt 5.9 on CircleCI and I don't see a reason to support Qt versions older than that.

For references, Ubuntu 16.04 ships with Qt 5.5. Upstream dropped support for that version last year. Qt 5.9 LTS is the oldest version which is still supported by upstream. Qt 5.6 LTS has been unsupported since 16.03.2019, but I think it's too early to drop support for Qt 5.6.

@lukas-w
Copy link
Member Author

lukas-w commented Aug 29, 2019

I think it's too early to drop support for Qt 5.6.

Can you explain why? Linux users don't benefit from this because we bundle Qt in our AppImages anyway and I think developers can be expected to install a recent Qt version through a PPA or the official Qt installer.

@PhysSong
Copy link
Member

Linux users don't benefit from this because we bundle Qt in our AppImages anyway

Agreed. BTW, does that mean we're going to deprecate LMMS packages from Linux distributions?
Downstream packagers should take care of the Qt version requirement, but now I think they won't package the new version of LMMS if they don't bundle a recent version of Qt. In other words, I think it's not a big deal for normal users.

I think developers can be expected to install a recent Qt version through a PPA or the official Qt installer.

If you want to go through the way, you should document this first.

@PhysSong
Copy link
Member

FYI, the CircleCI Linux build uses Ubuntu 16.04 now because of some issues with AppImages. It means the build uses Qt 5.5 unless upgraded by some packages from the official installer or a PPA.
And we can remove the Travis MinGW builds if you want because packaging works with the newer MinGW toolchains/libraries and even with MSVC.

@lukas-w
Copy link
Member Author

lukas-w commented Oct 30, 2019

Downstream packagers should take care of the Qt version requirement, but now I think they won't package the new version of LMMS if they don't bundle a recent version of Qt. In other words, I think it's not a big deal for normal users.

Exactly, any distro ship the next LMMS release will also provide a recent Qt version.

FYI, the CircleCI Linux build uses Ubuntu 16.04 now because of some issues with AppImages

Having to downgrade to an Ubuntu version that has reached end of life because of AppImages is unfortunate. Not that I've been active enough lately to judge but I feel like changes in AppImage and linuxdeployqt are constantly breaking our build. Out of curiosity, can you provide a link for context as to what went wrong? What's the oldest version of Ubuntu we want to support for AppImage users?

If you want to go through the way, you should document this first.

Added instructions to use Qt 5.6+ to the wiki and added the 5.6+ requirement to CMake via 60fc79e.

@PhysSong
Copy link
Member

From Discord:

It seems like linuxdeployqt added a check for libc6 bundling when using -unsupported-allow-new-glibc:
probonopd/linuxdeployqt@586aaf7

So, we decided to revert to Ubuntu 16.04 and it's done in #5212.

@lukas-w lukas-w force-pushed the noahb01-deprecation-warnings branch from 60fc79e to 400c8d8 Compare October 30, 2019 11:13
@LmmsBot
Copy link

LmmsBot commented Oct 30, 2019

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"commit_sha": "eb5940d348c13b72fc2b3f6f28983e868d1af8be", "platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://771-15899248-gh.circle-artifacts.com/0/lmms-1.2.0-rc4.956-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/lukas-w/lmms/771?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://770-15899248-gh.circle-artifacts.com/0/lmms-1.2.0-rc4.956-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/lukas-w/lmms/770?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://767-15899248-gh.circle-artifacts.com/0/lmms-1.2.0-rc4.956-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/lukas-w/lmms/767?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://768-15899248-gh.circle-artifacts.com/0/lmms-1.2.0-rc4.956-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/lukas-w/lmms/768?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}}

@PhysSong
Copy link
Member

@lukas-w FYI, I think we can add a newer Qt from a PPA on our CircleCI Linux build image.

@lukas-w
Copy link
Member Author

lukas-w commented Oct 30, 2019

@PhysSong Thanks for the hints. Working on it right now.

@lukas-w
Copy link
Member Author

lukas-w commented Oct 30, 2019

Fixed via eb5940d and LMMS/lmms-ci-docker#7, should be good to merge now.

@PhysSong
Copy link
Member

@lukas-w Since I already pushed commits for CircleCI changes, is it okay to drop the last commit? Sorry for missing this one.

@lukas-w
Copy link
Member Author

lukas-w commented Oct 31, 2019

@PhysSong No worries. I merged 400c8d8 into master, leaving out the last redundant commit.

@lukas-w lukas-w closed this Oct 31, 2019
@lukas-w lukas-w deleted the noahb01-deprecation-warnings branch October 31, 2019 08:43
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants