build qtwebkit bindings

izahn commented 2 years ago

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[x] Reset the build number to 0 (if the version changed)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

Upstream PyQtWebKit is no longer separate from PyQt5. This PR adds qtwebkit to the dependencies here so webkit bindings will be included in pyqt, mirroring changes in upstream development and distribution.

Once this is done we can consider arching pyqtwebkit as proposed and discussed in https://github.com/conda-forge/pyqtwebkit-feedstock/issues/7

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

izahn commented 2 years ago

@conda-forge-admin please rerender

github-actions[bot] commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pyqt-feedstock/actions/runs/2490262761.

ccordoba12 commented 2 years ago

I'm a bit -1 on this because the purpose of splitting PyQt on pyqt and pyqtwebengine is to avoid including a web widgets module as part of pyqt, given that many projects don't need it (e.g. Matplotlib).

So I think you should create a new package (built as part of this recipe) called pyqtwebkit instead.

izahn commented 2 years ago

I'm a bit -1 on this because the purpose of splitting PyQt on pyqt and pyqtwebengine is to avoid including a web widgets module as part of pyqt, given that many projects don't need it (e.g. Matplotlib).

So I think you should create a new package (built as part of this recipe) called pyqtwebkit instead.

That sounds like a lot of work for very little gain. All the other outputs here follow upstream, why not do the same here? We're talking about 4 files totalling 701.2 KiB here, hardly worth splitting.

izahn commented 2 years ago

But yeah I see the advantage for matplotlib. Okay I'll see about splitting it.

ccordoba12 commented 2 years ago

We're talking about 4 files totalling 701.2 KiB here, hardly worth splitting

I guess you're talking about the binding files. But what would really add more weight to the pyqt package for others is the qtwebkit direct dependency you're introducing here, which is ~15Mb in size.

izahn commented 2 years ago

We're talking about 4 files totalling 701.2 KiB here, hardly worth splitting

I guess you're talking about the binding files. But what would really add more weight to the pyqt package for others is the qtwebkit direct dependency you're introducing here, which is ~15Mb in size.

Makes sense, OK, will split it. Thanks!

izahn commented 2 years ago

This is currently a bit funky because I had trouble getting a stand-alone pyqtwebkit built. Instead I built pyqt with qtwebkit included and used ignore_run_exports to allow it to be installed without qtwebkit. A pyqtwebkit meta-package output is also included with depends on pyqt and qtwebkit. All this should work, but it is probably unconventional.

izahn commented 2 years ago

Any thoughts about this @conda-forge/pyqt ?

ccordoba12 commented 2 years ago

Instead I built pyqt with qtwebkit included and used ignore_run_exports to allow it to be installed without qtwebkit

This means that the Webkit bindings would be part of the pyqt package but this one wouldn't depend on qtwebkit?

andfoy commented 2 years ago

I think @izahn is having problems w.r.t the versioning of the packages, similarly to the situation when qt-main and qtwebengine had different versions

andfoy commented 2 years ago

I initially don't agree with having qtwebkit included as part of pyqt since it wouldn't be consistent with the package on PyPi

izahn commented 2 years ago

This means that the Webkit bindings would be part of the pyqt package but this one wouldn't depend on qtwebkit?

Yes, exactly

izahn commented 2 years ago

I initially don't agree with having qtwebkit included as part of pyqt since it wouldn't be consistent with the package on PyPi

That is interesting. For clarity we're talking about pyqtwebkit here, not qtwebkit. It looks like pyqtwebkit is no longer distributed on PyPi, since version 5.13.1 in 2019 (unless I missed something). Since conda-forge needs pyqtwebkit for qgis (among other things) it won't work for us to stop packaging pyqtwebkit to be consistent with PyPi.

izahn commented 2 years ago

I think @izahn is having problems w.r.t the versioning of the packages, similarly to the situation when qt-main and qtwebengine had different versions

Since pyqtwebkit source is included in pyqt source distribution the versions are the same. The problem I'm having is that I couldn't get this version to build just pyqtwebkit. https://github.com/conda-forge/pyqtwebkit-feedstock/blob/main/recipe/build.sh uses configure.py to disable everything but pyqtwebkit but I couldn't figure out an equivalent of that that works here.

izahn commented 2 years ago

What is the current thinking on this one? I'd like to get something worked out so we can migrate qgis to qt 5.15.

andfoy commented 2 years ago

According to https://www.riverbankcomputing.com/static/Docs/sip/pyproject_toml.html, you can enable pyqtwebkit using the option enable under [tool.sip.project] in pyproject.toml, which means that a patch is required here to just compile the package separate of the others. We're sorry about the extra work, but the rationale is to keep any extra features that are not properly part of pyqt or are deprecated outside the main package.

The diff should look like this:

--- pyproject.toml  2021-10-29 09:11:33.000000000 -0500
+++ pyproject.toml  2022-07-04 10:16:10.648316459 -0500
@@ -14,3 +14,6 @@
 license = "GPL v3"
 description-file = "README"

+[tool.sip.project]
+enable = ["QtWebKit", "QtWebKitWidgets"]
+

izahn commented 2 years ago

I'm on vacation, happy if someone else has time to take care of this, otherwise I'll look at it when I get back.

izahn commented 1 year ago

@conda-forge-admin please rerender

github-actions[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pyqt-feedstock/actions/runs/2686017674.

izahn commented 1 year ago

Hi folks, I'm having a lot of trouble pulling pyqtwebkit out into a separate package, and I'm not willing to spend any more time on it. If someone else wants to do it that would be great. Otherwise I recommend reverting to the meta-package approach that I started with, i.e., include pyqtwebkit files in the pyqt package but don't depend on qtwebkit, then output a pyqtwebkit meta-package that depends on both pyqt and qtwebkit. That approach was much simpler, works, and has no major drawbacks that I'm aware of.

The broader context for this PR is that we need to migrate pyqtwebkit to qt=5.15 so that we can proceed with migrating qgis. Since pyqtwebkit source is now included as part of pyqt I think it makes sense to follow upstream and package it here instead of continuing to maintain https://github.com/conda-forge/pyqtwebkit-feedstock/ separately.

ccordoba12 commented 1 year ago

I recommend reverting to the meta-package approach that I started with, i.e., include pyqtwebkit files in the pyqt package but don't depend on qtwebkit

The problem I see with this approach is that users would be able to import QtWebkit when only PyQt is installed, but they'd get an error because QtWebkit is not available. And that would reflect incorrect packaging on our side.

izahn commented 1 year ago

@conda-forge-admin please rerender

github-actions[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pyqt-feedstock/actions/runs/2706172390.

izahn commented 1 year ago

The problem I see with this approach is that users would be able to import QtWebkit when only PyQt is installed, but they'd get an error because QtWebkit is not available. And that would reflect incorrect packaging on our side.

In both cases users who wish to import PyQt5.QtWebKit should install pyqtwebkit package. With pyqtwebkit packaged separately attempting to import PyQt5.QtWebKit without installing pyqtwebkit will result in

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'PyQt5.QtWebKit

With my meta-package approach attempting to import PyQt5.QtWebKit without installing pyqtwebkit will result in

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: libQt5WebKit.so.5: cannot open shared object file: No such file or directory

In both cases the remedy is to install pyqtwebkit. My opinion is that the meta-package solution may be technically "incorrect", but it won't make any practical difference for users. This is why I preferred do describe it as "funky" rather than "incorrect" in https://github.com/conda-forge/pyqt-feedstock/pull/117#issuecomment-1157975984 . :-)

All that said, I'm delighted if someone else has time to do it correctly and pull pyqtwebkit out into a separate package. I don't have time for that at the moment, and won't anytime soon. If no one else has time either then I hope we can move forward with the meta package solution, even if it is incorrect in a technical sense.

ccordoba12 commented 1 year ago

With pyqtwebkit packaged separately attempting to import PyQt5.QtWebKit without installing pyqtwebkit will result in

Well, this makes sense to me because QtWebKit is not part of PyQt for now. I mean, it's equivalent to doing from PyQt import foo.

With my meta-package approach attempting to import PyQt5.QtWebKit without installing pyqtwebkit will result in

My point exactly, i.e. your approach makes possible to import QtWebKit but throws an error because the linked library (libQt5WebKit.so.5) is not present. And that clearly looks like a packaging error.

In both cases the remedy is to install pyqtwebkit.

Yeah, but in the second case users don't necessarily know about this, so it could be hard for them to understand why this is failing.

My opinion is that the meta-package solution may be technically "incorrect", but it won't make any practical difference for users.

I disagree but I'd like to ask @conda-forge/core what they think about this because I don't want to block QGis from supporting PyQt 5.15 due to this.

izahn commented 1 year ago

I just don't see much functional difference between

ModuleNotFoundError: No module named 'PyQt5.QtWebKit

and

ImportError: libQt5WebKit.so.5: cannot open shared object file: No such file or directory

In both cases you should install pyqtwebkit. If in the second case, if a user (reasonably) concludes that they need to install qtwebkit that will also work. I just don't see this being a likely source of confusion for users, though I continue to agree that it is technically incorrect to package it this way.

izahn commented 1 year ago

Yeah, but in the second case users don't necessarily know about this, so it could be hard for them to understand why this is failing.

As I argued above I think this is unlikely to cause confusion for users. However, I volunteer to monitor the issues in this feedstock and to respond to any issues about it.

ccordoba12 commented 1 year ago

I just don't see much functional difference

As I said, you'll get the same error if you do

>>> import PyQt5.foo
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'PyQt5.foo'

so I disagree.

My suggestion to solve this: what if for the pyqtwebkit package you compile the entire PyQt5 again along with the QtWebkit bindings, then remove all PyQt5 modules and only leave the files that belong to QtWebkit before creating the package?

I think in theory that should work and would provide the same package structure as pyqtwebengine.

izahn commented 1 year ago

As I said, you'll get the same error if you do

>>> import PyQt5.foo
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'PyQt5.foo'

so I disagree.

I'm not following this argument at all. Is ModuleNotFoundError: No module named 'PyQt5.QtWebKit the message you think is confusing? That is the status quo. To me it says "you need to install the package that provides the qtwebkit python module". The alternative of ImportError: libQt5WebKit.so.5: cannot open shared object file: No such file or directory says to me "you need to install the package that provides the qtwebkit shared library"; I don't think either message is particularly likely to be misunderstood by users.

My suggestion to solve this: what if for the pyqtwebkit package you compile the entire PyQt5 again along with the QtWebkit bindings, then remove all PyQt5 modules and only leave the files that belong to QtWebkit before creating the package?

This is a good solution and indeed I spent a lot of time in this PR trying to get it to work. You can see my efforts up until https://github.com/conda-forge/pyqt-feedstock/pull/117/commits/ca504b91d73a4422745df4c93fdced131e44ec6a where I decided to abandon it due to the difficulty. I will be delighted if someone else has time to get it working, but I'm not going to keep banging my head against that particular wall.

I think in theory that should work and would provide the same package structure as pyqtwebengine.

izahn commented 1 year ago

@conda-forge/pyqt anybody else want to take a shot at this? If not I recommend merging this as-is since it is an improvement over the status quo and will allow us to move forward with the feedstock migrations that depend on this.

izahn commented 1 year ago

@conda-forge-admin please rerender

izahn commented 1 year ago

Almost 2 months later I finally figured it out :-) Build pyqt without qtwebkit and then build it again as pyqtwebkit with qtwebkit in the host requirements then conda build will sort it out for us.

This no longer includes any pyqtwebkit stuff in the pyqt package, and is now ready for review @conda-forge/pyqt

bnavigator commented 1 year ago

I recommend you stop this before it is too late. QtWebKit is an abandoned fork of WebKit with lots of problems including unfixed security issues. You'r basically using a browser which has not been updated for several years.

There is a reason why it is not included the PyQt5 wheels on PyPI.

izahn commented 1 year ago

I recommend you stop this before it is too late. QtWebKit is an abandoned fork of WebKit with lots of problems including unfixed security issues. You'r basically using a browser which has not been updated for several years.

There is a reason why it is not included the PyQt5 wheels on PyPI.

I'm personally happy if pyqtwebkit and qtwebkit can be dropped. Note that this PR doesn't attempt to change the status quo in this regard though: pyqtwebkit is currently packaged in https://github.com/conda-forge/pyqtwebkit-feedstock , this PR just tries to move it here because the pyqtwebkit source code is included in the pyqt source releases.

bnavigator commented 1 year ago

because the pyqtwebkit source code is included in the pyqt source releases.

The PyQt5 "sources" are a wrapper for the Qt5 libraries. QtWebKit used to be the common webview until Qwebengine emerged in Qt 5.6. PyQt5.QtWebkit in the sources is a legacy from that time but has never been included into the wheels.

https://github.com/PyQt5/PyQtWebKit and https://pypi.org/project/PyQtWebKit/ is of questionable origin despite the official sounding names.

It can't be stressed enough that QtWebkit did not have any commit since 2020: https://github.com/qtwebkit/qtwebkit. Which is inherently bad for QGIS since they use(d) it in EXTERNAL content viewers.

izahn commented 1 year ago

I'm with you @bnavigator I'd also prefer to just drop both pyqtwebkit and qtwebkit from conda-forge. I'm not the one who needs to be convinced. Since qgis is currently the only thing that uses it we just need to convince the qgis-feedstock maintainers to drop the dependency.

izahn commented 1 year ago

@conda-forge/pyqt We have copied the recipe from this feedstock to https://github.com/conda-forge/pyqtwebkit-feedstock in https://github.com/conda-forge/pyqtwebkit-feedstock/pull/8 and are building pyqtwebkit there for now. I'm going to leave this PR open because it will be much better if we can get rid of this duplication and avoid having multiple feedstocks building from the same source code. I continue to urge you to merge this here, at which time we can archive https://github.com/conda-forge/pyqtwebkit-feedstock

conda-forge / pyqt-feedstock

build qtwebkit bindings #117