Bug #1136

Test MH with native windows and native keyboards

Added by Rob Baer almost 2 years ago. Updated almost 2 years ago.

Status:NewStart date:02/23/2017
Priority:NormalDue date:
Assignee:Joel Palmius% Done:

80%

Category:Code correctness
Target version:MakeHuman 1.1.2

Description

Set Locale to Russian on Windows 10. Run the beta for 1.1.1 from a directory named MH-111-B1--åäöüñé or MH-111-B1-ру́сскийязы́к

Beta fails to start with init dialog

The failure occurs quite early before ANY log files (python_err.txt, makehuman.log, makehuman_debug.txt are created). There are no messages as well if started from the command line.

Not sure how to get more information on this. This could be a show stopper

screenshot_1_1487864009.png (37.2 KB) Rob Baer, 02/23/2017 04:36 PM

1348

History

#1 Updated by Rob Baer almost 2 years ago

Probably a false alarm because I don't read Russian or recognize Russian characters. I thought MH-111-B1-ру́сскийязы́к were Russian characters :-(

I renamed the directory MH-111-B1-по традиции which has Russian characters that are not ascii and everything started fine. Thus, the rule seems to be, once again, don't work outside the character set of your own locale, but it is hard to know the exact details.

I think we are good... well, adequate ...

#2 Updated by Rob Baer almost 2 years ago

  • Category set to Code correctness
  • Assignee set to Joel Palmius
  • Priority changed from Normal to High

@joel,

I have bumped the priority on this and assigned it to you given the release target for 1.1.1. We need to be sure that this behavior is at least on your radar. I have no problem with you pushing it forward to next release if you are comfortable that it is beyond the scope of what we think "should work" at this juncture. The minimal goal should be to at least gracefully trap and log such failures.

The mysterious dialog represents a fairly catastrophic failure with no gentle landing or meaningful logging. But, I don't have a clue where in the code things are failing. I don't yet have a Python debugger that I know how to use effectively in the context of MakeHuman and QT. If you push to next release you can lower the priority back to normal as we can assume these will be uncommon errors in the wild.

#3 Updated by Joel Palmius almost 2 years ago

Ok, I'll take a look at it in the weekend.

#4 Updated by Joel Palmius almost 2 years ago

  • Subject changed from Starting Beta-1 build fails in Russian locale - Show stopper? to Test MH with native windows and native keyboards
  • Priority changed from High to Normal
  • Target version changed from MakeHuman 1.1.1 to MakeHuman 1.1.2

I have no way to test this.

Changing the locale and then copy/pasting characters from the net is not a fair test, as chanses are you're pasting character from a foreign encoding (such as utf-8) rather than the current file system code page.

Also, mixing two encodings inside one string, is bound to cause trouble.

The only fair test is to use a native windows and manually typing in path names on a native keyboard. So far, from all I've heard, in such scenarios the crash does not occur. Notably, this works with manually typed chinese characters, as opposed to copy/pasted chinese characters on an european windows. It also works with latin-1 characters (for example the swedish åäö).

However, since I don't have a russian windows with a russian keyboard (and wouldn't be able to use these even if I had access), I can't perform this test for russian specifically.

For now, I'm going to assume that in all natural scenarios, things are working as expected. And that all the crashes we are producing here are due to forcibly creating a circumstance where the file system contains a path name in a code page which is unrecognized by python and/or all console applications because it is foreign to the current windows version.

#5 Updated by Rob Baer almost 2 years ago

  • % Done changed from 0 to 80

Fine. Unfortunately in the US we are way too ignorant of internationalization issues at a practical level.

I note that in Windows 10, you can change locale settings in such a way that you can select between keyboards in the system tray on the lower right. I had the chance to choose for example Swedish or Russian keyboards when I looked at those two locale changes. I don't know if this also required that I had the proper hardware keyboard because I don't know much about how keyboards encode key presses.

We can talk about whether pounding on a US (or Swedish) keyboard with one of those sys tray keyboards selected is "fair" after 1.1.1 is out the door. On the other hand, if we're lucky, we won't need to :)

#6 Updated by Aranuvir # almost 2 years ago

Just two little side notes on this task.
1) Here is the result of sys.path executed inside MakeHuman 1.1.1 Beta:

['./', './lib', './apps', './shared', './apps/gui', './core', 'C:\\Users\\ANY_NAME\\DOWNLO~1\\MAKEHU~1.1-B']

Before reviewing the Python code why it creates those funny dir names, I'd propose to search the bugtracker of pyinstaller.

2) I decided to write a test program to better investigate Python's unicode issues. It took me several days to figure out the, admittedly very complex, code. But finally, I think, it could be shared, now.

The file was stored in C:\ру́сскийязы́к\test.py
Here is the code:

# -*- coding: utf-8 -*-
print(u'This complex code will never run on Pyhon2')

One might call it a slightly adapted "Hello World" program ...
I was NOT able to open the program with IDLE 2.7.13, so I executed it from PyCharm:
C:\Python27\python.exe C:/ру́сскийязы́к/test.py
C:\Python27\python.exe: can't open file 'C:/??�????????�?/test.py': [Errno 22] Invalid argument

Process finished with exit code 2

And that's the reason, why you will never get an error log from MakeHuman...

And here is the result with Python 3.6.0:

"C:\Program Files\Python36\python.exe" C:/ру́сскийязы́к/test.py
This complex code will never run on Pyhon2

Process finished with exit code 0

Of course the file could be opened with IDLE 3.6.0 (and executed as expected).

IMHO, I draw the conclusion, that this issue is outside MakeHuman and the only way to solve it is porting MakeHuman to Python3.

#7 Updated by Rob Baer almost 2 years ago

Thanks for the insight and the inspiration for us to forge ahead with Python 3 conversion.

Looking at your [sys.path result ], I am slightly disturbed by a couple of things:

1) Question: What if you do the sys.path from within your test program with python 3? Does it produce the same result? I guess you would need to import MH getpath to do all the right kinds of file names tests.

2) I ask because Windows should NOT be using 8.3 file names (MAKEHU~1.1-B -shortened file name with a tilde) anymore (unless possibly your windows is XP) but I think I was seeing something in our path handling like this even in the Windows 10 logs. These types of paths are accidents waiting to cause future debug problems.

3) The path base itself in the same path reference uses escaped backslashes rather than the now supported forward slashes. One sees this when one looks at our current log files that we are inconsistent in using linux-style paths within the program. It's not so much the we use the cumbersome escaped version, but that we use it sometimes but not other times. This suggests that we need to create re-usable code somewhere to replace redundancy.

One of the things that occurs to me every time I look at your code snippets, is your habit of being explicit with string literals as being unicode. Of course, this is necessary in Python 2, but it seems still supported in Python 3. I'm not sure how many string literals are strewn out across the MakeHuman code, but employing your explicit unicode practice might help us end up with more robust code in the long run.

#8 Updated by Aranuvir # almost 2 years ago

ad 1): Now this gets really a little complicated :-D. As far as I can tell the 8.3-filenames are a problem of Python2 and pyinstaller. I haven't used pyinstaller, yet. And to do your proposed testing creating a test.exe with pyinstaller would be necessary. Will see, if I could test it the next days ...

ad 2): IIRC, makehuman.py does some magic to sys.path and, I guess, the odd filename is from pyinstaller. To be more precise: the problem seems to be within MakeHuman's getCwd(), which on isBuild() returns sys.executable.
Here is some explanation:

codewarrior0 commented on 27 May 2015:

Yes, this should be documented as a difference between running in normal Python and running under PyInstaller. PYI explicitly uses GetShortPathName to set the value of 
sys.executable with an 8.3 format path as its directory component - "to overcome the Python and PyInstaller limitation to run with foreign characters in directory names." 
(pyi_path.c:157)

In normal Python, sys.executable contains the full path to argv[0]. On Windows, argv[0] is encoded using an MS-DOS codepage. Renaming python.exe to something like 
pyxěščřžýáíé.exe (or putting it in a folder with that name) will result in a worthless sys.executable and the only way to get a good one is with the Win32 API 
GetCommandLineW for getting the command line as wide characters (i.e. UTF-16 encoded).

However, there is a bug(?) in pywin32 where there is no way to call GetCommandLineW. Calling win32api.GetCommandLine always calls the ANSI variant which returns the 
program path encoded with a codepage - the same thing you get from sys.executable in normal Python. So you actually have to go through ctypes to call GetCommandLineW.

I want to be able to change sys.executable to always return a usable filename, but this isn't possible on Windows with Python 2.7. sys.executable is required to be a 
bytes type, so we can't just change it to a unicode. Also, python's filesystem functions will use the A variants of Windows APIs - which expect a codepage encoding (mbcs) 
whenever they are passed a bytes type, so we can't change sys.executable to be UTF-16 encoded either.

(On Python 3.3, sys.executable is a unicode type, but I didn't check if it actually contains valid data for the odd filename above.)

It looks like the current behavior of using an MS-DOS 8.3 ShortFileName is the best we can do for now. It has one shortcoming though. The path to the .exe's folder is 
encoded as an SFN, but the filename of the .exe itself is not - it's codepage-encoded as it comes from argv.


Source: https://github.com/pyinstaller/pyinstaller/issues/1282

I haven't evaluated the comment above, if we could try to make sys.path look nicer. Probably doing cosmetics here is superfluous. The path was just posted because it looked so weird in 2017, reminding me of Win95.

My intended take-home massage was Python2 does not like every unicode pathname as an argument to execute ...

ad post 3): If I got this correctly, Python3 declares every string to unicode, if it is not explicitly declared to something else. So the 'u' is very likely omittable in Python3 and just emphasizing the intention of the programmer (which sometimes could be helpful, see: those f***ing division changes)

#9 Updated by Rob Baer almost 2 years ago

YIKES - so pyinstaller has its own special traps for getting this cleaned up once and for all. :-(

Aranuvir wrote:

ad post 3): If I got this correctly, Python3 declares every string to unicode, if it is not explicitly declared to something else. So the 'u' is very likely omittable in Python3 and just emphasizing the intention of the programmer (which sometimes could be helpful, see: those f***ing division changes)

Exactly! And readable code is maintainable code. If there had been a little more care to telegraph the floating vs integer division intent in some places, it sure would make that easier to port. ;-)

With respect to characters, if I'm right, we will still need ascii byte characters for some of the opengl stuff to work properly, so telegraphing where we explicitly mean bytes and where we mean the default unicode will make finding mistakes much easier at a later date!!! Couldn't hurt to be explicit where possible and not too burdensome even though unicode IS the default.

#10 Updated by Joel Palmius almost 2 years ago

My current impression is that fixing these problems in the current py2 branch would require a lot more effort than getting the py3 branch up to speed. Barring unforeseen compatibility problems in dependencies.

Also available in: Atom