Add wx toolkit support #246

mdickinson · 2020-11-25T11:39:23Z

[Code extracted from #173; includes the commits from #245]

Following #231, this PR adds support for the wxPython toolkit, providing wx-specific implementations of the Pinger, Pingee and GuiTestAssistant classes.

Known issues / tasks:

At least one of the examples (using a progress bar) is Qt-specific; we need to either make it work on wx, or have it give a clear error message in that case.
We're getting segfaults with Travis CI on Linux, likely due to some incompatibility between the wxPython-provided wheel and the system that it's running on. I haven't yet found the cause of these, but if we can establish that they're not directly related to the PR, we may want to simply skip that CI job, or mark it as an allowed failure. (To check: are TraitsUI and Pyface running into the same issue? If so, how are they dealing with this?)

ci/__main__.py

traits_futures/wx/gui_test_assistant.py

mdickinson · 2020-11-25T12:34:13Z

Here's what Pyface is doing for wx on Linux: https://github.com/enthought/pyface/blob/0d1516e60051e52251627123891dff9085737f4c/etstool.py#L215-L219

Looks the same as what we have here, except for the version restriction (< 4.1). I'll try that.

…eature/wxpython-support

mdickinson · 2020-11-25T13:08:33Z

The faulthandler output is interesting: it shows clearly that the gui_test_assistant for the null toolkit is being used, indicating that the segfault was likely at import time.

mdickinson · 2020-11-25T14:52:39Z

The pi example fails because of a wxPython 4 compatibility issue in enable (fixed by enthought/enable#403).

Traceback (most recent call last):
  File "examples/pi_iterations.py", line 20, in <module>
    from chaco.api import ArrayPlotData, Plot
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/chaco/api.py", line 38, in <module>
    from .abstract_plot_renderer import AbstractPlotRenderer
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/chaco/abstract_plot_renderer.py", line 7, in <module>
    from .plot_component import PlotComponent
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/chaco/plot_component.py", line 4, in <module>
    from enable.api import Component
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/api.py", line 21, in <module>
    from .markers import MarkerTrait, marker_trait, MarkerNameDict, marker_names, \
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/markers.py", line 18, in <module>
    from .compiled_path import CompiledPath
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/compiled_path.py", line 17, in <module>
    from .toolkit import toolkit_object
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/toolkit.py", line 47, in <module>
    _init_toolkit()
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/toolkit.py", line 35, in _init_toolkit
    __import__(backend)
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/wx/quartz.py", line 14, in <module>
    from .base_window import BaseWindow
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/wx/base_window.py", line 22, in <module>
    from .constants import (
  File "/Users/mdickinson/.edm/envs/traits-futures-py36-wxpython/lib/python3.6/site-packages/enable/wx/constants.py", line 127, in <module>
    wx.WXK_NEXT,
AttributeError: module 'wx' has no attribute 'WXK_NEXT'

mdickinson · 2020-11-25T15:08:23Z

The "<4.1" in "wxPython<4.1" is problematic with EDM on Windows - the command passed to the EDM executable is not quoted properly by the EDM batch file, so it ends up interpreting the "<4.1" as a shell redirect.

One solution is to use an EDM bootstrap environment: then the invocation of edm from within that bootstrap environment doesn't go through a batch file.

We've fixed the same issue for Traits previously in enthought/traits#512

mdickinson · 2020-11-25T17:14:18Z

Still having trouble with the quoting and redirection on Windows. If the "wxPython<4.1" argument is not quoted, we get the following traceback, where the "cannot find the file specified" presumably means that the system is looking for a file called "4.1":

The system cannot find the file specified.
Traceback (most recent call last):
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\projects\traits-futures\ci\__main__.py", line 446, in <module>
    cli(prog_name="python -m ci")
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 717, in main
    rv = self.invoke(ctx)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "C:\projects\traits-futures\ci\__main__.py", line 127, in build
    pyenv.python(wxpython_install_cmd)
  File "C:\projects\traits-futures\ci\python_environment.py", line 252, in python
    return self.run(["python"] + command, capture)
  File "C:\projects\traits-futures\ci\python_environment.py", line 204, in run
    return self.edm(cmd)
  File "C:\projects\traits-futures\ci\python_environment.py", line 72, in edm
    return subprocess.check_call(self._edm_base_command + command)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\subprocess.py", line 311, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['C:\\Users\\appveyor\\.edm\\envs\\bootstrap\\Scripts\\edm.EXE', '--config', 'C:\\projects\\traits-futures\\ci\\data\\edm.yml', 'run', '--environment', 'traits-futures-py36-wxpython', '--', 'python', '-m', 'pip', 'install', 'wxPython<4.1']' returned non-zero exit status 1.

If it is quoted, we get instead:

ERROR: Invalid requirement: '"wxPython<4.1"'
Traceback (most recent call last):
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\projects\traits-futures\ci\__main__.py", line 446, in <module>
    cli(prog_name="python -m ci")
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 717, in main
    rv = self.invoke(ctx)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\site-packages\click\core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "C:\projects\traits-futures\ci\__main__.py", line 127, in build
    pyenv.python(wxpython_install_cmd)
  File "C:\projects\traits-futures\ci\python_environment.py", line 252, in python
    return self.run(["python"] + command, capture)
  File "C:\projects\traits-futures\ci\python_environment.py", line 204, in run
    return self.edm(cmd)
  File "C:\projects\traits-futures\ci\python_environment.py", line 72, in edm
    return subprocess.check_call(self._edm_base_command + command)
  File "C:\Users\appveyor\.edm\envs\bootstrap\lib\subprocess.py", line 311, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['C:\\Users\\appveyor\\.edm\\envs\\bootstrap\\Scripts\\edm.EXE', '--config', 'C:\\projects\\traits-futures\\ci\\data\\edm.yml', 'run', '--environment', 'traits-futures-py36-wxpython', '--', 'python', '-m', 'pip', 'install', '"wxPython<4.1"']' returned non-zero exit status 1.

rahulporuri · 2020-11-25T17:29:19Z

i'm not entirely sure why you're sticking with wxPython < 4.1 because traitsui doesn't place that restriction - https://github.com/enthought/traitsui/blob/b73c475fbf311465c244fe8a1e5b7a3aa16c294f/etstool.py#L254-L263

mdickinson · 2020-11-25T17:41:21Z

i'm not entirely sure why you're sticking with wxPython < 4.1

I'm getting segfaults on Linux with wxPython 4.1, and no segfaults with wxPython < 4.1.

mdickinson · 2020-11-25T19:57:27Z

I'm getting segfaults on Linux with wxPython 4.1, and no segfaults with wxPython < 4.1.

No longer. Thanks @kitchoi for pointing out the packages I was missing!

mdickinson · 2020-11-26T08:28:24Z

traits_futures/tests/test_gui_test_assistant.py

@@ -45,42 +45,58 @@ def tearDown(self):
    def test_run_until_timeout(self):
        # Trait never fired, condition never true.
        dummy = Dummy()
+
+        start_time = time.monotonic()


Note to reviewer: the changes in this file aren't specific to wx, and could be moved to a separate PR. They're regression tests for a bug I discovered in the wx implementation of the GuiTestAssistant.

rahulporuri · 2020-11-26T08:44:07Z

Note to reviewer: the changes in this file aren't specific to wx, and could be moved to a separate PR.

~~@mdickinson is this PR ready for review?~~ on second thought, I might not be the best person to review this PR because there are a lot of wx-specific pieces which look alien to me.

mdickinson · 2020-11-26T08:55:32Z

@rahulporuri Not ready for review yet - I'm trying to replace my own timer with the official Pyface timer, but the official Pyface timer is segfaulting.

mdickinson · 2020-11-26T09:12:41Z

I'm trying to replace my own timer with the official Pyface timer, but the official Pyface timer is segfaulting.

Giving up on this for now, at least for the purposes of this PR (I'll continue to investigate and open an issue in pyface if I find a way to consistently reproduce).

@rahulporuri Yes, it's ready for review! I suspect that the wx-specific pieces are going to look alien to anyone other than Robin Dunn. :-)

mdickinson · 2020-11-26T09:20:58Z

For making the examples work under wx, I'll open a new issue; I don't think it has to happen in this PR. The current status is:

the squares example works
the pi example runs into an Enable issue that's fixed by a pending PR: WxPython 4 Fixes enable#403
the primes example explicitly uses Qt machinery

rahulporuri

LGTM with a few questions/comments. The wx-specific code wasn't too hard to track down/understand.

traits_futures/wx/gui_test_assistant.py

rahulporuri · 2020-11-26T09:37:29Z

traits_futures/wx/pinger.py

+        """
+        Send a ping to the ping receiver.
+        """
+        event = _PingEvent(-1)


Any specific reason why we set event_id to -1? It doesn't matter what integer value we use, IIUC.
Also, this might be overkill but do we want to raise an error if event_id is not an integer? Looks like wx.PostEvent is not thread-safe for event objects having string fields.

It looks as though I don't actually need this at all. I'll remove it.

ci/__main__.py

mdickinson · 2020-11-26T10:06:17Z

do you want to create an issue or add a fixme here about replacing this timer with the pyface timer?

Done. I've opened issues both on the pyface repo and here.

rahulporuri

still LGTM

mdickinson · 2020-11-26T11:38:01Z

traits_futures/wx/pinger.py

+        wx.EvtHandler.__init__(self)
+        self._on_ping = on_ping
+        self._binder = wx.PyEventBinder(_PING_EVENT_TYPE, 1)
+        self.Bind(self._binder, self._on_ping_event)


Having a Bind without a corresponding Unbind bothers me a bit here.

I guess the Unbind call needs to happen during garbage collection so we will need to override Pinger.__del__?

Having a Bind without a corresponding Unbind bothers me a bit here.

Same here, and I'd be tempted to add it back. If there is a reason Unbinding should cause issues, maybe it deserves a comment?

It's not a case of adding it back, because it was never there, and the machinery to support "I'm done with this Pinger" isn't currently there. We'll need a `Pinger.close() method, or something similar.

Given that the lack of unbind isn't actually causing issues, I think I'll leave it as is for this PR, and then add the Pinger.close functionality in a separate PR.

IOW, there's currently no place in the code structure to put that Unbind call. I can fix that with a refactor, but it'll be messier than just adding wx support.

I guess the Unbind call needs to happen during garbage collection so we will need to override Pinger.__del__?

I don't have any evidence that it needs to happen at all, and likely everything that needs to happen is already taken care of at garbage collection time. It's just the lack of the explicit counterpart that bothered me.

And no, I definitely don't want to override __del__. __del__ methods are evil. :-) If we're going to unbind, it should be done at a time of our choosing rather than whenever the g.c. happens to feel like it.

…lear what it does, and we don't seem to need it.

traits_futures/wx/pinger.py

mdickinson · 2020-11-26T12:14:30Z

For making the examples work under wx, I'll open a new issue

Opened #253.

mdickinson · 2020-11-26T12:20:09Z

Thanks for all the review comments! I think the only thing outstanding is the Unbind. I'm going to merge this, then open a PR with a tiny bit of restructuring that lets us call Unbind.

mdickinson · 2020-11-26T12:35:59Z

One last pre-merge change: I removed the note in the documentation about the lack of wxPython support.

mdickinson added 6 commits November 25, 2020 09:28

Add ci support for wxPython installation

494e21d

Revert unrelated change

7f4af8a

Refactor to remove duplication

2b2c8ec

More refactoring and cleanup

0e54170

Add wx toolkit support

7360709

Merge branch 'build/wx-support' into feature/wxpython-support

abd8959

mdickinson commented Nov 25, 2020

View reviewed changes

ci/__main__.py Outdated Show resolved Hide resolved

mdickinson mentioned this pull request Nov 25, 2020

Add ci support for wxPython installation #245

Closed

mdickinson commented Nov 25, 2020

View reviewed changes

ci/__main__.py Outdated Show resolved Hide resolved

mdickinson commented Nov 25, 2020

View reviewed changes

traits_futures/wx/gui_test_assistant.py Show resolved Hide resolved

mdickinson commented Nov 25, 2020

View reviewed changes

traits_futures/wx/gui_test_assistant.py Show resolved Hide resolved

Fix ci install of wxPython to work independently of mode

fc259e9

mdickinson added 3 commits November 25, 2020 12:36

Enable faulthandler when running tests

881d2a9

Merge remote-tracking branch 'origin/feature/wxpython-support' into f…

63807cd

…eature/wxpython-support

FIX: use the given timeout rather than the default timeout for the timer

0265f92

Restrict wxPython to <4.1 to see if that solves the segfault

fad35d5

mdickinson mentioned this pull request Nov 25, 2020

Use the EDM executable instead of the batch file on Windows #247

Merged

mdickinson added 2 commits November 25, 2020 16:39

Merge branch 'master' into feature/wxpython-support

4c8af13

Add quotes around 'wxPython<4.1'

c110f98

mdickinson added 2 commits November 25, 2020 17:54

Add spaces around the '<'

1828f3a

Add some extra packages on Linux to see if it solves the segfault

596af3d

Add version number

e470da4

mdickinson commented Nov 26, 2020

View reviewed changes

rahulporuri marked this pull request as ready for review November 26, 2020 09:14

Remove unnecessary(?) wxPython dependencies

e0ad94f

rahulporuri approved these changes Nov 26, 2020

View reviewed changes

kitchoi reviewed Nov 26, 2020

View reviewed changes

ci/__main__.py Show resolved Hide resolved

Add note about why we're creating our own timer class

ef0b015

Pacify the flake8 check

02e7049

rahulporuri approved these changes Nov 26, 2020

View reviewed changes

Remove event_id argument to _PingEvent constructor

597a7ee

mdickinson commented Nov 26, 2020

View reviewed changes

Also get rid of the expectedIDs argument to PyEventBinder. It's not c…

b8871f3

…lear what it does, and we don't seem to need it.

rahulporuri reviewed Nov 26, 2020

View reviewed changes

traits_futures/wx/pinger.py Outdated Show resolved Hide resolved

mdickinson mentioned this pull request Nov 26, 2020

Make examples work for wxPython #253

Open

Clarify _PING_EVENT_TYPE docstring

4f7e39f

mdickinson mentioned this pull request Nov 26, 2020

Call Unbind in the wxPython Pingee implementation #254

Closed

Remove doc note about lack of wxPython support

bced91b

mdickinson merged commit 1cb1ce3 into master Nov 26, 2020

mdickinson deleted the feature/wxpython-support branch November 26, 2020 12:44

mdickinson mentioned this pull request Nov 26, 2020

Code is currently Qt-specific #55

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wx toolkit support #246

Add wx toolkit support #246

mdickinson commented Nov 25, 2020 •

edited

Loading

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020 •

edited

Loading

rahulporuri commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson Nov 26, 2020

rahulporuri commented Nov 26, 2020 •

edited

Loading

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

rahulporuri left a comment

rahulporuri Nov 26, 2020

mdickinson Nov 26, 2020

mdickinson commented Nov 26, 2020

rahulporuri left a comment

mdickinson Nov 26, 2020

rahulporuri Nov 26, 2020

kitchoi Nov 26, 2020

mdickinson Nov 26, 2020

mdickinson Nov 26, 2020

mdickinson Nov 26, 2020

mdickinson Nov 26, 2020

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

Add wx toolkit support #246

Add wx toolkit support #246

Conversation

mdickinson commented Nov 25, 2020 • edited Loading

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020 • edited Loading

rahulporuri commented Nov 25, 2020

mdickinson commented Nov 25, 2020

mdickinson commented Nov 25, 2020

Choose a reason for hiding this comment

rahulporuri commented Nov 26, 2020 • edited Loading

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

rahulporuri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson commented Nov 26, 2020

rahulporuri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

mdickinson commented Nov 26, 2020

mdickinson commented Nov 25, 2020 •

edited

Loading

mdickinson commented Nov 25, 2020 •

edited

Loading

rahulporuri commented Nov 26, 2020 •

edited

Loading