conda-forge / pyqt-feedstock

A conda-smithy repository for pyqt.
BSD 3-Clause "New" or "Revised" License
5 stars 35 forks source link

Update PyQt to 5.15 #107

Closed andfoy closed 2 years ago

andfoy commented 2 years ago

Checklist

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.

andfoy commented 2 years ago

@conda-forge-admin, please re-render

andfoy commented 2 years ago

@conda-forge-admin, please re-render

andfoy commented 2 years ago

@conda-forge-admin, please re-render

andfoy commented 2 years ago

@conda-forge/pyqt this one is ready for review

andfoy commented 2 years ago

@ccordoba12, the packages have now the layout that we discussed previously, I don't know if you have further comments

ccordoba12 commented 2 years ago

One last question for @isuruf: how should packages that depend on pyqtwebengine handle this transition for them to be able to work with PyQt 5.12 and 5.15?

In other words: those packages will need to add the dependency on pyqtwebengine explicitly for them to work with both versions (right now they only need to depend on pyqt). But this could catch feedstock maintainers off guard if they have not followed this process. So how do you think we should let them about it?

isuruf commented 2 years ago

Good question. I guess there's no easy way to avoid it. Maybe we should add a pyqtwebengine empty meta package for 5.12 so that maintainers can opt in early.

andfoy commented 2 years ago

Good question. I guess there's no easy way to avoid it. Maybe we should add a pyqtwebengine empty meta package for 5.12 so that maintainers can opt in early.

Should I send a separate PR updating the already existing recipe to include pyqtwebengine as an empty package? Although, the pyqtwebengine package already exists: https://anaconda.org/conda-forge/pyqtwebengine

ccordoba12 commented 2 years ago

I guess there's no easy way to avoid it.

Perhaps a blog post? And then tweet about it?

Although, the pyqtwebengine package already exists: https://anaconda.org/conda-forge/pyqtwebengine

Yeah, there is. And it contains the bindings for qtwebengine. So maintainers will only need to add that dependency explicitly to their recipes (even if it doesn't add anything new for 5.12).

ccordoba12 commented 2 years ago

I'll let @isuruf give the final word and merge here (I have nothing else to add).

Tobias-Fischer commented 2 years ago

Many thanks for the hard work @andfoy! Looking forward to seeing this merged - and even more to the migration to 5.15 :)

andfoy commented 2 years ago

@isuruf @ccordoba12 @h-vetinari Do you have any other comments?

ccordoba12 commented 2 years ago

None from my side. Also, thanks @h-vetinari for your review! Nice to see the community taking part of this effort.

@isuruf, are we ready to merge?

andfoy commented 2 years ago

@isuruf @ccordoba12 @h-vetinari do you have any other comments?

h-vetinari commented 2 years ago

@isuruf @ccordoba12 @h-vetinari do you have any other comments?

I think we're mainly waiting for @isuruf to have a look (who is in high demand for reviews and much other work throughout cf, so it can take a while).

isuruf commented 2 years ago

We need to make an announcement that we'll be dropping pyqtwebengine and pyqtcharts from the main pyqt package to match upstream PyPI package and to add them as dependencies (regardless of the pyqt version) if a package needs them.

isuruf commented 2 years ago

Otherwise looks good to merge

h-vetinari commented 2 years ago

We need to make an announcement

Blogpost on conda-forge.github.io, or where did you have in mind?

Would be cool if we could add that message to the 5.15 migrator itself, but I don't think that's implemented yet? (I'm only aware of the "special" migrators having custom pr bodies, not the ones in the pinning feedstock)

isuruf commented 2 years ago

Announcements page https://github.com/conda-forge/conda-forge.github.io/blob/main/src/user/announcements.rst

isuruf commented 2 years ago

Yes, a migrator for qt 5.15 would be good to have so that pyqt=5.15 can be used in conjunction with other packages.

h-vetinari commented 2 years ago

Announcements page https://github.com/conda-forge/conda-forge.github.io/blob/main/src/user/announcements.rst

TIL... 🙃

@andfoy, probably'd be best if you prepare a PR to https://github.com/conda-forge/conda-forge.github.io for the announcement?

andfoy commented 2 years ago

Thanks for all your help reviewing all the packages throughout these months! It's great to see that the final step is getting merged!

Announcements page

Regarding the announcements page, should I leave today's date as the one on the announcement, or will the announcement be published once this is merged?

h-vetinari commented 2 years ago

Regarding the announcements page, should I leave today's date as the one on the announcement

The exact date is probably not hyper-relevant, but I'd add e.g. tomorrows date and open a PR with that. The order of merging can then still be decided, but I guess the idea is to keep it close-to-concurrent.

andfoy commented 2 years ago

The announcement PR is now open: https://github.com/conda-forge/conda-forge.github.io/pull/1734

h-vetinari commented 2 years ago

Alrighty, announcement has been merged. This should be good now? @conda-forge/pyqt @isuruf

duncanmmacleod commented 2 years ago

@conda-forge/pyqt, can this be merged?

ccordoba12 commented 2 years ago

I think qt-main and qt-webengine are incompatible due to them being compiled against different versions of ICU.

That's being worked on by @hmaarrfk and @Tobias-Fischer right now:

https://github.com/conda-forge/qt-webengine-feedstock/pull/6

hmaarrfk commented 2 years ago

I think you can install 5.15.2 and icu69. Should be fine for many people.

ccordoba12 commented 2 years ago

Mmmh, I don't know if that would work. I mean, when running

mamba create -n foo -c conda-forge qt-webengine

I get

icu                                70.1  h27087fc_0          conda-forge/linux-64       14 MB
...
qt-main                          5.15.3  hf97cb25_0          conda-forge/linux-64       62 MB
qt-webengine                     5.15.4  hefd91c5_2          conda-forge/linux-64       56 MB
hmaarrfk commented 2 years ago

Yeah.... We should probably pull build 2 of qt we engine since it wasn't actually built with icu70 support. It used it's vendored icu library (if it used icu at all)

ccordoba12 commented 2 years ago

Sorry, I guess you're saying that we should pin this package to qt-main 5.15.2 while PR https://github.com/conda-forge/qt-webengine-feedstock/pull/6 is sorted out, right?

hmaarrfk commented 2 years ago

Yes that way it will be forward compatible

ccordoba12 commented 2 years ago

Ok, could you look at commit ec85b81 to see if that's exactly what you have in mind?

ccordoba12 commented 2 years ago

i think you can pin runtime a little looser

Ok, but I don't understand how that would pull 5.15.2 and not 5.15.4.

Also, are not your suggestions equivalent to?

- {{ pin_compatible("qt-main", max_pin="x.x") }}

That's what it was before my commit.

andfoy commented 2 years ago

I think this should suffice for this case:

- {{ pin_compatible("qt-main", min_pin="x.x", max_pin="x") }}
ccordoba12 commented 2 years ago

Thanks @andfoy for chiming in! I added your suggestion in my last commit.

ccordoba12 commented 2 years ago

Ok, @andfoy suggestion didn't work because it's pulling qt-main 5.15.3. Trying @hmaarrfk's suggestion now.

ccordoba12 commented 2 years ago

Unfortunately, that also pulls qt-main 5.15.3, so going back to my original pin, which was successful yesterday. Since a lot of people is waiting for this, I think we should merge with a hard pin on qt-main 5.15.2 for now.

@hmaarrfk, do you agree?

hmaarrfk commented 2 years ago

I guess the thing is that the icu migration is half way through https://conda-forge.org/status/#icu70 and I would like to be able to use this package in ICU69 and ICU70.

I guess our current understanding is:

icu qt-main qt-webenigne
69 5.15.2 5.15.4 build 0 or 1
70 5.15.3 5.15.4 build 2 -- was pulled for OSX

But in truth, qt-webengine was never built with system icu support. We are working through this at the moment..

So you have 3 options:

Therefore the most compatible option, is in my mind, to build WITH 5.15.2 and pin to 5.15

ccordoba12 commented 2 years ago

Therefore the most compatible option, is in my mind, to build WITH 5.15.2 and pin to 5.15

Ok, thanks for explaining things in detail and sorry for the misunderstanding. Trying that option then.

hmaarrfk commented 2 years ago

Thank you for helping so much!

ccordoba12 commented 2 years ago

Ok, everything is green now, so I'll merge tomorrow unless I hear any objection about it.

ccordoba12 commented 2 years ago

Ok, the time to merge has come!

Thanks @andfoy for all your hard work on this!! This is a terrific improvement for all projects that use PyQt in conda-forge.

Thanks also to @isuruf, @hmaarrfk, @h-vetinari and all others that reviewed this PR and helped @andfoy to finish it!

andfoy commented 2 years ago

Thanks everyone for your help!

SrNetoChan commented 2 years ago

Great work @andfoy and others. Thanks!