Skip to content

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
@goodevile
Copy link

my POV:

Windows is PAIN.

There should be documentation before implementing this, it's more like a hack than a functionality, why not just add a documentation for Windows users like:

"if you are a windows user, we have this script that automates this X process, you can read the script and review it , we are not responsible for any unexpected behavior."

I think that there are more important stuff than creating scripts for automating Windows stuff.

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.

6 participants