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

PR: Define Qt/binding versions at top level, fix warnings if versions not found, and fix test dir on CIs #292

Merged
merged 2 commits into from
Dec 2, 2021

Conversation

CAM-Gerlach
Copy link
Member

@CAM-Gerlach CAM-Gerlach commented Dec 1, 2021

Followup to merging PR #289 to master following a very weird and unexpected check failure on Win 10 + PyQt 5.9 + conda + Python 3.6; see CI logs.

Seems like there's some weird behavior on this very specific CI run, where the tests pass, but when we run the command to save the path for the test coverage, which imports QtPy a second time in a new instance of the same interpreter, QT_VERSION is not defined, which must mean that all the if blocks either didn't trigger or had an ImportError and QtPy fell back to using PyQt5. I could understand if PyQt 5.9 didn't define those top-level constants (since the source repo is not public, I tried to check beforehand but couldn't), but the fact that it worked without error the first time for the tests leads me to believe that it must either have been something with setting the env variables that triggered the different behavior on the second, or there's something else I'm not accounting for going on.

EDIT: It was the latter, the second Python command was run without -I which resulted in shadowing due to the hack I used to avoid problems due to the non-src directory layout. See below for full details.

While this PR fixes the proximate error and ensures that the top-level constants are always at least defined, I want to see if I can reproduce this locally in the same env, as it may indicate an underlying issue. Done, see below.

So, this PR implements three levels of fixes to the related issues discovered here:

  • Checks that QT_VERSION is defined first before acting on it (like all the others)
  • Initializes the top-level version constants to None at the top of the file, which avoids them being undefined if no Qt API is found and also reduces duplication and verbosity
  • Fixes the shadowing issue in the tests that surfaced this behavior to begin with

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Dec 2, 2021

Okay, I figured out what's going on. Since QtPy doesn't use the recommended src directory layout, in the CI tests I had to cd into another directory, in this case qtpy, to avoid testing the local copy rather than the actual installed package (which not only would not detect any packaging problems, but also plays havoc with the coverage). However, this was a rather short-sighted fix, as this results in QtPy's sip module shadowing the sip package (and could result in other such tricky issues). This doesn't cause problems for the tests since I pass the -I flag to python in the tests command, which prevents Python from adding the working dir to the path. However, when the standalone command is run to import qtpy and get its path for the coverage, importing PyQt5 fails due to this and thus version is left undefined. I'll fix that as well here, to avoid any future knock-on effects from this, by just cding into a temporary directory instead.

@CAM-Gerlach CAM-Gerlach marked this pull request as ready for review December 2, 2021 00:45
@CAM-Gerlach
Copy link
Member Author

@dalthviz @ccordoba12 if either of you could look this over and merge this if there's nothing serious going on, so that tests are passing on master again, that would be great, thanks.

Copy link
Member

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

Looks good to me, thanks @CAM-Gerlach!

@CAM-Gerlach CAM-Gerlach changed the title PR: Ensure versions are defined before checking for outdated ones PR: Define Qt/binding versions at top level, fix warnings if versions not found, and fix test dir on CIs Dec 2, 2021
@CAM-Gerlach
Copy link
Member Author

Thanks @ccordoba12 !

@CAM-Gerlach CAM-Gerlach merged commit 43a8ca1 into spyder-ide:master Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants