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

treewide: Qt5.X -> Qt5.15 (when possible) #102840

Merged
merged 57 commits into from Nov 20, 2020
Merged

Conversation

freezeboy
Copy link
Contributor

@freezeboy freezeboy commented Nov 4, 2020

Motivation for this change

I tried to migrate the software still using Qt5.12 to a newer version, sometimes I had to update the software to make it work.

Among the reasons blocking the packages, I already spotted:

  • Dependency to qtwebkit which is broken with Qt5.15
  • Use of QPainterPath class, apparently the header definition has moved between 5.14 and 5.15
  • Some package build configurations forbid deprecated members use ... and there are such in Qt5.15
  • Other oddities in the code that is not more valid (changing, return types)
  • A few of them compile .. then segfault (digitalbitbox)
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

This is a tremendous work! Did you test each and every package to build and run right?

Overall I approve, but the commit messages sometimes miss some details of other changes, it might help to get this approved but I wouldn't make this a blocker.

@freezeboy
Copy link
Contributor Author

Yes I built and run all the executables, just to check it starts.

For the commit messages, I can see to improve them, at least the ones where an update was needed.

@doronbehar
Copy link
Contributor

Well done 👏.

@freezeboy
Copy link
Contributor Author

Most of the effort is done by my cpu and its fan 😏

@freezeboy
Copy link
Contributor Author

I force pushed with more details in the commit messages, please tell me if you need more

@doronbehar
Copy link
Contributor

Pushed after a rebase to fix merge conflicts. Ideally I think every usage of a pinned libsForQt5.. should be accompanied with a comment. I counted 129 usages of libsForQt514.callPackage that come with no comment explaining them.

@freezeboy
Copy link
Contributor Author

Thank you, I will continue to add other apps, and I think we can think to merge this PR, I will start another PR for to continue this process otherwise we will have more and more conflicts

freezeboy and others added 17 commits November 20, 2020 22:24
 * Port to Qt5.15 in the process
 * Cleanup the derivation to use new style
Fixes build for Qt5.15

Split output to mutilple outputs to reduce user closure size
moved the initial qtcurve package to mkLibsForQt5 function
to decouple from Qt5 version

added an alias qtcurve -> libsForQt5.qtcurve for backward compatibility

add option to disable gtk2 support (still enabled by default)
Did not touch the dependency to python27 but may require another PR
obs-nvi was not touched but should be modified as well,
it requires a custom download that I don't use, so I prefer to
let official maintainer do it.
@freezeboy
Copy link
Contributor Author

Rebase done

@ofborg ofborg bot removed the 6.topic: qt/kde label Nov 20, 2020
@jonringer
Copy link
Contributor

LGTM

failures exist on master

https://github.com/NixOS/nixpkgs/pull/102840
2 packages marked as broken and skipped:
retroshare retroshare06

3 packages failed to build:
kmymoney obs-ndi xflr5

70 packages built:
amarok appcsxcad birdtray calaos_installer chiaki clipgrab communi csound-qt cutecom digitalbitbox eagle flameshot fritzing golden-cheetah herqq kdiff3 kgraphviewer krename kronometer ktorrent libsForQt5.herqq libsForQt5.kde2-decoration qcsxcad qtcurve libsForQt512.herqq libsForQt512.kde2-decoration libsForQt512.qcsxcad libsForQt512.qtcurve libsForQt514.kde2-decoration libsForQt514.qcsxcad libsForQt514.qtcurve luminanceHDR massif-visualizer minitube obs-move-transition obs-studio obs-v4l2sink obs-wlrobs openems ostinato pentobi peruse plex-media-player protonmail-bridge psi psi-plus python27Packages.python-csxcad python37Packages.python-csxcad python37Packages.python-openems python38Packages.python-csxcad python38Packages.python-openems qlcplus rsibreak rssguard saga samplv1 sdrangel sigil skanlite skrooge solarus solarus-quest-editor sonic-lineup strawberry teamspeak_client tellico virtmanager-qt vnote webcamoid zoom-us

@jonringer jonringer merged commit f0aac91 into NixOS:master Nov 20, 2020
@freezeboy
Copy link
Contributor Author

I guess obs-ndi is normal as the rest of the stack now use qt5.15, I can change its Qt version too, but I won't be able to build it anyway

I start looking the other errors

@freezeboy freezeboy deleted the update-fixes branch November 20, 2020 22:15
@jonringer
Copy link
Contributor

I guess obs-ndi is normal as the rest of the stack now use qt5.15, I can change its Qt version too, but I won't be able to build it anyway

I start looking the other errors

either way, this is still an improvement. Thanks! :)

@ghost ghost mentioned this pull request Nov 21, 2020
@doronbehar
Copy link
Contributor

3 packages failed to build:
kmymoney obs-ndi xflr5

That's a regression from the last time I tried to build kmymoney - it fails due to xmlsec not building:

https://gist.github.com/doronbehar/4b34941feb73855eb307b3470d12dff2

@doronbehar
Copy link
Contributor

@freezeboy when you rebased it you forced pushed and forgot some commits of mine :(. For instance qtwebkit was fixed there.

@doronbehar
Copy link
Contributor

Follow up PR: #104474

kmymoney needs #104472 in order to work.

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.

None yet

8 participants