labscript-suite / runmanager

𝗿𝘂𝗻𝗺𝗮𝗻𝗮𝗴𝗲𝗿 is an intuitive graphical interface for controlling 𝘭𝘢𝘣𝘴𝘤𝘳𝘪𝘱𝘵 𝘴𝘶𝘪𝘵𝘦 experiments. Includes multi-dimensional parameter scans and a remote programming interface for automation.
http://labscriptsuite.org
Other
3 stars 45 forks source link

pywintypes error on close #73

Closed chrisjbillington closed 4 years ago

chrisjbillington commented 4 years ago

Get this when closing runmanager on Windows after running it with python -m

Don't know why this callback is being called when the window is closing, it's supposed to be called when the window is first created.

Traceback (most recent call last):
  File "c:\users\bilbo\desktop\labscript-suite-dev\runmanager.git\runmanager\__main__.py", line 139, in set_win_appusermodel
    set_appusermodel(window_id, appids['runmanager'], icon_path, relaunch_command, relaunch_display_name)
  File "c:\users\bilbo\desktop\labscript-suite-dev\labscript_utils.git\labscript_utils\winshell.py", line 154, in set_appusermodel
    store.SetValue(pscon.PKEY_AppUserModel_ID, propsys.PROPVARIANTType(appid))
pywintypes.com_error: (-2147023496, 'Invalid window handle.', None, None)
philipstarkey commented 4 years ago

That function is called if the win id change event is fired. I suspect either windows or PyQt has changed it's behaviour. Are you on the latest PyQt? There have been other issues with it (see stylesheets issue in BLACS repo)

chrisjbillington commented 4 years ago

I'm on whatever pip's latest PyQt is, so yep.

Occasionally I get another error on startup from pywintypes (within the same callback) saying "the data area passed to the function was too small" or similar (didn't copy it down). I suspect it might be a 32-bit specific bug, because I accidentally installed 32-bit Python (the default download link on python.org!). Gonna try with 64 bit Python.

chrisjbillington commented 4 years ago

Here's the other error:

Traceback (most recent call last):
  File "C:\Users\bilbo\Desktop\labscript-suite-dev\.venv\lib\site-packages\runmanager\__main__.py", line 139, in set_win_appusermodel
    set_appusermodel(window_id, appids['runmanager'], icon_path, relaunch_command, relaunch_display_name)
  File "C:\Users\bilbo\Desktop\labscript-suite-dev\.venv\lib\site-packages\labscript_utils\winshell.py", line 155, in set_appusermodel
    store.SetValue(
pywintypes.com_error: (-2147024774, 'The data area passed to a system call is too small.', None, None)

and it does occur on 64 bit Python just the same. PyQt version was 5.14.2

Perhaps we should just depend on an older PyQt version, but the known-good version 5.9.2 which conda uses is not available on PyPI. 5.12.3 seems no good either (Qt 5.12 is an LTS series so I thought there might be more stability there - but PyQt seems to not ship the later patch releases of it, which go up to 5.12.8).

The function call seem to work: runmanager has an icon in the taskbar, so maybe it's being called correctly at least once, but then also incorrectly.

philipstarkey commented 4 years ago

So that second error is probably unrelated to PyQt I think. I wonder if the relaunch command is too long for some reason? I know that's the opposite of what it implies, but windows is weird...

But the if the first error is happening on exit, then I think either windows or PyQt is firing off a new event that it didn't use to, which triggers our code.

chrisjbillington commented 4 years ago

This comment regarding someone on another project getting the 'data area too small' error implies it might be something about maximum filepath length.

When I installed Python, there was an option to modify the maximum path length in Windows, which I opted for. Not sure how these relate.

''Invalid window handle.' seems more likely to be Qt's fault, but kinda weird to get two unrelated errors in the same function call.

philipstarkey commented 4 years ago

My understanding is that enabling long path lengths on windows is fraught with danger. git has an option for it that I've used successfully, but I've never enabled it for python or system wide.

chrisjbillington commented 4 years ago

I'll try disabling it. The error at startup does look to be a path length issue. It turns out I had accidentally installed a development version to site-packages as an egg, rather than done an editable install. This ended up creating a relaunch command:

"c:\users\bilbo\desktop\labscript-suite-dev\.venv\scripts\pythonw.exe" "C:\Users\bilbo\Desktop\labscript-suite-dev\.venv\Lib\site-packages\labscript_utils-2.15.1.dev43+geb0c05b.d20200502-py3.8.egg\labscript_utils\winlauncher.py" "C:\Users\bilbo\Desktop\labscript-suite-dev\.venv\lib\site-packages\runmanager-2.6.0.dev1-py3.8.egg\runmanager\__main__.py"

which is 352 characters long. It's longer than it would be for a usual install would be because it's installed as an egg that contains the full whacky version number. A regular pip install would not do that, there would just be module directories directly within site-packages.

When I do an editable install, I instead get

"c:\users\bilbo\desktop\labscript-suite-dev\.venv\scripts\pythonw.exe" "C:\Users\bilbo\Desktop\labscript-suite-dev\labscript_utils.git\labscript_utils\winlauncher.py" "c:\users\bilbo\desktop\labscript-suite-dev\runmanager.git\runmanager\__main__.py"

which is only 249 characters long. The max path length in Windows is 260, and I'm not sure if it applies to all of the above, or maybe only to the argument list.

It's still pretty verbose. I could slim down the way paths are passed to winlauncher such as letting you pass a module name instead of the full path to its __main__.py, and I could have winlauncher be at the toplevel in site-packages instead of a submodule of labscript_utils. That would save a bit. But it's pretty annoying.

philipstarkey commented 4 years ago

It's also possible that regardless of long path support, shortcuts are limited to 260 characters. The relaunch command is likely treated as a shortcut internally by Windows.

chrisjbillington commented 4 years ago

It's also possible that regardless of long path support, shortcuts are limited to 260 characters. The relaunch command is likely treated as a shortcut internally by Windows.

Sounds like it could be more complicated than that, but I wouldn't be surprised if the API was limited to 260 characters even if you could in principle crack open the shortcut file on disk and manually edit it to make it longer

chrisjbillington commented 4 years ago

I might investigate the setuptools entry_points argument. This lets you create GUI or command line scripts that point to a specific function in a module somewhere. I've already used it to make the labscript-profile-create script, and it is quite nice.

Basically in Windows this creates at install time actual .exes that run the correct Python.exe or Pythonw.exe for you, importing and running the specified module/function.

My thinking is that we can create such an executable for each GUI app, and then the shortcuts can point directly to the exe, instead of to Python and winlauncher and the script to be run.

Have to think about how this interacts with conda envs, but there's probably something to it.

philipstarkey commented 4 years ago

I was thinking the same thing, but unsure if the conda env needs activating before executing, which might lose a problem. That might explain why Spyder does not do it...

chrisjbillington commented 4 years ago

Python can run without the conda env being active, but it won't be able to import some compiled extensions. So the script will still have to be pointed at winlauncher which will then activate the conda env (by manipulating environment variables) and then run a Python subprocess to run the program as it does now. I think it will work.

rpanderson commented 4 years ago

I get a similar error but of the Unspecified error variety:

Traceback (most recent call last):
  File "c:\users\rpanderson\labscript-testing\runmanager.git\runmanager\__main__.py", line 139, in set_win_appusermodel
    set_appusermodel(window_id, appids['runmanager'], icon_path, relaunch_command, relaunch_display_name)
  File "c:\users\rpanderson\labscript-testing\labscript_utils.git\labscript_utils\winshell.py", line 154, in set_appusermodel
    store.SetValue(pscon.PKEY_AppUserModel_ID, propsys.PROPVARIANTType(appid))
pywintypes.com_error: (-2147467259, 'Unspecified error', None, None)
chrisjbillington commented 4 years ago

Path length issues can be addressed by using entry_points as per https://github.com/labscript-suite/labscript_utils/pull/36. I think that will be resolved once I modify the winshell functions to use the entry point scripts as their relaunch commands.

chrisjbillington commented 4 years ago

Worked out the other one too.

At window close, newer versions of Qt appear to emit a WinIdChange event, but the effectiveWinId() is None! This actually makes sense I guess - the window id stopped being valid, so it lets us know about it.

This is somehow being case to an int within the Qt machinery, and since normally the winid is a sip void pointer, I'm thinking somwhere this gets turned into the memory address of None - which (probably) isn't a valid window...

There was even a warning about invalid casting to int that newer Python have started emitting, so we would have figured this out eventually.

Anyway this seems to fix it, just not emitting the event it if the id is None:

diff --git a/runmanager/__main__.py b/runmanager/__main__.py
index 03e7925..cca9d7b 100644
--- a/runmanager/__main__.py
+++ b/runmanager/__main__.py
@@ -1363,7 +1363,9 @@ class RunmanagerMainWindow(QtWidgets.QMainWindow):
     def event(self, event):
         result = QtWidgets.QMainWindow.event(self, event)
         if event.type() == QtCore.QEvent.WinIdChange:
-            self.newWindow.emit(self.effectiveWinId())
+            winid = self.effectiveWinId()
+            if winid is not None:
+                self.newWindow.emit(int(winid))
         return result

     def paintEvent(self, event):

Just deciding whether to fix this on a per-app basis or to move the whole main window subclass into labscript_utils.qtwidgets since it is common. Still need a way of communicating to it the name of the app that is running though - maybe a class method to set it.

rpanderson commented 4 years ago

Confirming the above patch resolves the unspecified pywintypes.com_error above.