-
Notifications
You must be signed in to change notification settings - Fork 432
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 Windows build and lint #685
Conversation
This looks like an undeclared dependency in `remoteproess`?
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 for the PR! The changes look good to me,
I'd like to get the windows CI working before merging - I realized I might have messed up the how python is installed in the last PR (moved setup-python to the wrong location in the GHA yaml), and am also testing out a tweak to fix in this other PR |
`console` requires an older version of `windows-sys` still
Yeah agree. I'm pulling updates from my fork to see if one of those fixes the issue — I don't know what else I would have changed to make CI pass. |
It looks like you might have changed from Python 3.9 -> 3.11? |
I'm sorta wondering if this is related to the python version being tested - I'm getting a different error with python 3.10 than with python 3.11 in CI, and I'm wondering if the working versions are with python 3.9. Because of how I messed up the setup python install, its possible that the 'fix clippy' pr we merged tested with python 3.9 (since the install for python 3.11 happened afterwards). |
yeah, I'm testing now on #642 with 3.9 to see. The error with 3.10 was |
Yeah 1d0ffc1 resolves the error. If you want, I can separate that commit from this pull request. Otherwise, it seems fine to merge this as a whole. |
I'm good merging this as a whole - thanks for fixing. It does like there is an issue on windows with python 3.10+ that needs fixing =( - but that doesn't need to be fixed in this PR. |
@benfred regarding #679 (comment)
Includes a few other changes as we debugged the cause of the failure.