Bug 7557 - hiveconf requires Python 2
Summary: hiveconf requires Python 2
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Misc (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.13.0
Assignee: Samuel Mannehed
URL:
Keywords: frifl_tester, linma_tester, relnotes
Depends on:
Blocks: 7554
  Show dependency treegraph
 
Reported: 2020-09-17 09:43 CEST by Samuel Mannehed
Modified: 2021-07-20 10:31 CEST (History)
3 users (show)

See Also:
Acceptance Criteria:


Attachments
hconf file for printer nearest (255 bytes, text/plain)
2020-11-12 11:33 CET, Frida Flodin
Details

Description Samuel Mannehed cendio 2020-09-17 09:43:35 CEST
It needs to be converted to work with Python 3. Furthermore, there are no unit tests available for hiveconf yet.
Comment 23 Linn cendio 2020-10-23 13:29:21 CEST
Hiveconf now supports both Python 2 and Python 3. Before resolving this bug we will drop all Python 2 support.

We have decided that all strings internal to hiveconf will be unicode strings. We made this decision since it will fit better into the Python 3 world. This means that the content read from these files must be converted to unicode somehow. The files can reasonably be assumed to be encoded in the system's locale, which in turn means that we have a dependency on the system's locale.

All programs that use hiveconf must therefore call setlocale() in order for Python to be able to access which locale the system uses. The fact that some ThinLinc programs doesn't do this was split out to bug 7571.

All the above means that while supporting Python 2, we must do extra work to ensure that all strings are internally converted to unicode. This is done by decoding these strings using the system's locale. In Python 3, strings are unicode by default, special handling is not needed in most cases.
Comment 24 Linn cendio 2020-10-23 13:29:40 CEST
Note however that, at the moment, Python 3 only works on systems where the system's locale is UTF-8. This is due to a change regarding the file path argument to built-in Python functions (e.g. open() and os-functions). In Python 2 the argument is encoded using the system's locale, and in Python 3 these are encoded using UTF-8, always. We don't agree with how Python 3 works, and want to use file names encoded using the system's locale instead. This work remains before we consider this bug resolved.
Comment 25 Linn cendio 2020-10-23 15:51:34 CEST
Here's a list of things we need to clean up once we are ready to drop support for Python 2:

hiveconf.py
-----------
* Clean up imports (io.open, urllib2 etc.). Don't miss 'from __future__ import' on the very first line of the file. Indicated by FIXME:s.

* Conversions regarding only using unicode internally, which includes both decoding input and encoding output (i.e. return values). These conversions are not needed in Python 3, and thus should be removed. Indicated by both FIXME:s and two distinct if-cases on the form below, that will only be true for Python 2.
> if str == bytes
> if 'unicode' in dir(__builtins__)
* Use of u""-strings when writing to file. In Python 2, when writing a Python string to a file opened with io.open(), the string needs be a unicode string. In order to convert the Python string, we added a 'u' to the write argument. In Python 3 this is not necessary any more, and the 'u' should be removed to change it to a regular string.

hivetool
--------
* Remove 'from __future__ import'. Indicated by a FIXME.


test_hiveconf.py
----------------
* Clean up imports. 

* For tests of functions that returned different values depending on Python version, change it to only use the Python 3 value. Indicated by FIXME:s and an if-case on the form:
> if str == bytes
* Remove set..list-non-ascii test fully, it's no longer relevant for Python 3. Indicated by a FIXME.

* Unicode strings that contains byte syntax (e.g. \xe5) should be changed to their proper characters. Indicated by FIXME:s.

* Change u""-strings to regular strings.
Comment 26 Niko Lehto cendio 2020-10-27 14:06:40 CET
I get the following output whenever I do a command that access profiles.hconf:
>file:///opt/thinlinc/etc/conf.d/profiles.hconf: line 22: Can't decode from ANSI_X3.4-1968 to unicode.
>file:///opt/thinlinc/etc/conf.d/profiles.hconf: line 23: Can't decode from ANSI_X3.4-1968 to unicode.
>file:///opt/thinlinc/etc/conf.d/profiles.hconf: line 24: Can't decode from ANSI_X3.4-1968 to unicode.
>file:///opt/thinlinc/etc/conf.d/profiles.hconf: line 29: Can't decode from ANSI_X3.4-1968 to unicode.
The commands I used to get this was "l-session-param -a /" and "tl-mount-localdrives -v".
Comment 36 Linn cendio 2020-10-29 16:39:20 CET
Instead of using the system's locale for files, hiveconf now always operates in UTF-8. This is because the .hconf-files shipped with ThinLinc are in UTF-8, and therefore they can't be decoded using a different locale. 

For example, even if ThinLinc is installed on a system with a locale not compatible with UTF-8, we still want to be able to properly read our shipped .hconf-files. If we try to read the file in the system's locale, we will not be able to read it properly. Thus, for ThinLinc's usage of hiveconf, it needs to always use UTF-8.
Comment 38 Linn cendio 2020-10-30 10:10:58 CET
(In reply to Linn from comment #25)
> Here's a list of things we need to clean up once we are ready to drop
> support for Python 2:
> 
> ... 
> 
> test_hiveconf.py
> ----------------
> 
> ...
> 
> * Unicode strings that contains byte syntax (e.g. \xe5) should be changed to
> their proper characters. Indicated by FIXME:s.
The tests that included this have been removed since they were no longer relevant. Ignore this bullet point.
Comment 50 Frida Flodin cendio 2020-11-12 11:33:17 CET
Created attachment 966 [details]
hconf file for printer nearest

Hivetool should be able to drop Python 2 support and run solely in Python 3 by now. 


What I tested so far:
---------------------
Tested on the following latin-1 system:
- Build: 6654
- System: Ubuntu20.04
- Locale: sv_SE.ISO-8859-1
- nearest.hconf: (See attachment)

1. Changed a parameter via tl-config:
> /opt/thinlinc/bin/tl-config/printing/nearest/terminals/01:23:45:67:89:AB/name=göran
   This works as expected and the name is changed to 'göran' in 
   tlwebadm/locations/terminals

   When changing hivetool to run python3 manually by editing the first line in 
   /opt/thinlinc/bin/hivetool to: 
> #!/usr/bin/env python3
   And then running the same command:
> /opt/thinlinc/bin/tl-config/printing/nearest/terminals/01:23:45:67:89:AB/name=göran
   I get 'göran' in webadm.



2. Listing all configuration with hivetool running with Python3:
> /opt/thinlinc/bin/tl-config -Ra /
   I get traceback: 
>    greeting[pt_BR] = Bem-Vindo ao Cendio ThinLinc
>    greeting[ru] = Traceback (most recent call last):
>  File "/opt/thinlinc/bin/../bin/hivetool", line 257, in <module>
>    main()
>  File "/opt/thinlinc/bin/../bin/hivetool", line 252, in main
>    folder.walk(recursive)
>  File "/opt/thinlinc/modules/hiveconf.py", line 746, in walk
>    folder.walk(recursive, indent)
>  File "/opt/thinlinc/modules/hiveconf.py", line 731, in walk
>    print(paramname, "=", param.get_string(), file=indent)
>  File "/opt/thinlinc/modules/hiveconf.py", line 67, in write
>    sys.stdout.write(data)
>UnicodeEncodeError: 'latin-1' codec can't encode characters in position 0-5: ordinal not in range(256)
Comment 51 Frida Flodin cendio 2020-11-12 15:54:28 CET
I did some general testing and did not find any problems, except the previous
comment. Only Web Access runs with Python 3 at the moment, so all other tests are with Python 2. 

On the same system as previous comment, I tested: 

 * Adding terminal and location via webadmin with all names non-ascii. Then 
   activate tl-limit-printers, works fine.

 * Manually editing a .hconf-file, the changes are displayed correctly in 
   tlwebadm.

 * Changing a .hconf-file to latin-1 character encoding, gives traceback as
   expected since hiveconf now only support UTF-8.

 * Profile chooser
 * Desktop customizer from tlwebadm
 * Printing to nearest
 * Printing to thinlocal
 * tl-limit-printer
 * Localdrives
 * Starting session with native client
 * Starting session with Web Access (Web Access runs with Python 3, so this
   tests hiveconfs Python 3 compatibility)
Comment 53 Samuel Mannehed cendio 2020-11-12 17:07:08 CET
(In reply to Frida from comment #50)
> Created attachment 966 [details]
> hconf file for printer nearest
> 
> Hivetool should be able to drop Python 2 support and run solely in Python 3
> by now.

Fixed in r35676.

> What I tested so far:
> ---------------------
> Tested on the following latin-1 system:
> - Build: 6654
> - System: Ubuntu20.04
> - Locale: sv_SE.ISO-8859-1
> - nearest.hconf: (See attachment)
> 
> 1. Changed a parameter via tl-config:
> > /opt/thinlinc/bin/tl-config/printing/nearest/terminals/01:23:45:67:89:AB/name=göran
>    This works as expected and the name is changed to 'göran' in 
>    tlwebadm/locations/terminals
> 
>    When changing hivetool to run python3 manually by editing the first line
> in 
>    /opt/thinlinc/bin/hivetool to: 
> > #!/usr/bin/env python3
>    And then running the same command:
> > /opt/thinlinc/bin/tl-config/printing/nearest/terminals/01:23:45:67:89:AB/name=göran
>    I get 'göran' in webadm.

This is expected behavior since your machine isn't using UTF-8.

> 2. Listing all configuration with hivetool running with Python3:
> > /opt/thinlinc/bin/tl-config -Ra /
>    I get traceback: 
> >    greeting[pt_BR] = Bem-Vindo ao Cendio ThinLinc
> >    greeting[ru] = Traceback (most recent call last):
> >  File "/opt/thinlinc/bin/../bin/hivetool", line 257, in <module>
> >    main()
> >  File "/opt/thinlinc/bin/../bin/hivetool", line 252, in main
> >    folder.walk(recursive)
> >  File "/opt/thinlinc/modules/hiveconf.py", line 746, in walk
> >    folder.walk(recursive, indent)
> >  File "/opt/thinlinc/modules/hiveconf.py", line 731, in walk
> >    print(paramname, "=", param.get_string(), file=indent)
> >  File "/opt/thinlinc/modules/hiveconf.py", line 67, in write
> >    sys.stdout.write(data)
> >UnicodeEncodeError: 'latin-1' codec can't encode characters in position 0-5: ordinal not in range(256)

This is good enough. There is no way we can handle this perfectly. With Python2 you should be getting garbage characters when doing the above. It's arguably more clear to crash like that.

Note that if you use gnome-terminal on your Latin-1 machine the Python2 approach will output correct characters for our hconf files. The reason is that gnome-terminal is bugged and will run in UTF-8 regardless of what locale the system is using. This is easily noticed by running 'apt' or some other program that will output text using the system's locale, you will see garbage characters. Using a terminal like xterm is better in this usecase.
Comment 54 Samuel Mannehed cendio 2020-11-12 17:08:48 CET
The release notes should be updated to reflect that we will not just skip non UTF-8 hconf files. The product REQUIRES it and login will even not work if any hconf file is some other encoding.
Comment 56 Frida Flodin cendio 2020-11-13 10:53:17 CET
(In reply to Samuel Mannehed from comment #53)
> 
> Note that if you use gnome-terminal on your Latin-1 machine the Python2
> approach will output correct characters for our hconf files. The reason is
> that gnome-terminal is bugged and will run in UTF-8 regardless of what
> locale the system is using. This is easily noticed by running 'apt' or some
> other program that will output text using the system's locale, you will see
> garbage characters. Using a terminal like xterm is better in this usecase.

This gnome-terminal bug is also the reason for my first comment about setting 'göran' via tl-config with Python 3. I can verify this by running XTerm and everything works fine now with hivetool in Python 3 (Python 3 automatically decodes stdin to unicode using the system locale).

* The issue is that gnome-terminal always writes to stdin using UTF-8. Then when python 3 tries to read stdin it needs to decode the bytes, and does so by using the system locale, in this case latin-1. The result is garbage characters.

* In python 2, the stdin file never needed to be decoded as Python 2 treats strings as bytes. Recently hivetool ran in python 2 and hiveconf assumed UTF-8. Then it worked when using gnome-terminal since it wrote to stdin with UTF-8 bytes. With XTerm it would be latin-1 bytes in stdin and we got a crash when hiveconf tries to decode to unicode using UTF-8.
Comment 60 Linn cendio 2020-11-18 10:44:51 CET
(In reply to Frida from comment #50)
> 
> ...
> 
> 2. Listing all configuration with hivetool running with Python3:
> > /opt/thinlinc/bin/tl-config -Ra /
>    I get traceback: 
> >    greeting[pt_BR] = Bem-Vindo ao Cendio ThinLinc
> >    greeting[ru] = Traceback (most recent call last):
> >  File "/opt/thinlinc/bin/../bin/hivetool", line 257, in <module>
> >    main()
> >  File "/opt/thinlinc/bin/../bin/hivetool", line 252, in main
> >    folder.walk(recursive)
> >  File "/opt/thinlinc/modules/hiveconf.py", line 746, in walk
> >    folder.walk(recursive, indent)
> >  File "/opt/thinlinc/modules/hiveconf.py", line 731, in walk
> >    print(paramname, "=", param.get_string(), file=indent)
> >  File "/opt/thinlinc/modules/hiveconf.py", line 67, in write
> >    sys.stdout.write(data)
> >UnicodeEncodeError: 'latin-1' codec can't encode characters in position 0-5: ordinal not in range(256)

The error message for this error has been improved, and needs testing. The error can be reproduced by changing locale to e.g. Latin-1 and running:
> /opt/thinlinc/bin/tl-config -Ra /
The terminal should then try to print the hiveconf content in Latin-1, which should cause an error when it reaches a character it cannot interpret.

Testing should also include checking the error message in the log when trying to use a hconf file in non UTF-8. The error message should still say something along the lines of "File X contains non UTF-8 characters."
Comment 62 Frida Flodin cendio 2020-11-18 14:20:28 CET
Tested to generate the error messages and they look good.
Also proofread the release notes.
Closing!
Comment 64 Pierre Ossman cendio 2021-06-17 13:49:26 CEST
This was prematurely closed as we still want to clean out the Python 2 compatibility code.
Comment 69 Frida Flodin cendio 2021-07-09 13:29:34 CEST
The Python 2 support should now be cleaned out. Tested on Fedora 34.

 ✓ tl-setup (setting/getting email)
 ✓ '/opt/thinlinc/bin/tl-config -Ra /'
 ✓ Starting session
 ✓ Configuring nearest printer via tlwebadm
Comment 70 Frida Flodin cendio 2021-07-12 09:56:15 CEST
Created a upstream pull request with our converted Python 3 hiveconf: 
https://github.com/astrand/hiveconf/pull/1
Comment 71 Linn cendio 2021-07-12 14:55:02 CEST
Tested on RHEL 8 and Ubuntu 20.04 with server build 2213 and everything seems to be working well.

 ✓ tl-setup (setting/getting admin email)
 ✓ /opt/thinlinc/bin/tl-config
   ✓ Print all parameters
   ✓ Add a parameter
   ✓ Show value of a parameter
 ✓ Can change the colour of the background during session start
 ✓ Can change default profile via tlwebadm
Comment 72 Linn cendio 2021-07-13 12:25:44 CEST
If we try and print all hiveconf variables with tl-config on a Latin-1 system, we currently crash when we come to the russian characters, which are unavilable in Latin-1:

> cendio@ubuntu2004:~$ tl-config -Ra /
> printing/ 
>     nearest/ 
> profiles/ 
>     default = 
>     order = xterm xfce unity ubuntu gnome gnome-classic kde cinnamon mate lxde
>     show_intro = true
>     greeting = Welcome to Cendio ThinLinc
>     greeting[de] = Willkommen bei Cendio ThinLinc
>     greeting[es] = Bienvenido a Cendio ThinLinc
>     greeting[it] = Benvenuto a Cendio ThinLinc
>     greeting[fr] = Bienvenue chez CendioThinLinc
>     greeting[nl] = Welkom bij Cendio ThinLinc
>     greeting[pt_BR] = Bem-Vindo ao Cendio ThinLinc
>     greeting[ru] = /usr/lib/python3/dist-packages/apport/report.py:13: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
>   import fnmatch, glob, traceback, errno, sys, atexit, locale, imp, stat
> Traceback (most recent call last):
>   File "/opt/thinlinc/modules/hiveconf.py", line 616, in _unicode_to_print
>     print(*args, file=kwargs["file"])
>   File "/opt/thinlinc/modules/hiveconf.py", line 58, in write
>     sys.stdout.write(data)
> UnicodeEncodeError: 'latin-1' codec can't encode characters in position 0-5: ordinal not in range(256)
> 
> During handling of the above exception, another exception occurred:
> 
> Traceback (most recent call last):
>   File "/opt/thinlinc/bin/../bin/hivetool", line 264, in <module>
>     main()
>   File "/opt/thinlinc/bin/../bin/hivetool", line 259, in main
>     folder.walk(recursive)
>   File "/opt/thinlinc/modules/hiveconf.py", line 649, in walk
>     folder.walk(recursive, indent)
>   File "/opt/thinlinc/modules/hiveconf.py", line 634, in walk
>     _unicode_to_print(paramname, "=", param.get_string(), file=indent)
>   File "/opt/thinlinc/modules/hiveconf.py", line 619, in _unicode_to_print
>     raise UnicodeError("Characters in [%s] in %s cannot be printed." \
> hiveconf.UnicodeError: Characters in [/profiles] in file:///opt/thinlinc/etc/conf.d/profiles.hconf cannot be printed.

We shouldn't crash when doing this, and since it is just a print we could e.g. replace any character that can't be presented in the system's locale.
Comment 83 Pierre Ossman cendio 2021-07-20 10:31:09 CEST
Now works properly with unprintable characters. Tested on Ubuntu 20.04 with sv_SE.ISO-8859-15 as the locale:

> ...
>         description[ru] = ??????? ??????? ????? ? ????? ??????????? ??????????. ????????????? ??? ??????? ? ????????????.
>         description[sv] = En minimal miljö med enbart en enda grafisk terminal. Endast tänkt för felsökning och testning.
>         description[tr] = Yaln?zca tek grafiksel terminalin ba?lat?ld??? yal?n bir sistemdir. Yaln?zca hata ay?klama ve test için kullan?lmak amaçl? sa?lanmaktad?r.
> ...

Also tested hiveconf migration as that code was touched as part of this fixed. Upgraded from 4.9.0 on a SLES 12 machine and saw that "admin_email" and "agent_hostname" were both correctly migrated.

Note You need to log in before you can comment on or make changes to this bug.