-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
PR: Unify and cleanup CI infra, improve robustness and test Python 3.9 and PyQt6 #262
Conversation
7cb7db5
to
10ac3d5
Compare
8035991
to
5189293
Compare
5189293
to
340d4be
Compare
d947d12
to
afaa3b3
Compare
Ready for final review @dalthviz The only skips I needed to add were Similarly, the only platforms PyQt6 is skipped on is Python 3.6 non-conda on macOS and Linux, as wheels for PyQt6 6.2 are apparently not (currently) available there. Other than that, I substantially increased platform coverage, e.g. added coverage for all platforms and Python versions for the conda Pyside2 packages and Pyside2 5.13, added tests of PyQt 5.15 and PyQt + Windows + Py3.8/3.9, and filled in gaps in patch version coverage, plus other issues. Finally, more broadly, it is now much easier to add new Python, Qt and PyQt/PySide versions, and platforms, conda use and more to the matrix without having to duplicate workflows or test scripts, as well as gain precise control over whether and the versions of each tested for each combination, all in one place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CAM-Gerlach for all the work! I left some comments regarding QtWebEngine
(which maybe will allow us to test it with PyQt6
), the matrix values for Python and the segfaults for the uic module
afaa3b3
to
bdc359f
Compare
Also, @CAM-Gerlach I saw this PR was done in a branch from the actual repo and not from your fork. Was that necessary for some reason? Maybe due the changes in the Github Actions workflow? |
Yes, that's it; normally I never create branches on upstreams except for this reason. I'm not sure if its changed, but at least previously (apparently for security reasons) GitHub actions workflows would use the upstream repo's workflow file, not the fork's file, or not trigger at all, and I had to do the same thing on previous PRs adding/making changes to workflows (e.g. PR #208 ) or Actions wouldn't always work. Has that been your experience, or is that not necessary anymore? It would be nice to not have to do that anymore. |
I think we changed the workflow file for dropping Python 2 drop but since that only removed testing instances not sure. Although, to add PySide6 testing, I think that was done from a branch fork so maybe is not needed anymore but no idea for sure to be honest |
Yeah, I was thinking of the PySide6 PR as well, as it looks like that did work fine. It seems the problem, assuming it still exists, is likely only limited to creating new workflow as opposed to making changes to an existing one, so for most workflow-related things an upstream branch shouldn't be needed in the future, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CAM-Gerlach!
Should the branch be deleted @CAM-Gerlach ? Edit: Also, checking seems like now our coverage is set to Could you check that please @CAM-Gerlach ? |
Yup, thanks! Done.
Huh, I had previously verified that the coverage reports and the coveralls output all looked good (after some fixes to get things working), and I can confirm they still do in the latest build, so I'm not sure why coveralls isn't seeing the coverage other than it not handling coverage from the installed package (vs. the local source) properly. I opened #267 and am getting on this right away. |
Try using native Python pip with venv for non-conda builds?Maybe laterTry Python 3.10, fix any issues & add tagsTest deps on conda not yet officially compatibleResolves #253
Part of #233