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

Make winreg import compatible to modern Python #7500

Closed
wants to merge 3 commits into from

Conversation

uranusjr
Copy link
Member

Fix #7495.

I intend to work on #6040 to eliminate the internal copy next, but figured it’s better to post this quick fix first.

@xavfernandez
Copy link
Member

A bugfix newsfile is needed and ideally a test also :)

@xavfernandez xavfernandez added this to the 20.0 milestone Dec 19, 2019
@uranusjr
Copy link
Member Author

I am without a computer so the news is the best I can do now :(

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

This is pretty much verbatim what is in the upstream version (except PY2 vs PY3), and we're trying to get rid of the bundled version anyway, so I'm personally OK with no tests in this case.

@uranusjr
Copy link
Member Author

I thought about it a bit but am not sure how to meaningfully test this :/

@xavfernandez
Copy link
Member

I thought about it a bit but am not sure how to meaningfully test this :/

Simply calling site_config_dirs() with PY2/3 & with/without ctypes ?

@uranusjr
Copy link
Member Author

uranusjr commented Dec 20, 2019

with/without ctypes

Is there a way to make import ctypes unavailable in the test? (without e.g. deleting the module from stdlib)

@xavfernandez
Copy link
Member

@uranusjr cf

monkeypatch.setitem(sys.modules, "ctypes", None)
for an example

monkeypatch.setattr(
appdirs,
"_get_win_folder_from_registry",
_get_win_folder_from_registry,
Copy link
Member

Choose a reason for hiding this comment

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

I was actually hoping that winreg.OpenKey and winreg.QueryValueEx could be called for real on Windows. Is there a blocker ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not blocker but I don’t think it’s meaningful. Their return values might not be predictable (being essentially system configuration).

@uranusjr
Copy link
Member Author

Closing since #7501 is now in a mergeable state

@uranusjr uranusjr closed this Dec 24, 2019
@uranusjr uranusjr deleted the internal-appdirs-winreg branch January 1, 2020 14:06
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 31, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation OS: windows Windows specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Winreg import failure in appdirs
4 participants