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

Update PyQt to 5.15 #107

Merged
merged 21 commits into from
May 11, 2022
Merged

Update PyQt to 5.15 #107

merged 21 commits into from
May 11, 2022

Conversation

andfoy
Copy link
Contributor

@andfoy andfoy commented Mar 14, 2022

Checklist

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

@conda-forge-linter
Copy link

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
Copy link
Contributor Author

andfoy commented Mar 14, 2022

@conda-forge-admin, please re-render

@andfoy
Copy link
Contributor Author

andfoy commented Mar 14, 2022

@conda-forge-admin, please re-render

conda-forge-webservices[bot] and others added 2 commits March 14, 2022 17:46
@andfoy
Copy link
Contributor Author

andfoy commented Mar 14, 2022

@conda-forge-admin, please re-render

@andfoy
Copy link
Contributor Author

andfoy commented Mar 15, 2022

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

Copy link
Contributor

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this @andfoy!

I left some minor questions/suggestions of form, otherwise looks good to me.

recipe/charts_bld.bat Outdated Show resolved Hide resolved
recipe/meta.yaml Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@andfoy
Copy link
Contributor Author

andfoy commented Mar 28, 2022

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

@ccordoba12
Copy link
Contributor

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
Copy link
Member

isuruf commented Mar 29, 2022

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.

@duncanmmacleod
Copy link

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

@ccordoba12
Copy link
Contributor

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:

conda-forge/qt-webengine-feedstock#6

@hmaarrfk
Copy link
Contributor

hmaarrfk commented May 9, 2022

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

@ccordoba12
Copy link
Contributor

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
Copy link
Contributor

hmaarrfk commented May 9, 2022

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
Copy link
Contributor

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

@hmaarrfk
Copy link
Contributor

hmaarrfk commented May 9, 2022

Yes that way it will be forward compatible

@ccordoba12
Copy link
Contributor

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

Copy link
Contributor

@hmaarrfk hmaarrfk left a comment

Choose a reason for hiding this comment

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

i think you can pin runtime a little looser

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Contributor

ccordoba12 commented May 9, 2022

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
Copy link
Contributor Author

andfoy commented May 9, 2022

I think this should suffice for this case:

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

@ccordoba12
Copy link
Contributor

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

@ccordoba12
Copy link
Contributor

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

@ccordoba12
Copy link
Contributor

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
Copy link
Contributor

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:

  • If you compile with qt-main 5.15.2, pin to 5.15.* then this system will be compatible with ICU69 and ICU70. In the test time, it will pull in icu70. That is OK.
  • If you compile with qt-main 5.15.3, pin to 5.15.* then this system will only be compatible with ICU70.
  • If you compile with qt-main 5.15.2 and pin to 5.15.2, then you will only be compatible with icu69.

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

@ccordoba12
Copy link
Contributor

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
Copy link
Contributor

Thank you for helping so much!

@ccordoba12
Copy link
Contributor

ccordoba12 commented May 10, 2022

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

@ccordoba12
Copy link
Contributor

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!

@ccordoba12 ccordoba12 merged commit 38a1bd2 into conda-forge:main May 11, 2022
@andfoy andfoy deleted the pyqt_5.15 branch May 11, 2022 16:28
@andfoy
Copy link
Contributor Author

andfoy commented May 11, 2022

Thanks everyone for your help!

@SrNetoChan
Copy link
Contributor

Great work @andfoy and others. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants