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

Draft: Fix pywin32 dep, widen range to >=227,<400 #2857

Closed
wants to merge 1 commit into from

Conversation

jmahlik
Copy link

@jmahlik jmahlik commented Jun 10, 2021

  1. I'm not sure what the testing process is supposed to look like for windows. Is there ci in place for it? Did the best I could.
    • Ran the test suite on python==3.9.0,&python==3.7.0 pywin32==301, dockerengine==20.10.7 via docker desktop w/ wsl backend using py.test ./tests/
    • 2 below failed, they seem related to not using the makefile to test (the makefile runs everything on linux containers so that didn't seem right)
    • Ran py37 via tox, should more python interpreters/version combos of pywin32 be added here maybe? Seems like a troublesome dependency that could benefit from combinations. Maybe a separate testenv for it that only triggers for windows platforms?
    • Flake8 was failing, I can fix it if desired, trying not to touch a bunch of files.
  2. Is widening the range acceptable? pywin32 seems to stick to major version bumps for breaking changes changelog. There's a bunch of issues related to pywin32 versions, about half seem related to version pinning/various versions of python.

Failures

tests\unit\utils_config_test.py:27: in test_find_config_fallback
tests\unit\utils_test.py:141: in test_kwargs_from_env_no_cert_path

closes #2835

@jmahlik jmahlik changed the title Draft: Fix pywin32 dep, widen range to >=227,<400 Fix pywin32 dep, widen range to >=227,<400 Jun 10, 2021
@jmahlik
Copy link
Author

jmahlik commented Jun 13, 2021

Added a possible tox config that will test this across all supported pythons and a range of pywin32's depending on platform. This should give confidence as to what pywin32's can be supported based on python version.

It does take a while to run initially because there are many venvs. Using tox -p all -r causes 100% cpu usage/probable race condition on my machine, so use tox -r only.

Thinking it would be best to run the integration tests in the windows env as well?

@chickenkiller
Copy link

hello, I would really appreciate that this dependency could be bumped up! It is blocking usage of docker-py on environments such as MSYS2... Thanks in advance

@jmahlik
Copy link
Author

jmahlik commented Jun 14, 2021

hello, I would really appreciate that this dependency could be bumped up! It is blocking usage of docker-py on environments such as MSYS2... Thanks in advance

It's seems like quite a rough dep to test. The unit tests did pass for all the version combos besides for a test involving tls (digging in to that more).

I haven't had a chance to run the integrations against all combos of python/pywin32 yet. It's almost as if one needs to run it in cmd and msys2.

@tadeu
Copy link

tadeu commented Aug 13, 2021

Gentle ping on this :)

@jmahlik
Copy link
Author

jmahlik commented Aug 13, 2021

Gentle ping on this :)

If some folks would like to clone and try to debug the tls issues, I could use some help in validating which versions are compatible. It takes a long time to run against all these versions on my machine.

There are failing integration tests on windows in the current test suite with the current deps on python 3.8, so trying to debug through those as well. Some might be docker/env related.

Signed-off-by: Justin Mahlik <j38999128+jmahlik@users.noreply.github.com>
@jmahlik jmahlik changed the title Fix pywin32 dep, widen range to >=227,<400 Draft: Fix pywin32 dep, widen range to >=227,<400 Aug 14, 2021
@jmahlik jmahlik marked this pull request as draft August 14, 2021 17:42
@stan-sz
Copy link

stan-sz commented Oct 21, 2021

Related to #2903

@jackwhelpton
Copy link

Any progress on this? If there's anything I can do to help out let me know: new to this codebase though.

@milas
Copy link
Member

milas commented Jul 26, 2022

Hi! Thanks so much for your PR and apologies for the delay in review. A fix for this has been merged and we're planning to issue a new release containing it soon. For context, changes similar to yours were done in #3004 to address some CI changes in the repo, which blocked merging of your PR as-is, and given the delay on this, we wanted to be respectful of our contributor's time and not require you to rebase + re-review.

@milas milas closed this Jul 26, 2022
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.

Update pywin32
6 participants