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

Added an option to compress the created hosts file. #459

Merged
merged 4 commits into from Jan 2, 2018

Conversation

Projects
None yet
7 participants
@stefanopini

stefanopini commented Dec 30, 2017

In particular, the compression option removes non-necessary lines (empty lines and comments) and puts multiple domains in each line.
This option should solve the issue #411 regarding the DNS client service of Windows.
A test for this option is still missing, I hope someone can work on it.

Stefano
Added an option to compress the created host file.
In particular, the compression option removes non-necessary lines (empty lines and comments) and puts multiple domains in each line.
This option should solve the issue #411 regarding the DNS client service of Windows.
@welcome

This comment has been minimized.

Show comment
Hide comment
@welcome

welcome bot Dec 30, 2017

Thank you for submitting this pull request! We’ll get back to you as soon as we can!

welcome bot commented Dec 30, 2017

Thank you for submitting this pull request! We’ll get back to you as soon as we can!

@tbalden

This comment has been minimized.

Show comment
Hide comment
@tbalden

tbalden Dec 30, 2017

Excellent. How much smaller the file is in average?

tbalden commented Dec 30, 2017

Excellent. How much smaller the file is in average?

@stefanopini

This comment has been minimized.

Show comment
Hide comment
@stefanopini

stefanopini Dec 30, 2017

My hosts file, with all the extensions and some custom entries, passes from 1.47MB to 1.00MB, but the biggest change is in the number of lines: 5947 instead of 58143.
If it's useful, I can do some tests to see the average improvement on different scenarios.

stefanopini commented Dec 30, 2017

My hosts file, with all the extensions and some custom entries, passes from 1.47MB to 1.00MB, but the biggest change is in the number of lines: 5947 instead of 58143.
If it's useful, I can do some tests to see the average improvement on different scenarios.

@tbalden

This comment has been minimized.

Show comment
Hide comment
@tbalden

tbalden Dec 30, 2017

30% is an excellent gain. I'm adding the hosts file into initramfs in my custom kernels, so the smaller it is, the better.
I'm curious how Linux/android would handle the parsing. I'll try it when I get some time, out of curiosity.

tbalden commented Dec 30, 2017

30% is an excellent gain. I'm adding the hosts file into initramfs in my custom kernels, so the smaller it is, the better.
I'm curious how Linux/android would handle the parsing. I'll try it when I get some time, out of curiosity.

@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack Dec 30, 2017

Owner

@stefanopini as you can see, travis-ci is failing... Why did you comment-out the teardown code for testing?

Owner

StevenBlack commented Dec 30, 2017

@stefanopini as you can see, travis-ci is failing... Why did you comment-out the teardown code for testing?

Stefano
Added an option to compress the created hosts file.
In particular, the compression option removes non-necessary lines (empty lines and comments) and puts multiple domains in each line.
This option should solve the issue #411 regarding the DNS client service of Windows.
@stefanopini

This comment has been minimized.

Show comment
Hide comment
@stefanopini

stefanopini Dec 30, 2017

@StevenBlack sorry, I forgot to check the code with flake8 before committing it.
There were some too-long lines and an ambiguous variable... It should be ok now.
I commented the second tearDown call because it was executed twice, causing an error in my Windows environment. Why was it called twice? I didn't get it.

stefanopini commented Dec 30, 2017

@StevenBlack sorry, I forgot to check the code with flake8 before committing it.
There were some too-long lines and an ambiguous variable... It should be ok now.
I commented the second tearDown call because it was executed twice, causing an error in my Windows environment. Why was it called twice? I didn't get it.

Show outdated Hide outdated updateHostsFile.py Outdated
@@ -249,7 +250,7 @@ def test_freshen_update(self, _):
def tearDown(self):
BaseStdout.tearDown(self)
BaseStdout.tearDown(self)
# BaseStdout.tearDown(self)

This comment has been minimized.

@rautamiekka

rautamiekka Dec 30, 2017

Couldn't we do this like this to avoid erroring ?

while 1:
    try:
        BaseStdout.tearDown(self)
    except:
        break

Of course, if there are any exceptions to deal with, deal with them appropriately, and don't forget to break the loop in any case.

@rautamiekka

rautamiekka Dec 30, 2017

Couldn't we do this like this to avoid erroring ?

while 1:
    try:
        BaseStdout.tearDown(self)
    except:
        break

Of course, if there are any exceptions to deal with, deal with them appropriately, and don't forget to break the loop in any case.

This comment has been minimized.

@stefanopini

stefanopini Jan 2, 2018

Even if it should work, I think this is not necessary.
As far as I understand, the tearDown method closes the current standard output file (it was changed before calling the setUp method) and re-sets it to the system standard output.
Calling tearDown twice causes the closure of the system standard output, raising an error in the following.

@stefanopini

stefanopini Jan 2, 2018

Even if it should work, I think this is not necessary.
As far as I understand, the tearDown method closes the current standard output file (it was changed before calling the setUp method) and re-sets it to the system standard output.
Calling tearDown twice causes the closure of the system standard output, raising an error in the following.

@ScriptTiger

This comment has been minimized.

Show comment
Hide comment
@ScriptTiger

ScriptTiger Dec 31, 2017

Contributor

If line quantity is the issue, would "compressing" contiguous comment lines to a single line also reduce line quantity significantly enough to be acceptable without needing to remove comment lines entirely?

Also, in light of this being a line issue, have the type of line endings been tested, such as testing and comparing CR, LF, CR+LF? Depending on how resources might be allocated and how line endings are interpreted during parsing, is it possible for those having this issue that their systems were designed for efficiency at reading small lines of data iteratively broken by specific line endings, in which case inappropriate line endings may glob the entire file together as one line and be inefficiently parsed?

Contributor

ScriptTiger commented Dec 31, 2017

If line quantity is the issue, would "compressing" contiguous comment lines to a single line also reduce line quantity significantly enough to be acceptable without needing to remove comment lines entirely?

Also, in light of this being a line issue, have the type of line endings been tested, such as testing and comparing CR, LF, CR+LF? Depending on how resources might be allocated and how line endings are interpreted during parsing, is it possible for those having this issue that their systems were designed for efficiency at reading small lines of data iteratively broken by specific line endings, in which case inappropriate line endings may glob the entire file together as one line and be inefficiently parsed?

@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack Jan 1, 2018

Owner

Hi @stefanopini thank you for this pull request. I'll be evaluating this shortly, thank you for your patience!

Owner

StevenBlack commented Jan 1, 2018

Hi @stefanopini thank you for this pull request. I'll be evaluating this shortly, thank you for your patience!

@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack Jan 1, 2018

Owner

@stefanopini can I ask you to please do the following before I merge?

Thanks!

Owner

StevenBlack commented Jan 1, 2018

@stefanopini can I ask you to please do the following before I merge?

Thanks!

@stefanopini

This comment has been minimized.

Show comment
Hide comment
@stefanopini

stefanopini Jan 2, 2018

@ScriptTiger the idea before the removal of the comments and the empty lines is that the hosts file created with this script shouldn't need to be edited manually.
In my scenario (using all the extensions and some custom rules), comment and empty lines are respectively 2271 and 2629 out of 58143 lines. Contiguous comment lines are 793 only, therefore I don't consider them remarkable. Nevertheless, since comment lines are not impacting a lot on the total number of lines, we could add an option to keep them in the future.

Thank you for suggesting to test the different like breaks, I didn't think about that. Unfortunately, the problem occurs with every line break (CR, LF, and CR+LF). Interestingly, Windows is capable to parse each of them.

I also tried the hosts files on a machine with up-to-date Windows 10 (build 1709) and the issue is still there.

stefanopini commented Jan 2, 2018

@ScriptTiger the idea before the removal of the comments and the empty lines is that the hosts file created with this script shouldn't need to be edited manually.
In my scenario (using all the extensions and some custom rules), comment and empty lines are respectively 2271 and 2629 out of 58143 lines. Contiguous comment lines are 793 only, therefore I don't consider them remarkable. Nevertheless, since comment lines are not impacting a lot on the total number of lines, we could add an option to keep them in the future.

Thank you for suggesting to test the different like breaks, I didn't think about that. Unfortunately, the problem occurs with every line break (CR, LF, and CR+LF). Interestingly, Windows is capable to parse each of them.

I also tried the hosts files on a machine with up-to-date Windows 10 (build 1709) and the issue is still there.

@tbalden

This comment has been minimized.

Show comment
Hide comment
@tbalden

tbalden Jan 2, 2018

As this is an opt-in type of feature, it's worth considering for addition even if windows will be fixed in the future. I for one would be very happy about the size gain in the initramfs scenario I'm using

tbalden commented Jan 2, 2018

As this is an opt-in type of feature, it's worth considering for addition even if windows will be fixed in the future. I for one would be very happy about the size gain in the initramfs scenario I'm using

Stefano
Updated the documentation with the new compress option.
Removed a redundant skipstatichosts option.
@stefanopini

This comment has been minimized.

Show comment
Hide comment
@stefanopini

stefanopini Jan 2, 2018

@StevenBlack I updated the docs and the startswith code.
Do you think we're ready to merge now?

stefanopini commented Jan 2, 2018

@StevenBlack I updated the docs and the startswith code.
Do you think we're ready to merge now?

Show outdated Hide outdated updateHostsFile.py Outdated
Stefano
Fixed the number of domains in each line.
Fixed the number of domains in each line and added the support to
inline comments (they will be ignored as the comment lines).
Code refactoring.
@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack Jan 2, 2018

Owner

Thanks @stefanopini. You're awesome.

Owner

StevenBlack commented Jan 2, 2018

Thanks @stefanopini. You're awesome.

@StevenBlack StevenBlack merged commit 9d634c3 into StevenBlack:master Jan 2, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@welcome

This comment has been minimized.

Show comment
Hide comment
@welcome

welcome bot Jan 2, 2018

Congratulations on merging your first pull request! 🎉🎉🎉

welcome bot commented Jan 2, 2018

Congratulations on merging your first pull request! 🎉🎉🎉

@user789465

This comment has been minimized.

Show comment
Hide comment
@user789465

user789465 Jan 5, 2018

thanks for your continuing effort, if that can help people, good.

as for me, I'm really sorry, but I have no idea how to "compress" a host file.

user789465 commented Jan 5, 2018

thanks for your continuing effort, if that can help people, good.

as for me, I'm really sorry, but I have no idea how to "compress" a host file.

@onmyouji

This comment has been minimized.

Show comment
Hide comment
@onmyouji

onmyouji Jan 5, 2018

@user789465

  • install python and download the repo.
  • from command prompt type "python updateHostsFile.py -a -c"

onmyouji commented Jan 5, 2018

@user789465

  • install python and download the repo.
  • from command prompt type "python updateHostsFile.py -a -c"

@ScriptTiger ScriptTiger referenced this pull request Jan 5, 2018

Closed

DNS Client Cache Issue #5

@tbalden

This comment has been minimized.

Show comment
Hide comment
@tbalden

tbalden Jan 5, 2018

@user789465
Please read the opening description and big things will be revealed to you!
"In particular, the compression option removes non-necessary lines (empty lines and comments) and puts multiple domains in each line."

tbalden commented Jan 5, 2018

@user789465
Please read the opening description and big things will be revealed to you!
"In particular, the compression option removes non-necessary lines (empty lines and comments) and puts multiple domains in each line."

@ScriptTiger

This comment has been minimized.

Show comment
Hide comment
@ScriptTiger

ScriptTiger Jan 5, 2018

Contributor

I was doing a bit more research into this and actually came full circle back to this repository, ranked second on Google to a hit on superuser.com, so I thought I'd share it here just in case anyone wanted to do a bit more digging into this: #49. I guess @stefanopini had already mentioned it here at some point, as well, but edited/deleted it out at some point.

I am not actually personally affected by this, but it definitely is an interesting new feature indeed. So props to everyone that helped finally solve this and bring the solution to fruition.

Contributor

ScriptTiger commented Jan 5, 2018

I was doing a bit more research into this and actually came full circle back to this repository, ranked second on Google to a hit on superuser.com, so I thought I'd share it here just in case anyone wanted to do a bit more digging into this: #49. I guess @stefanopini had already mentioned it here at some point, as well, but edited/deleted it out at some point.

I am not actually personally affected by this, but it definitely is an interesting new feature indeed. So props to everyone that helped finally solve this and bring the solution to fruition.

@user789465

This comment has been minimized.

Show comment
Hide comment
@user789465

user789465 Jan 7, 2018

the compression seems to work a little;
is there a way to compress even more ?
(before I had 60sec delay lag, now it feels like 10 or 5sec)
the host file is about 5000 lines, maybe 100 would to the trick ?
how to make that happen ?

user789465 commented Jan 7, 2018

the compression seems to work a little;
is there a way to compress even more ?
(before I had 60sec delay lag, now it feels like 10 or 5sec)
the host file is about 5000 lines, maybe 100 would to the trick ?
how to make that happen ?

@StevenBlack

This comment has been minimized.

Show comment
Hide comment
@StevenBlack

StevenBlack Jan 7, 2018

Owner

@user789465 Windows – always a problem it seems – limits this to 9 domains per line.

Owner

StevenBlack commented Jan 7, 2018

@user789465 Windows – always a problem it seems – limits this to 9 domains per line.

@funilrys funilrys referenced this pull request Jul 8, 2018

Closed

DNS Client High Cpu Usage #710

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment