Skip to content

pyqt5: make qtwebkit optional, disable by default#51846

Merged
FRidh merged 10 commits intoNixOS:masterfrom
veprbl:pr/pyqt5_opt_qtwebkit
Dec 30, 2018
Merged

pyqt5: make qtwebkit optional, disable by default#51846
FRidh merged 10 commits intoNixOS:masterfrom
veprbl:pr/pyqt5_opt_qtwebkit

Conversation

@veprbl
Copy link
Member

@veprbl veprbl commented Dec 10, 2018

Motivation for this change

qtwebkit appears to be unsupported in Qt 5.11. We are using some old port

# Community port of the now unmaintained upstream qtwebkit.
qtwebkit = {
src = fetchFromGitHub {
owner = "annulen";
repo = "webkit";
rev = "4ce8ebc4094512b9916bfa5984065e95ac97c9d8";
sha256 = "05h1xnxzbf7sp3plw5dndsvpf6iigh0bi4vlj4svx0hkf1giakjf";
};
version = "5.212-alpha-01-26-2018";
};

and it is broken on darwin. We are likely will be looking to be phasing out QtWebKit in the future as it will break and have more and more security problems. It is nice to have packages that still rely on it to be marked out.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
  • Tested compilation of all pkgs that depend on this change using nix-review pr 51846
  • Tested execution of all binary files (usually in ./result/bin/)
  • Test if QtWebKit is needed (grep -r QtWebKit ... and try to run the binary)
    • anki
    • blink - broken runtime, but grep shows that it does import QtWebKit and QtWebKitWidgets
      • fix
      • test (Segfaults on master)
    • cadence - works, but could potentially try to import QtWebKitWidgets
      • fix
      • test
    • calibre - imports QtWebKitWidgets
      • fix
      • test
    • cura
    • electron-cash - was broken on Hydra due to cytoolz, setup-release.py says it needs PyQt5.QtWebKit doesn't need QtWebKit
    • electrum - crashes, grep QtWebKit on sources gives nothing
    • electrum-ltc
    • flent
    • frescobaldi - imports QtWebKitWidgets
      • fix
      • test
    • git-cola - imports QtWebKitWidgets imports QtWebKitWidgets, but only if QtWebEngine is not available
    • gns3-gui
    • hplip
    • hplipWithPlugin
    • ihaskell - depends on jupyter
    • kmymoney
    • krop
    • leo-editor - if QtWebKit is not available seems to gracefully switch to QtWebEngine
    • manuskript
    • multibootusb
    • nagstamon
    • openshot-qt - imports QtWebKitWidgets
      • fix
      • test
    • picard
    • plover.dev
    • python27Packages.jupyter
    • python27Packages.ovito
    • python27Packages.poppler-qt5
    • python27Packages.qtconsole
    • python27Packages.weboob
    • python37Packages.jupyter
    • python37Packages.ovito
    • python37Packages.poppler-qt5
    • python37Packages.qtconsole
    • python37Packages.uranium
    • scudcloud - needs QtWebKitWidgets
      • fix
      • test
    • qarte
    • qutebrowser
    • spyder
    • tribler
    • urh
  • Investigate failed builds
    • gitAndTools.git-annex-metadata-gui - broken on Hydra due to git-annex-adapter, grep QtWebKit on sources gives nothing
    • mnemosyne - broken on Hydra due to objgraph, judging by sources it uses QtWebEngine, grep QtWebKit on sources gives nothing
    • rapid-photo-downloader - broken on Hydra due to rawkit, grep QtWebKit on sources gives nothing

@veprbl veprbl requested a review from FRidh as a code owner December 10, 2018 21:27
@GrahamcOfBorg GrahamcOfBorg added the 6.topic: python Python is a high-level, general-purpose programming language. label Dec 10, 2018
@veprbl
Copy link
Member Author

veprbl commented Dec 10, 2018

@GrahamcOfBorg build python27Packages.pyqt5 python37Packages.pyqt5

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Dec 10, 2018
@veprbl veprbl added the 6.topic: qt/kde Object-oriented framework for GUI creation label Dec 11, 2018
@veprbl
Copy link
Member Author

veprbl commented Dec 11, 2018

I guess I should not remove QtWebEngine...

@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch 3 times, most recently from d9d81e5 to 3fe8491 Compare December 11, 2018 17:40
@GrahamcOfBorg GrahamcOfBorg added 8.has: package (new) This PR adds a new package and removed 6.topic: qt/kde Object-oriented framework for GUI creation labels Dec 11, 2018
@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch from 6edeb89 to 2e48b0f Compare December 12, 2018 01:00
@veprbl
Copy link
Member Author

veprbl commented Dec 14, 2018

This is ready for review now

@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch from 2e48b0f to 80bb03a Compare December 20, 2018 21:48
@veprbl
Copy link
Member Author

veprbl commented Dec 20, 2018

@GrahamcOfBorg build python2Packages.jupyter python3Packages.jupyter

@GrahamcOfBorg GrahamcOfBorg added the 8.has: clean-up This PR removes packages or removes other cruft label Dec 20, 2018
@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch from 80bb03a to 3c749ac Compare December 20, 2018 22:03
@veprbl
Copy link
Member Author

veprbl commented Dec 20, 2018

@GrahamcOfBorg build python2Packages.jupyter python3Packages.jupyter

veprbl referenced this pull request Dec 21, 2018
* python36Packages.qtconsole: 4.4.2 -> 4.4.3 (#50590)

Semi-automatic update generated by
https://github.com/ryantm/nixpkgs-update tools. This update was made
based on information from
https://repology.org/metapackage/python3.6-qtconsole/versions

* pythonPackages.qtconsole: linux platforms only
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Looks good to me. Do add the comment.

Copy link
Member

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 here explaining that this one should not be used by any Python libraries (so pkgs/development/python-modules/*) to avoid potential collisions.

Copy link
Member Author

Choose a reason for hiding this comment

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

There can still be a collision on application level. Should I we move pyqt5_with_qtwebkit uses from propagatedBuildInputs to wrappers modifying PYTHONPATH? Or maybe use python.withPackages?

@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch 3 times, most recently from 6571ce9 to ebe5104 Compare December 26, 2018 16:46
Since about 5 hydra evaluations ago the build log has:

substituteStream(): WARNING: pattern 'install_dir=pydbusmoddir' doesn't match anything in file 'configure.py'
substituteStream(): WARNING: pattern 'ModuleMetadata(qmake_QT=['webkitwidgets'])' doesn't match anything in file 'configure.py'

Looking at the original configure.py I don't see any mention of
pydbusmoddir and ModuleMetadata seems to be set like the patch suggests:

    'QtWebKitWidgets':      ModuleMetadata(
                                    qmake_QT=['webkitwidgets',
                                            'printsupport']),

It appears that we don't need the fix anymore.

Reverts: d3ed0ab ('PyQt: fix build')
Fixes: d7ef9a7 ('python36Packages.qtconsole: 4.4.2 -> 4.4.3')
From the qutebrowser README:

  "support for QtWebKit will be dropped soon"
@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch from 61bffea to 8df467e Compare December 27, 2018 14:14
@FRidh
Copy link
Member

FRidh commented Dec 27, 2018

Having two different version used by libraries is going to cause trouble. Looking at the changeset, they're the webkit one is only used in applications. It may still cause a collision there, but that is easily solvable by overriding the package set.

@FRidh FRidh self-assigned this Dec 27, 2018
@veprbl
Copy link
Member Author

veprbl commented Dec 28, 2018

It probably should be fine to just use a wrapProgram --prefix PYTHONPATH : on applications using pyqt5_with_webkit instead of relying on propagatedBuildInputs. In this case there is no harm if some pyqt5 app instead loads pyqt5_with_webkit by some chance.

@FRidh Or you know a better way?

@FRidh
Copy link
Member

FRidh commented Dec 28, 2018

Introduce a pyqt5_without_qtwebkit attribute and set pyqt5 = pyqt5_without_qtwebkit;. Applications that require qtwebkit, but have dependencies that use pyqt5 without qtwebkit, can then override the Python package set.

Did you build all applications you've listed?


Actually, because there are no libraries depending on the qtwebkit version, that won't be necessary.

@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch from c32138b to 243be7e Compare December 30, 2018 04:19
@veprbl
Copy link
Member Author

veprbl commented Dec 30, 2018

@FRidh
Thanks for the tip with pyqt5_without_qtwebkit, that should help to avoid infinite recursion.

The problem I was talking about was when we do something like:

nix-shell -p qutebrowser -p frescobaldi --run frescobaldi

Then frescobaldi will fail to import QtWebKit because "setupHooks" will override PYTHONPATH with the wrong version of pyqt5. It should not be a problem when using nix-env though. I made an example workaround for frescobaldi, see my WIP commit. What do you think?

@FRidh
Copy link
Member

FRidh commented Dec 30, 2018

@veprbl using nix-shell like that isn't "supported" anyway, so that's no problem.

@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch from 243be7e to 16f11df Compare December 30, 2018 14:36
@veprbl
Copy link
Member Author

veprbl commented Dec 30, 2018

Then this should be ready.

@FRidh
Copy link
Member

FRidh commented Dec 30, 2018

Note git-cola does not require Webkit. It only uses it to show shortcuts, and when the module is missing, it simply uses the default browser.

Looks good to me. I would like to remove the usage of the qt5webkit asap, e.g. by passing the non-webkit version forcing maintainers to fix their packages.

QtWebKit is only used if QtWebEngine is not available
@veprbl veprbl force-pushed the pr/pyqt5_opt_qtwebkit branch from 149c752 to eecc336 Compare December 30, 2018 15:23
@FRidh FRidh merged commit 26e5c0f into NixOS:master Dec 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants