Socketify pynut #2792

cgarz · 2025-01-31T06:46:14Z

As discussed in #2183 I have updated PyNUT.py.in to no longer require nut_telnetlib.py.

The update required only minimal changes to how the client sends and receives data from upsd.
Essentially, I just went through the file and switched all self.__srv_handler write to send and all read_until calls to a new function. The only other thing is fixing the timeout init argument.

Testing has been performed by simply adding:

if __name__ == '__main__':
    nut = PyNUTClient(debug=True, login='upsuser', password='upspass')
    ups_name = nut.GetUPSNames()[0]

    GetUPSList =        nut.GetUPSList()
    GetUPSNames =       nut.GetUPSNames()
    GetUPSVars =        nut.GetUPSVars(ups=ups_name)
    CheckUPSAvailable = nut.CheckUPSAvailable(ups=ups_name)
    GetUPSCommands =    nut.GetUPSCommands(ups=ups_name)
    GetRWVars =         nut.GetRWVars(ups=ups_name)
    SetRWVar =          nut.SetRWVar(ups=ups_name, var='driver.debug', value='1')
    RunUPSCommand =     nut.RunUPSCommand(ups=ups_name, command='driver.reload')
    DeviceLogin =       nut.DeviceLogin(ups=ups_name)
    FSD =               nut.FSD(ups=ups_name)
    _help =             nut.help()
    ver =               nut.ver()
    ListClients =       nut.ListClients(ups=ups_name)

To the bottom of the script and running in pycharm debug mode to note the contents of all the variables and noting the logs/effect on the server.

jimklimov · 2025-01-31T07:15:51Z

Sounds great, thanks!

For FSD, you can extend the testing setup in tests/NIT/nit.sh (called as make check-NIT in CI). There are dummy drivers and a dummy upsmon that just logs its actions so the script can review its log file to check the events it saw.

The stack runs on dedicated random ports, can use custom SHUTDOWNCMD, and not running as root, so it won't actually shut down the computer.

It does also call the python and C++ client tests. With FSD I think the Python client would be essentially another upsmon by role, so might need using that login? I'm at a conference now so can't think more deeply on that.

To develop tests, see NIT_CASE envvar and make check-NIT-sandbox-devel & (wait some 15 sec and follow printed comments to source the generated env file).

cgarz · 2025-01-31T18:01:25Z

I tried that testing setup but couldn't get it to work.
I then tried a custom SHUTDOWNCMD, forgot to restart upsmon, so it shut down 🤣
Ah well, at least it's been tested now.

Is this DCO thing mandatory? I really don't want to add my private email.
My commits are already signed with my verified signature, would that not be equivalent?

jimklimov · 2025-02-01T06:51:27Z

I guess it's okay then.

jimklimov · 2025-02-04T12:50:19Z

From CI failures, it seems the solution's syntax is for Python 3. Can you please rewrite it so the module would work on both new systems and those stuck with 2.7 or so?

Traceback (most recent call last):
  File "/dev/shm/jenkins-nutci/nut_nut_PR-2792@2/nut-2.8.2.2040.3/_build/sub/scripts/python/module/test_nutclient.py", line 6, in <module>
    import PyNUT
  File "/dev/shm/jenkins-nutci/nut_nut_PR-2792@2/nut-2.8.2.2040.3/_build/sub/scripts/python/module/PyNUT.py", line 122
    while (data_end_index := data.find(finished_reading_data)) == -1:
                          ^
SyntaxError: invalid syntax

Per https://stackoverflow.com/a/73333155/4715872 seems this would just need to dumb down the code a little, and write two lines instead of one (assign and check condition separately).

UPDATE: Hm, inside a loop that might be a bit more difficult. Probably pre-assign the var to -1, then in the loop actually assign it, check value, break out if needed. Looks dumb, but we are dumbing it down after all :\

jimklimov

Looks generally solid, thanks! Kudos for finding the extra bugs (e.g. timeout ignorance) ;)

Now we need to brush it up a bit, so tests would also pass for older systems stuck with Python 2.x, as commented earlier, and update some other recipe and doc files as noted in review comments.

jimklimov · 2025-02-04T12:51:30Z

scripts/python/module/PyNUT.py.in

@@ -110,18 +105,31 @@ timeout  : Timeout used to wait for network response
        self.__port     = port
        self.__login    = login
        self.__password = password
-        self.__timeout  = 5
+        self.__timeout  = timeout


jimklimov · 2025-02-04T12:53:16Z

scripts/python/module/PyNUT.py.in

-    if "true" == os.getenv('DEBUG', 'false'):
-        print( "[DEBUG] Fall back to private copy of telnetlib for PyNUTClient\n" )
-    import nut_telnetlib as telnetlib
+import socket


Probably makes sense to add to the changelog above, that a "better solution" did get developed. And in NEWS.adoc too.

Eventually also drop nut_telnetlib from Git, Makefile.am, setup.py...

I gather that socket is available in core of both 2.x and 3.x Pythons, right?

cgarz · 2025-02-04T22:27:52Z

From CI failures, it seems the solution's syntax is for Python 3. Can you please rewrite it so the module would work on both new systems and those stuck with 2.7 or so?

Ah yea I forgot the walrus operator was only added in 3.8. I've gotten a bit addicted to it and use it automatically without thinking now 😆.
I've pushed a commit to address that now.

Probably makes sense to add to the changelog above, that a "better solution" did get developed. And in NEWS.adoc too.

Eventually also drop nut_telnetlib from Git, Makefile.am, setup.py...

For the changelog I was thinking this:

# 2025-01-31 cgar <github.com/cgarz> - Version 1.8.0
#            Removed telnetlib dependency. Switched to using socket directly.

I'm not sure how to update NEWS.adoc though. What section would it go in? Would it overwrite the older one?

I gather that socket is available in core of both 2.x and 3.x Pythons, right?

Oh yea absolutely, socket is ancient, it's been there since at least v1.4 in 1996. Maybe even pre 1.0, though not sure.

jimklimov · 2025-02-04T23:13:03Z

Thanks! The changelog proposal looks right. For NEWS.adoc, see one of the topmost section with (planned) changes between 2.8.2 and 2.8.3. There could be an entry about providing nut_telnetlib - if it was after 2.8.2 already, can be replaced :)

jimklimov · 2025-02-04T23:17:45Z

Just love to see how far a bit of know-how can go! :)

AppVeyorBot · 2025-02-04T23:20:33Z

✅ Build nut 2.8.2.2710-master completed (commit f874be659c by @cgarz)

cgarz · 2025-02-04T23:32:27Z

For NEWS.adoc, see one of the topmost section with (planned) changes between 2.8.2 and 2.8.3. There could be an entry about providing nut_telnetlib - if it was after 2.8.2 already, can be replaced :)

Ah yea I see it now. Reading it I'm actually thinking it might be good as is.
Do you think just appending this PR's number after the [#2183] (or replacing it) would be sufficient?

jimklimov · 2025-02-04T23:38:15Z

Might be, not online now to check more.

cgarz · 2025-02-04T23:41:21Z

Might be, not online now to check more.

Ah no probs, appending would make it:

 - the `PyNUTClient` module should no longer rely on presence of a `telnetlib`
   module in the build or execution environment (deprecated in Python 3.11,
   removed since Python 3.13). [#2183] [#2792]

That look good?

jimklimov · 2025-02-05T07:52:58Z

In NEWS, PR/issue numbers can be comma-separated.

cgarz · 2025-02-05T10:28:02Z

In NEWS, PR/issue numbers can be comma-separated.

Thanks that does look nicer. Switched to: [issue #2183, PR #2792]

Should I commit those now? Or is there anything else to update?

jimklimov · 2025-02-06T07:50:07Z

I guess, any mentions of git grep nut_telnetlib in recipes, if the file should not be delivered (or git-tracked) anymore?

cgarz · 2025-02-06T09:00:39Z

I guess, any mentions of git grep nut_telnetlib in recipes, if the file should not be delivered (or git-tracked) anymore?

Yes it shows:

$ git grep nut_telnetlib
COPYING: The scripts/python/module/nut_telnetlib.py file is copied for fall-back
configure.ac:        AC_MSG_CHECKING([if we can use stock Python3 telnetlib module for PyNUTClient provided with interpreter ${PYTHON3} (note for warnings from Python 3.11 and beyond: we have a fallback nut_telnetlib module just in case)])
configure.ac:            AC_MSG_CHECKING([if we can use fallback Python3 nut_telnetlib module for PyNUTClient])
configure.ac:            if (cd "${srcdir}"/scripts/python/module && ${PYTHON3} -c "import nut_telnetlib as telnetlib") \
configure.ac:        AC_MSG_CHECKING([if we can use stock Python telnetlib module for PyNUTClient provided with interpreter ${PYTHON} (note for warnings from Python 3.11 and beyond: we have a fallback nut_telnetlib module just in case)])
configure.ac:            AC_MSG_CHECKING([if we can use fallback Python nut_telnetlib module for PyNUTClient])
configure.ac:            if (cd "${srcdir}"/scripts/python/module && ${PYTHON} -c "import nut_telnetlib as telnetlib") \
scripts/python/Makefile.am:     module/nut_telnetlib.py
scripts/python/README.adoc:as a quick stop-gap solution, NUT sources provide `nut_telnetlib.py` -- a copy
scripts/python/module/Makefile.am:EXTRA_DIST = tox.ini MANIFEST.in nut_telnetlib.py
scripts/python/module/Makefile.am:# nut_telnetlib.py is not converted from a .in template, it
scripts/python/module/Makefile.am:.pypi-src: test_nutclient.py.in PyNUT.py.in setup.py.in nut_telnetlib.py README.adoc Makefile $(top_srcdir)/LICENSE-GPL3
scripts/python/module/Makefile.am:       cp -pf "$(srcdir)/nut_telnetlib.py" PyNUTClient/ || exit ; \
scripts/python/module/Makefile.am:              cp -pf "$(srcdir)/nut_telnetlib.py" . || exit ; \
scripts/python/module/PyNUT.py.in:                data += self.__srv_handler.recv(50)  # nut_telnetlib.py uses 50
scripts/python/module/setup.py.in:    # and removed since 3.13, so as a hotfix, NUT provides a copy nut_telnetlib.py

Should I remove/change these and nut_telnetlib.py?

jimklimov · 2025-02-06T11:28:03Z

Yes please, if you have a moment to perfect the PR :)

Handles choosing IPv4 and IPv6 automatically.

remove nut_telnetlib.py references.

remove nut_telnetlib.py reference.

remove nut_have_telnetlib checks

cgarz · 2025-02-07T09:10:16Z

I've committed those changes now however I'm not sure about configure.ac.
Have I removed too much? or does that look ok?

Running git grep telnet now only shows:

NEWS.adoc: - the `PyNUTClient` module should no longer rely on presence of a `telnetlib`
docs/net-protocol.txt:make use of this program by using telnet or netcat.  If you use
docs/net-protocol.txt:telnet, make sure you don't have it set to negotiate extra options.
docs/net-protocol.txt:upsd doesn't speak telnet and will probably misunderstand your first
docs/nut.dict:telnetlib
scripts/python/module/PyNUT.py.in:[...]
scripts/python/module/nut_telnetlib.py:[...]
tests/NIT/nit.sh:    # only bind to IPv6, and Python telnetlib resolves IPv4

I'll remove nut_telnetlib.py once the commits look good and the only other thing is nit.sh at the end there.
It mentions telnetlib only does IPv4, however now that I've changed from socket.connect to socket.create_connection that should handle IPv6 too. Should that be changed? The telnet lib actually used that method too so this might be a mistake.

jimklimov · 2025-02-07T10:10:39Z

Thanks for the heads-up about configure.ac. The current change indeed won't work I think, at least not as the "if" clause intends to make it a choice of logical path: the nut_with_pynut_py* vars would remain unpopulated (and never explicitly emptied either).

I suggest to look at earlier changes that added (nut_)telnetlib checks in https://github.com/networkupstools/nut/pull/2505/files and https://github.com/networkupstools/nut/pull/2501/files and git revert certain commits, or just git difftool -y v2.8.2 configure.ac (with Meld or something else you like set up as the difftool) and import back that part of the script from before the changes.

… for "socket" instead of "telnetlib" module [networkupstools#2183] Signed-off-by: Jim Klimov <[email protected]>

jimklimov · 2025-02-07T11:27:34Z

I've updated the configure.ac to use the same logic block here as was in NUT v2.8.2, just replaced the check for telnetlib with a check for socket (so we are sure both the python interpreter is usable, and the module we actually need is available - and remains in core/distro for future NUT builds).

Local ./ci_build.sh runs that did fail for me with your branch tip (configure: error: Prerequisites for PyNUT not found, can't install as required for NUT-Monitor desktop application) passed now, so I've pushed the change here.

cgarz · 2025-02-07T11:32:59Z

Excellent! Thanks so much, I was sitting there staring at the diffs not having a clue how to fix it 😆.

Are all the other files good?
Do you think its worth changing that IPv4 note thing in tests/NIT/nit.sh ?

…do not need anymore Signed-off-by: Jim Klimov <[email protected]>

jimklimov · 2025-02-07T14:30:52Z

I've re-read that note, it seems to have been more about name resolution, which I am not sure which layer had a problem with.

De-facto, as there were experiments with "dual-stack" TCP support, it was found that in some cases the server with LISTEN localhost 3493 requirement could bind to only one of the families (IPv4 or IPv6) due to whatever reasons, and a client could get the other family's address first if both are localhost in /etc/hosts (same thing with round-robined aliases in DNS). This seems to have been a frequent occurrence with Python tests, while NUT C clients might try all addresses the resolver returns (at least, I had that idea at the time).

I'll generalize the message and that would be it. Here it is a comment why we don't use one "localhost" listener but two for explicit IP addresses.

…IPv4/6 addresses, not a name, for upsd.conf Signed-off-by: Jim Klimov <[email protected]>

jimklimov · 2025-02-08T12:02:25Z

Thanks a lot, merging!

…empty) for recipe to be fully defined [#2792] Signed-off-by: Jim Klimov <[email protected]>

cgarz · 2025-02-08T12:12:08Z

Fantastic! Thanks for all your help with finishing it off.

jimklimov · 2025-02-08T12:40:45Z

Well, sadly or not, but development entails more than just a bit of great coding :)

jimklimov · 2025-02-08T15:56:21Z

FWIW, current state of the module should be uploaded to https://test.pypi.org/project/pynutclient/2.8.2.2132/ now, so can be tested as a standalone module.

cgarz added 3 commits January 31, 2025 06:06

added replacement read_until and leftover buffer

94f6109

switch to socket send and replacement read_until

60a7ca1

Switch to socket timeout, fix its init argument

1209078

cgarz mentioned this pull request Jan 31, 2025

PyNUTClient needs telnetlib which is deprecated #2183

Closed

jimklimov added enhancement packaging python portability We want NUT to build and run everywhere possible impacts-release-2.8.2 Issues reported against NUT release 2.8.2 (maybe vanilla or with minor packaging tweaks) labels Feb 4, 2025

jimklimov added this to the 2.8.3 milestone Feb 4, 2025

jimklimov requested changes Feb 4, 2025

View reviewed changes

Remove walrus op for older python compatibility.

7ee628f

cgarz added 2 commits February 7, 2025 08:45

Use socket create_connection instead of connect,

9f81e81

Handles choosing IPv4 and IPv6 automatically.

Update PyNUT changelog, bump version to 1.8.0

cf0ea3e

cgarz added 7 commits February 7, 2025 08:49

update COPYING, remove nut_telnetlib.py mention.

f9a41e7

Update python module Makefile.am,

67d6689

remove nut_telnetlib.py references.

Update python Makefile.am,

9b06ddb

remove nut_telnetlib.py reference.

Update NEWS.adoc PyNUTClient mention to include PR

3cb289d

Update README.adoc, remove telnetlib section

ce23967

Update setup.py.in, remove install_requires lib

0488ec5

Update configure.ac,

a940334

remove nut_have_telnetlib checks

configure.ac: restore nut_with_pynut_py* checks from v2.8.2, but test…

6d671b0

… for "socket" instead of "telnetlib" module [networkupstools#2183] Signed-off-by: Jim Klimov <[email protected]>

scripts/python/module/nut_telnetlib.py: Remove the copy of module we …

15df281

…do not need anymore Signed-off-by: Jim Klimov <[email protected]>

tests/NIT/nit.sh: update comment about use of two explicit localhost …

c5540c4

…IPv4/6 addresses, not a name, for upsd.conf Signed-off-by: Jim Klimov <[email protected]>

jimklimov merged commit d0086f0 into networkupstools:master Feb 8, 2025
26 of 27 checks passed

jimklimov added a commit that referenced this pull request Feb 8, 2025

scripts/python/Makefile.am: restore definition of PYNUT_COMMON_CODE (…

384f697

…empty) for recipe to be fully defined [#2792] Signed-off-by: Jim Klimov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socketify pynut #2792

Socketify pynut #2792

cgarz commented Jan 31, 2025 •

edited

Loading

jimklimov commented Jan 31, 2025

cgarz commented Jan 31, 2025

jimklimov commented Feb 1, 2025

jimklimov commented Feb 4, 2025 •

edited

Loading

jimklimov left a comment

jimklimov Feb 4, 2025

jimklimov Feb 4, 2025

cgarz commented Feb 4, 2025

jimklimov commented Feb 4, 2025

jimklimov commented Feb 4, 2025

AppVeyorBot commented Feb 4, 2025

cgarz commented Feb 4, 2025

jimklimov commented Feb 4, 2025

cgarz commented Feb 4, 2025 •

edited

Loading

jimklimov commented Feb 5, 2025

cgarz commented Feb 5, 2025

jimklimov commented Feb 6, 2025

cgarz commented Feb 6, 2025

jimklimov commented Feb 6, 2025

cgarz commented Feb 7, 2025

jimklimov commented Feb 7, 2025 •

edited

Loading

jimklimov commented Feb 7, 2025

cgarz commented Feb 7, 2025 •

edited

Loading

jimklimov commented Feb 7, 2025

jimklimov commented Feb 8, 2025

cgarz commented Feb 8, 2025

jimklimov commented Feb 8, 2025

jimklimov commented Feb 8, 2025

Socketify pynut #2792

Socketify pynut #2792

Conversation

cgarz commented Jan 31, 2025 • edited Loading

jimklimov commented Jan 31, 2025

cgarz commented Jan 31, 2025

jimklimov commented Feb 1, 2025

jimklimov commented Feb 4, 2025 • edited Loading

jimklimov left a comment

Choose a reason for hiding this comment

jimklimov Feb 4, 2025

Choose a reason for hiding this comment

jimklimov Feb 4, 2025

Choose a reason for hiding this comment

cgarz commented Feb 4, 2025

jimklimov commented Feb 4, 2025

jimklimov commented Feb 4, 2025

AppVeyorBot commented Feb 4, 2025

cgarz commented Feb 4, 2025

jimklimov commented Feb 4, 2025

cgarz commented Feb 4, 2025 • edited Loading

jimklimov commented Feb 5, 2025

cgarz commented Feb 5, 2025

jimklimov commented Feb 6, 2025

cgarz commented Feb 6, 2025

jimklimov commented Feb 6, 2025

cgarz commented Feb 7, 2025

jimklimov commented Feb 7, 2025 • edited Loading

jimklimov commented Feb 7, 2025

cgarz commented Feb 7, 2025 • edited Loading

jimklimov commented Feb 7, 2025

jimklimov commented Feb 8, 2025

cgarz commented Feb 8, 2025

jimklimov commented Feb 8, 2025

jimklimov commented Feb 8, 2025

cgarz commented Jan 31, 2025 •

edited

Loading

jimklimov commented Feb 4, 2025 •

edited

Loading

cgarz commented Feb 4, 2025 •

edited

Loading

jimklimov commented Feb 7, 2025 •

edited

Loading

cgarz commented Feb 7, 2025 •

edited

Loading