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

Support DNS Flush for Windows in Python #2529

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gfyoung
Copy link
Contributor

@gfyoung gfyoung commented Dec 15, 2023

Finally getting around to this. Title is self-explanatory. Ran locally to confirm it works.

With this feature now in Python, updateHostsWindows.bat becomes unnecessary.

@StevenBlack
Copy link
Owner

StevenBlack commented Dec 17, 2023

Thank you for this @gfyoung.

I feel this modification merits discussions beforehand.

Firstly, we would want to do this because... ?

Secondly, the required readme_template.md file modifications do not appear to be included in this PR.

Thirdly we're talking about Windows here, with its vast clusterf of version in userland, users who may or may not have admin privileges, how does doing this in Python help at all? In particular, if it fails, what recourse does the end user have? Run the whole updateHostsFile.py again? Where are the error messages to help prevent an avalanche of support requests from Windows userland?

At least the .bat file is a separate process with fairly clear recourse for the user. That's my gut feeling; I'm willing to be convinced otherwise.

@gfyoung
Copy link
Contributor Author

gfyoung commented Dec 17, 2023

@StevenBlack:

the required readme_template.md file modifications do not appear to be included in this PR

Good catch. I'll add those.

users who may or may not have admin privileges, how does doing this in Python help at all

The Python script has already incorporated admin-level Windows commands (see #1999) and relevant code here. This PR simply continues what was started.

The script prints out that admin privileges are required for flush DNS cache (see [here] (https://github.com/StevenBlack/hosts/blob/master/updateHostsFile.py#L1431-L1440)) and will also print an error message if it fails as part of the PR. If the user has no admin privileges, the script will still run. It just won't flush the DNS cache, which wouldn't be possible anyway because you don't have admin privileges.

The .bat file doesn't change that reality and isn't really a recourse to that case. In fact, it's unclear why Windows is the only one that needs this treatment vs. other platforms i.e., replace in your feedback "Windows" with Linux or Mac.

@StevenBlack
Copy link
Owner

Thank you for those thoughts @gfyoung.

Here's my perspective: well over 90% of support issues here (and in my personal email inbox) are Windows-related.

Quite frankly, Windows is a pain in the ass. I don't want to do anything that increases support loading from Windows' userland.

This PR strikes me as a solution in search of a problem. There's nothing to gain, but lots to lose.

I appreciate your "works on my machine certification" but there are much wider risks here and presently I'm inclined to decline this PR.

I'll think about it.

@gfyoung
Copy link
Contributor Author

gfyoung commented Dec 18, 2023

This PR strikes me as a solution in search of a problem. There's nothing to gain, but lots to lose.

Windows support is subordinate to other operating systems. The Python script handles everything for non-Windows systems. I shouldn't have to run both the Python script and .bat file for the same level of support - doing so actually increases the chances of something going wrong and triggering a support ticket/email. As it stands, you already merged #1999, which was arguably more risky than this change. I'm simply building off of it.

I appreciate your "works on my machine certification" but there are much wider risks here and presently I'm inclined to decline this PR.

Totally get there could be wider risks, but that's why this is a PR.

Others are welcome to verify themselves that this change works before this gets merged.

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 4, 2024

Actually this change does make sense as long as it still works the same, which I haven't tried since I don't use the hosts this way.

I'd say let's wait until more people on Windows try this and confirm it works the same as the previous solution and then merge it.

PS. I never used an elevated cmd to flush the DNS on Windows. Is it really needed?

@gfyoung
Copy link
Contributor Author

gfyoung commented Jan 4, 2024

PS. I never used an elevated cmd to flush the DNS on Windows. Is it really needed?

Good question: I do recall it being required at some point but definitely not needed in more recent versions of Windows. A good follow-up PR would be to remove the SUDO call if testing confirms that it's no longer needed.

Actually this change does make sense as long as it still works the same, which I haven't tried since I don't use the hosts this way.

@XhmikosR if you wouldn't mind giving it a try though 😄

@gfyoung gfyoung force-pushed the windows-flush-dns branch from 47c9e1d to 22b8b7f Compare January 9, 2024 05:11
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.

5 participants