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

Fix python tests #1456

Merged
merged 7 commits into from
Nov 8, 2022
Merged

Fix python tests #1456

merged 7 commits into from
Nov 8, 2022

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Nov 3, 2022

Fixes #1455.

@@ -33,7 +33,6 @@ jobs:
uses: actions/setup-python@v4
with:
python-version: '3.10'
if: ${{ runner.os == 'Windows' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Christian might know this already, but just in case: This line (deleted in this PR) was added by me specifically to make the macOS tests pass. The GH Actions runners have Python pre-installed, so the existence of the extra Python seemed to confuse CMake. The test failures we are seeing know are probably the same as what blocked me for more than a month.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are probably right here. The strange thing is that cmake does not even use the version installed by the github action (3.10), but uses some other version that it finds (3.8). I don't know how to resolve this... The default Python version in the MacOS runner is now 3.11 which our Python target is not compatible to. I will continue trial and error and see where it gets me...

Peter, you know the Python target better than me: What do you think how much effort it would be to actually fix the errors that we see popping up in CI when Python 3.11 is used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just noticed that I made a mistake and excluded version 3.10, which explains why cmake selected version 3.8 (which seems to be preinstalled as well)

@cmnrd
Copy link
Collaborator Author

cmnrd commented Nov 4, 2022

It looks like it works fine when we specify a range of possible Python versions in the cmake script. @petervdonovan what do you think about this solution? Should we merge it?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Nov 4, 2022

The LSP tests on Mac OS consistently fail. I tried 3 times. Run 1 and 3 have the same error, but in run 2 another error is reported. I am not sure if there is an actual problem lurking here, or if I just had bad luck with the LSP tests.

@petervdonovan
Copy link
Collaborator

petervdonovan commented Nov 4, 2022

Yes, I think it's a real bug. Probably when we changed Python versions, the error messages coming out of Python changed slightly and failed to parse.

Co-authored-by: Peter Donovan <[email protected]>
@petervdonovan
Copy link
Collaborator

petervdonovan commented Nov 7, 2022

Not only is the LSP tests problem apparently platform-specific, but I am having trouble reproducing it even by running CI from a branch that is based on this branch. I'm worried that the two issues blocking this and #1457 will be difficult to find.

Never mind, it was just something silly -- pylint (the Python linter) is being installed on macOS, but for some reason the Java process was not able to find it (and it was apparently not on the PATH).

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@lhstrh lhstrh merged commit a5273af into master Nov 8, 2022
@lhstrh lhstrh added the ci Continuous integration label Nov 8, 2022
@cmnrd cmnrd deleted the fix-python-tests branch November 8, 2022 07:49
@cmnrd cmnrd restored the fix-python-tests branch November 8, 2022 07:51
@cmnrd cmnrd deleted the fix-python-tests branch March 10, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python test failures
3 participants