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.

Sign up for GitHub

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

Jump to bottom

pyqt5: make qtwebkit optional, disable by default #51846

Merged
merged 10 commits into from Dec 30, 2018
Merged

pyqt5: make qtwebkit optional, disable by default #51846

merged 10 commits into from Dec 30, 2018

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Dec 10, 2018 •
edited

Motivation for this change

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

nixpkgs/pkgs/development/libraries/qt-5/5.11/default.nix

Lines 39 to 48 in 475d653

# 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
@veprbl veprbl mentioned this pull request Dec 10, 2018
Closed
@veprbl
Copy link
Member Author

veprbl commented Dec 10, 2018

@GrahamcOfBorg build python27Packages.pyqt5 python37Packages.pyqt5

@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
@veprbl
Copy link
Member Author

veprbl commented Dec 14, 2018

This is ready for review now

@veprbl
Copy link
Member Author

veprbl commented Dec 20, 2018

@GrahamcOfBorg build python2Packages.jupyter python3Packages.jupyter

@veprbl
Copy link
Member Author

veprbl commented Dec 20, 2018

@GrahamcOfBorg build python2Packages.jupyter python3Packages.jupyter

This was referenced Dec 21, 2018
Merged
Merged
veprbl referenced this pull request Dec 21, 2018
@r-ryantm @c0bw3b
python36Packages.qtconsole: 4.4.2 -> 4.4.3
d7ef9a7 
* 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
FRidh
FRidh requested changes Dec 26, 2018
View reviewed changes
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.

pkgs/top-level/python-packages.nix
@@ -612,6 +612,7 @@ in {
pyqt5 = pkgs.libsForQt5.callPackage ../development/python-modules/pyqt/5.x.nix {
pythonPackages = self;
};
pyqt5_with_qtwebkit = self.pyqt5.override { withWebKit = true; };
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
@veprbl
pyqt5: remove old fixes
9b07d48 
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')
@veprbl
pyqt5: make qtwebkit optional, disable by default
facefec 
qtwebkit appears to be unsupported in Qt 5.11. We are using some old port
https://github.com/NixOS/nixpkgs/blob/475d653afdbd8fe3e00ccfd22a30014b0df7aeaa/pkgs/development/libraries/qt-5/5.11/default.nix#L39-L48
and it is broken on darwin.
@veprbl
pyqt5: refactor
3c19829
@veprbl
treewide: use pythonPackages.pyqt5_with_qtwebkit where QtWebKit is ne…
20d60c8 
…eded
@veprbl
pythonPackages.qtconsole: build on darwin
97ec7a9 
Fixes: d7ef9a7 ('python36Packages.qtconsole: 4.4.2 -> 4.4.3')
@veprbl
pyqt5_with_qtwebkit: add warning comment
07a6736
@veprbl
qutebrowser: drop QtWebKit support
8df467e 
From the qutebrowser README:

  "support for QtWebKit will be dropped soon"
@FRidh
Copy link
Member

FRidh commented Dec 27, 2018 •
edited

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 •
edited

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 •
edited

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
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
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.

@veprbl
git-cola: doesn't need QtWebKit
eecc336 
QtWebKit is only used if QtWebEngine is not available
@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
Reviewers

@orivej orivej orivej approved these changes

@FRidh FRidh FRidh approved these changes

Assignees

@FRidh FRidh

Labels
6.topic: python 8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@veprbl @FRidh @orivej @GrahamcOfBorg