Closed mrtnRitter closed 2 years ago
In addition to the other concern raised, three unit tests are failing in this PR. @mrtnRitter, since you added functionality, you will also have to
interface
argument)Separately, I have found this line of code to be troublesome on some Windows machines (although the details are fuzzy because it has been so long since I've ran into that issue).
Hi @DavidPratt512, thank you for reaching out. You are right, I just edited wakeonlan.py, and left test_wakeonlan.py untouched. Adding support for multiple NICs was my only aim, because I needed it for work. The script runs on my windows machine for years now, waking computers day by day.
I would like to know more about the unit test you did and the circumstances in which they are failing. Please provide any useful information, otherwise I cannot offer help. On the other hand, with only 2 lines pure python added and a some expanded by "interface", I'm concerned that there is not much to dig into.
My code might not be perfect, but being an expert for WOL by yourself, you are welcome to contribute on a better working solution.
Hello @ericfrey, sorry for replying late! I briefly looked at etherwake after you have written. Assuming we are talking about the same script, it isn't written in Python and kind of a different story, doing things differently in a different language (C or C++, if I remember correctly).
To get a starting point for a solution for you, can you try the wol-script from DavidPratt512? It supports interfaces natively. If it does the job, we know something is wrong with my code, if it doesn't work either, we know we have to dig deeper. https://github.com/DavidPratt512/wol
Thank you, I am curious about your result!
Hey @mrtnRitter, I will try and retrace my steps about that particular issue I was facing and get back to you. I believe I had very similar code as the code you wrote in this PR in my own wol script that you linked to in your message to Eric - so hopefully that makes it a bit easier for me.
Also, I have a feeling we are creating a trap for ourselves the more socket/networking logic we write inside the send_magic_packet()
function. From the perspective of a standalone command-line utility, it doesn't matter at all, but I believe some people are using this package in their python code since this package is on PyPI.
The reason why it feels like we would be falling into a trap is because of a few reasons:
interface
parameter has nothing to do with the actual sending of a magic packet; the interface used helps defines the socket, not what the socket does.send_magic_packet()
function then makes its use impossible in this scenario. In fact, this very discussion is a symptom of creating the socket inside that function.What's funny is that I currently do these same things in my own script... but I'm working on fixing them... eventually :)
Hi @DavidPratt512, thank you for your answer.
Regarding the trap:
create_magic_packet
. However, this should be added to the readme.interface
disturbs this logic, you also might disagree with this line sock.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1)
.create_socket_and_send_magic_packet
, but just send_magic_packet.
We can only move the defining and creation of the socket into a new function, like create_socket.
However, one would still call send_magic_packet
and it will just call create_socket
and there is no difference for the one, who use this as a package. I think, the code is as clear as possible right now. The only think missing was the interface
.
If you remove all the comments, it is pretty short and easy to overlook. Additional functions would only make sense, if they can be used independently, like create_magic_packet
. But I don't see such a use for something like create_socket
in terms of this script.
Don't fix what isn't broken ;)
Hey @mrtnRitter,
the function is not
create_socket_and_send_magic_packet
but justsend_magic_packet
.
That is basically my entire point summed up very nicely. I think since I've been reading about design patterns, I've been eager to jump on code that is coupled when it doesn't have to be. But let me argue against myself for a moment...
There are two scenarios I see:
send_magic_packet()
function has unwanted functionality (creating a socket). One solution is to extract the socket creation logic out of the function and add a parameter that expects a socket. So, something like this:
def send_magic_packet(*macs, ip_address=BROADCAST_IP, port=DEFAULT_PORT, sock=None):
if sock is None:
raise ValueError('expected socket')
packets = [create_magic_packet(mac) for mac in macs]
for packet in packets:
sock.sendto(packet, (ip, port))
if name == 'main': parser = ... args = parser.parse_args()
with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as sock:
sock.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1)
sock.bind((args.n, 0)) # bind to interface
send_magic_packet(*args.macs, ip_address=args.i, port=args.p, sock=sock)
But now if anyone is using this library in a script, their code is broken.
2. The second scenario tries to un-break their code. Just change the `send_magic_packet()` function to this:
```python
def send_magic_packet(*macs, ip_address=BROADCAST_IP, port=DEFAULT_PORT, sock=None):
packets = [create_magic_packet(mac) for mac in macs]
with sock or create_socket() as s: # create socket here
for packet in packets:
s.sendto(packet, (ip, port))
def create_socket(interface=None):
sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
sock.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1)
if interface is not None:
sock.bind((interface, 0))
return sock
But now we're back to square one. So extracting socket creation logic out of the function isn't a good idea (because it either breaks existing code or leads to that same logic being present in the function - and nobody is asking for this anyway).
You're exactly right when you say that someone can just use the create_magic_packet()
function and send that packet however they wish and I was silly to not see that before.
Okay, so with that idea left behind, let's talk about interfaces :)
I've tested the current code in the main
branch as well as in your pull request across a few systems. I set up my desktop and laptop with Wireshark to listen for WOL packets. Success was defined if both the desktop and laptop received the broadcasted magic packet. Here are the results:
main
: ✔️main
: ✔️main
: ✔️main
: ✔️I'm not sure exactly why the laptop and VM are failing to send the magic packet, but it obviously has something to do with the sock.bind((interface, 0))
line of code. A way around this is to only use that line if the function was passed the interface argument:
def send_magic_packet(*macs, ip_address=BROADCAST_IP, port=DEFAULT_PORT, interface=None):
packets = [create_magic_packet(mac) for mac in macs]
with socket.socket(socket.AF_INET, socket.SOCK_DGRAM) as sock:
if interface is not None:
sock.bind((interface, 0))
sock.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1)
sock.connect((ip_address, port))
for packet in packets:
sock.send(packet)
With this code, the behavior remains the same for anyone currently using this library and allows users to specify an interface to bind to if they so desire. This is probably the least intrusive change while still adding this feature. What are your thoughts?
Also, if I have some spare time, I'll try and find out exactly why the code in this PR is failing on those particular setups I listed above.
Hi @DavidPratt512 ,
I did some research and it seems, that this line DEFAULT_INTERFACE = socket.gethostbyname(socket.gethostname())
isn't returning the host IP such reliable as I thought. Especially on Linux-like systems this often just returns the localhost IP.
The script is edited following your suggestion. It's not that elegant now, but a reliable way to get the host IP across all systems isn't either, so this is still the best option.
Thank you very much for your improvements and testing!
Hey @mrtnRitter,
I am also still not entirely sure why socket.gethostbyname(socket.gethostname())
isn't terribly consistent across different systems. Maybe there is some neat trick to get consistent behavior, but I suppose that's by the by.
I do have some nit-picky feedback on the code changes in this PR.
The first clause on line 68 should probably reference a NIC as being a network interface controller, as opposed to a network adapter, because we are using the abbreviation NIC.
On a host with more than one **network interface controller** (NIC), ...
Also the next line of the README says
The default interface is the first NIC registered in system.
What exactly does this mean? And is it accurate now that we've made a change to the code? Also should it read in **the** system.
?
On line 71 of the README, the ip_address
keyword argument is misspelled. Also there is no closing parenthesis on the function call. Additionally, is 0.0.0.1
a common address for an interface? I'm asking because I honestly don't know.
On line 94 of the README, "address" is misspelled as adresse
; "through" is misspelled as thrue
. Also at the beginning of the help sentence, we see nic
, and by the end of the sentence we see NIC
. I think we should stick to a single capitalization scheme for the abbreviation.
On line 51, "send" should be "sent"; "through" may make more sense as "from"; there needs to be a period to close the line.
Line 57 should be
if interface is not None:
(All comparisons to None
should be done by identity, not equality.)
Line 97 help text should match the (adjusted) text from line 51 (following the pattern of the other script-level arguments).
The test test_main
is failing because it is expecting only the keyword arguments ip_address
and port
for the send_magic_packet()
function. Simply add interface=None
to the list of parameters on line 219.
Or you could change the test to this to make sure we are testing the main()
function with the new interface
parameter:
@patch("wakeonlan.send_magic_packet")
def test_main(send_magic_packet: Mock) -> None:
"""
Test if processed arguments are passed to send_magic_packet.
"""
main(["00:11:22:33:44:55", "-i", "host.example", "-p", "1337"])
main(["00:11:22:33:44:55", "-i", "host.example", "-p", "1337", "-n", "0.0.0.1"])
assert send_magic_packet.mock_calls == [
call("00:11:22:33:44:55", ip_address="host.example", port=1337, interface=None),
call("00:11:22:33:44:55", ip_address="host.example", port=1337, interface="0.0.0.1"),
]
Now we've "fixed" the broken tests, another test should probably be added mimicking the test test_send_magic_packet
. You could just copy that entire existing test, add interface="0.0.0.1"
to the call to send_magic_packet()
, and finally add the line call().__enter__().bind(("0.0.0.1", 0))
after the first call().__enter__()
line. At this stage, I see 7 passing tests.
Also, be sure to run black
on any changes to python code since this repository uses that code format :)
Let me know if you have thoughts on any of the changes I'm suggesting.
Hi @DavidPratt512, thank you for your nit-picky feedback! I take it as a valuable lesson :)
From what I found at stackoverflow, socket.gethostbyname(socket.gethostname())
uses DNS to resolve the host IP address. However, DNS may not having a corresponding entry and so it fails. I don't know if it works differently on windows machines since it seems to work more consistent there.
Unfortunately there is no trick. The suggested solutions all include to check the returned IPs for plausibility.
I corrected the misspellings in the README.rst (sorry, I'm not a native speaker and git editor don't show spelling errors - should check the readme files with word or something in the future) and got rid of the term NIC in favor of network adapter. I changed the example IP address to a more realistic one.
Thank you especially for your suggestions regarding test_wakeonlan.py. I ignored this file all the time, because I didn't quite know what to do with it.
I hope this PR is now more consistent to the main branch.
Hey @mrtnRitter,
No problem :) Thanks for making those changes.
I retested this PR on all of the machines again, with much better results this time around:
main
: ✔️main
: ✔️main
: ✔️main
: ✔️Running pytest
yields seven passing tests. Also, the script successfully wakes my desktop PC.
I only have a few minor nit-picks left.
wakeonlan.py
file in line 78. python wakeonlan.py -h
. The only difference is that the .py
suffix in the first line is removed.black
on all the python files. You can do this with black .
. And if you need to install black
, you can do that with pip install black
.pyproject.toml
to 2.1.0
. (You could also do this with poetry version minor
.)Last, after you make and commit all these changes, could you squash all of the commits in this PR? You should be able to do that with
git reset --soft 9ba45a9 # latest commit before your changes
git commit -a
git push -f
I think that will be the end of my nit-picks :) Thanks for making all the changes previously.
Once we get this squared away, I think we can ping the owner of the repo for a review.
Hey @DavidPratt512,
I did as you said and we have now a nice PR following all the rules. Thank you very much for taking the time and giving feedback!
Hi @remcohaszing!
Good idea removing the exposed DEFAULT_INTERFACE
. The code does even make more sense now.
I edited everything as requested and looking forward to finally see the interface option in the main branch.
Awesome! I’ll wait for the weekend for a response on #21, so both can be released as 2.1.0, but if that takes longer, I’ll release this sunday or monday.
Sorry, I missed to check all usages of DEFAULT_INTERFACE
.
Should work now.
Merging #22 (01c8c47) into main (9ba45a9) will increase coverage by
0.74%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #22 +/- ##
==========================================
+ Coverage 92.59% 93.33% +0.74%
==========================================
Files 1 1
Lines 27 30 +3
Branches 4 5 +1
==========================================
+ Hits 25 28 +3
Misses 1 1
Partials 1 1
Impacted Files | Coverage Δ | |
---|---|---|
wakeonlan.py | 93.33% <100.00%> (+0.74%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 9ba45a9...01c8c47. Read the comment docs.
Aaand released as 2.1.0! :tada:
I have an ethernet USB adapter on a Raspberry Pi running Raspbian 10. I was the code in this pull request would let me send the magic packet out that interface, but it doesn't seem to work. I used the -n option with the IP address bound to that NIC. I tried as normal users (a member of the netdev group) and root. The command runs, but the system does not turn on. The call and command worked when I had only one interface. The etherwake command (from the etherwake package) works when I specify the interface on the command line, so it seems there must be a way to do it in principle.