Cysignals for windows #76

vinklein · 2018-03-15T14:40:21Z

This PR is a proposal for issue #63. For feedback and advices.

The core of this PR is based on cypari fork of cysignals.

Continuous integration is working in four version python versions Python27, Python27-64bits, Python34 and Python34-64bits (build #186 for example).

Some notes / remaining questions :

I don't have managed yet to have working with python35 and python36.
test_helper.c : signal_pid_after_delay use fork. There is no real equivalent to "fork" in windows. I have not find what the best way to manage this for windows.
alarm.pyx: Not translated because SIGALRM doesn't exist on Windows.
Error with python 35
macro.h : I don't clearly understand the role of win32ctrlc
tests.pyx CYSIGNALS_CRASH_QUIET doesn't work yet on windows. Fixed

jdemeyer · 2018-03-15T14:55:20Z

One immediate major comment: I don't like code of the form

/* Posix version */
#if !defined(__MINGW32__) && !defined(_WIN32)
....
#else /* Windows version */
....
#endif

where the "POSIX version" and "Windows version" are mostly the same but not really. You really should avoid that.

jdemeyer · 2018-03-15T14:56:28Z

setup.py

 ]

+scripts = []
+
+if os.name is not 'nt':


This is very bad. Compare strings with !=

jdemeyer · 2018-03-15T15:00:14Z

rundoctests.py

+        # side effects from doctests.
+        p = Process(target=testfile, args=(f,))
+        p.start()
+        p.join(200)


Maybe to be safer, increase the timeout to 10 minutes or so. And better be explicit and use join(timeout=600). You also need to handle the case of timeout.

jdemeyer · 2018-03-15T15:01:30Z

src/cysignals/signals.pyx

    void setup_cysignals_handlers() nogil
    void print_backtrace() nogil
    void _sig_on_interrupt_received() nogil
    void _sig_on_recover() nogil
    void _sig_off_warning(const char*, int) nogil
-
+    IF UNAME_SYSNAME != 'Windows':


No need for that. I doesn't hurt to declare functions which do not exist.

jdemeyer · 2018-03-15T15:01:58Z

src/cysignals/signals.pyx

    elif sig == SIGSEGV:
        if msg is NULL:
            msg = "Segmentation fault"
        PyErr_SetString(SignalError, msg)
-    else:
-        raise SystemError(f"unknown signal number {sig}")
+    IF UNAME_SYSNAME != 'Windows':


No need for that.

Can you please be more explicit on this one ?

I am just saying that you should try to replace the code

IF UNAME_SYSNAME == 'Windows': X ELSE: Y

by

X Y

(i.e. remove the conditions)

jdemeyer · 2018-03-15T15:02:44Z

src/cysignals/signals.pyx

@@ -193,22 +205,23 @@ def init_cysignals():
    import signal
    old = signal.signal(signal.SIGINT, python_check_interrupt)

-    setup_alt_stack()
+    IF UNAME_SYSNAME != 'Windows':
+        setup_alt_stack()


No need for that.

This one cause an undefined reference to 'setup_alt_stack' if removed

This one cause an undefined reference to 'setup_alt_stack' if removed

Yes of course. But then you just implement setup_alt_stack to do nothing on Windows. This is related to the "major comment" that you should try to implement things as much as possible the same for POSIX and Windows.

Ok i get the idea. Thx.

jdemeyer · 2018-03-15T15:02:52Z

src/cysignals/signals.pyx

-    feature...
-    """
-    setup_alt_stack()
+IF UNAME_SYSNAME != 'Windows':


No need for that.

undefined reference to 'setup_alt_stack' if removed

jdemeyer · 2018-03-15T15:04:12Z

rundoctests.py

+flag_skip_posix = OPTIONFLAGS_BY_NAME["SKIP_POSIX"]
+
+
+class WindowsSkipDocTestParser(DocTestParser):


Why Windows in the name here? You handle SKIP_WINDOWS as well as SKIP_POSIX.

You are right. I should have rename after adding SKIP_POSIX.

jdemeyer · 2018-03-16T09:00:38Z

I don't have managed yet to have working with python35 and python36.

Can you at least enable tests such that we can follow up on the errors?

jdemeyer · 2018-03-16T09:02:15Z

Can you explain why you need all the patches in appveyorutil?

jdemeyer · 2018-03-16T09:03:06Z

example/setup.py

+# Option "-D_hypot=hypot" is mandatory for mingw64
+extensions = [Extension('cysignals_example',
+                        ['cysignals_example.pyx'],
+                        extra_compile_args=['-D_hypot=hypot'])]


What's up with hypot? The example code doesn't involve hypot at all, so why is this needed?

short story :

In file included from c:/mingw-w64/x86_64-7.2.0-posix-seh-rt_v5-rev1/mingw64/lib/gcc/x86_64-w64-mingw32/7.2.0/include/c++/math.h:36:0, from c:\Python27-x64\include/pyport.h:325, from c:\Python27-x64\include/Python.h:58, from cysignals_example.cpp:24: c:/mingw-w64/x86_64-7.2.0-posix-seh-rt_v5-rev1/mingw64/lib/gcc/x86_64-w64-mingw32/7.2.0/include/c++/cmath:1136:11: error: '::hypot' has not been declared using ::hypot;

Long story :
https://ci.appveyor.com/project/vinklein/cysignals/build/1.0.194

That is likely a bug in mingw64.

Can you send me the pre-processed source file (created with -save-temps)?

And maybe also the file c:/mingw-w64/x86_64-7.2.0-posix-seh-rt_v5-rev1/mingw64/lib/gcc/x86_64-w64-mingw32/7.2.0/include/c++/cmath

It's actually a Python bug: python/cpython#880

In master, I pushed a commit to stop using <math.h> in the example. Hopefully the issue will be gone then.

I don't know why but it look like <math.h> is still called (https://ci.appveyor.com/project/vinklein/cysignals/build/job/no6rwrud4sojkauv).

jdemeyer · 2018-03-16T09:05:49Z

src/cysignals/implementation.c

@@ -4,7 +4,8 @@ Interrupt and signal handling for Cython

 /*****************************************************************************
 *       Copyright (C) 2006 William Stein <wstein@gmail.com>
- *                     2006-2016 Martin Albrecht <martinralbrecht+cysignals@gmail.com>
+ *                     2006-2016 Martin Albrecht <martinralbrecht+cysignals@gmail.com


Don't change this line

jdemeyer · 2018-03-16T09:06:04Z

src/cysignals/implementation.c

@@ -4,7 +4,8 @@ Interrupt and signal handling for Cython

 /*****************************************************************************
 *       Copyright (C) 2006 William Stein <wstein@gmail.com>
- *                     2006-2016 Martin Albrecht <martinralbrecht+cysignals@gmail.com>
+ *                     2006-2016 Martin Albrecht <martinralbrecht+cysignals@gmail.com
+ *                     2016 Marc Culler and Nathan Dunfield
 *                     2010-2018 Jeroen Demeyer <J.Demeyer@UGent.be>


Feel free to add your own name here

jdemeyer · 2018-03-16T09:07:02Z

src/cysignals/implementation.c

@@ -21,6 +22,23 @@ Interrupt and signal handling for Cython
 * along with cysignals.  If not, see <http://www.gnu.org/licenses/>.
 *
 ****************************************************************************/
+#define ENABLE_DEBUG_CYSIGNALS 0
+#if ENABLE_DEBUG_CYSIGNALS
+#define DEBUG(...) fprintf(stderr, __VA_ARGS__); fflush(stderr);


Don't do this. Use cysigs.debug_level instead.

If i understand correctly the different debug_levels are here to indicate how much details we want to be printed rather than a trace category like WARNING, INFO DEBUG ?

rather than a trace category like WARNING, INFO DEBUG ?

I consider it as various levels of debug information. It's more like DEBUG_A_LITTLE, DEBUG_SOME_MORE and DEBUG_A_LOT.

jdemeyer · 2018-03-16T09:13:00Z

src/cysignals/implementation.c

+}
+
+__PYX_EXTERN_C DL_EXPORT(int) sig_raise_exception(int, char const *);
+//extern int sig_raise_exception(int sig, const char* msg);


What's wrong with this?

jdemeyer · 2018-03-16T09:14:19Z

src/cysignals/macros.h

@@ -92,8 +92,11 @@ extern "C" {
 * if the first returns 0).
 */

-#define _sig_on_(message) ( unlikely(_sig_on_prejmp(message, __FILE__, __LINE__)) || _sig_on_postjmp(cysetjmp(cysigs.env)) )
-
+#if defined(__MINGW32__) || defined(_WIN32)


No need for this. Just #define cysetjmp appropriately.

jdemeyer · 2018-03-16T09:19:47Z

Am I understanding correctly that you are still using the same configure script on Windows? That would be good because you could add tests to configure for certain features which are missing on Windows, such as sigaction and sigaltstack. That way, you could use #if HAVE_SIGACTION instead of checking for things like __MINGW32__.

jdemeyer · 2018-03-16T09:24:40Z

It would be good to (eventually) port alarm.pyx and pysignals.pyx also to Windows. I think that should not be too hard. But I understand that you don't want to do this right now.

For now, the most important thing is to clean up and reorganize the code. Feel free to write wrapper functions where needed, for example to hide the difference between signal and sigaction (similar to the use of cysetjmp to hide the difference between setjmp and sigsetjmp).

vinklein · 2018-03-16T09:47:23Z

Can you explain why you need all the patches in appveyorutil?

You mean in a documentation or here ?
The patchs are for ditsutils to know what compiler to use. Microsoft compiler would be used be default without this.

jdemeyer · 2018-03-16T09:52:10Z

The patchs are for ditsutils to know what compiler to use. Microsoft compiler would be used be default without this.

Where did you get those patches from? Is it possible to configure distutils to use the mingw compiler without patching?

The problem with patching on appveyor is that cysignals won't work on real Windows machines, it will only work on appveyor.

vinklein · 2018-03-16T10:28:54Z

I didn't get them form some place i written them based after finding this stackoverflow Python 3.4: compile cython module for 64-bit windows. They are just git diff with vanilla versions of distutils.

The problem with patching on appveyor is that cysignals won't work on real Windows machines, it will only work on appveyor.

This is not really a problem as the patchs are not appveyor specific. The wiki page How-to-install-cysignals-on-Windows is not up to date but the idea is here.

jdemeyer · 2018-03-16T10:38:29Z

I didn't get them form some place

I seems to me that your patches come from https://bugs.python.org/issue11723

jdemeyer · 2018-03-16T10:41:01Z

It also seems like you should be able to monkey-patch distutils in setup.py to add those compilers. That would be much better than patching distutils itself.

vinklein · 2018-03-16T10:54:51Z

I seems to me that your patches come from https://bugs.python.org/issue11723

Yes via the stackoverflow mentioned above. I don't mean i invented the solution.

vinklein · 2018-03-16T10:56:14Z

It also seems like you should be able to monkey-patch distutils in setup.py to add those compilers. That would be much better than patching distutils itself.

I will look at it.

jdemeyer · 2018-03-16T11:12:05Z

Makefile

-# This was discovered at https://github.com/sagemath/cysignals/issues/71
-# but I don't know a good analysis nor solution.
-DOCTEST = ulimit -s 1024 && $(PYTHON) -B rundoctests.py
+ifeq ($(OS),Windows_NT)


I just pushed a commit to master which hopefully makes these changes to Makefile unneeded.

It works for removing LN_SITE_PACKAGE.
But ulimit don't exist on windows.
I will push the rebase after some others tests

But ulimit don't exist on windows.

OK, so you get an error but the tests continue, right?

I will push the rebase after some others tests

Maybe squash your commits. It's not really helpful to have a lot of commits, especially when some just revert earlier commits.

OK, so you get an error but the tests continue, right?

Yes ulimit don't look to be blocking, but i have another error crashing distcheck with my current rebase.

Maybe instead of doing these kind of hacks, we could just say that one should run make distcheck on Windows. I think that it's fine if we run make distcheck on at least one platform, but not all.

jdemeyer · 2018-03-16T13:29:51Z

src/cysignals/macros.h

@@ -276,10 +304,21 @@ static inline void sig_error(void)
    {
        fprintf(stderr, "sig_error() without sig_on()\n");
    }
+#if defined(__MINGW32__) || defined(_WIN32)
+    /*
+     * The Windows abort function will terminate the process no


Does the abort() function terminate the process or does the SIGABRT signal abort the process? If only abort() really aborts, then you could use raise(SIGABRT) instead.

jdemeyer · 2018-12-21T11:30:59Z

src/cysignals/implementation.c

+                    fprintf(stderr, "Incremented win32ctrlc to %d\n", win32ctrlc);
+                }
+#endif
+            return;


Shouldn't you raise(SIGFPE) here?

jdemeyer · 2018-12-21T11:32:08Z

src/cysignals/implementation.c

    sig_atomic_t inside = cysigs.inside_signal_handler;
    cysigs.inside_signal_handler = 1;

    if (inside == 0 && cysigs.sig_on_count > 0 && sig != SIGQUIT)
+#else
+    int inside = 0; // not used for windows
+    if(cysigs.sig_on_count > 0)


Is there a particular reason that you are not using inside_signal_handler on Windows?

jdemeyer · 2018-12-21T11:33:51Z

src/cysignals/implementation.c

+                if(cysigs.debug_level >=1)
+                    fprintf(stderr,  "Mapped from %d\n", cysigs.sig_mapped_to_FPE);
+#endif
+                int mapped_sig = cysigs.sig_mapped_to_FPE;


Simplify this as sig = cysigs.sig_mapped_to_FPE;

jdemeyer · 2018-12-21T11:45:06Z

Looking again at some Windows documentation, I'm not convinced that the sig_mapped_to_FPE machinary is really needed for any signal besides SIGINT. The documentation at https://docs.microsoft.com/nl-nl/cpp/c-runtime-library/reference/signal?view=vs-2017 says

SIGINT is not supported for any Win32 application. When a CTRL+C interrupt occurs, Win32 operating systems generate a new thread to specifically handle that interrupt. This can cause a single-thread application, such as one in UNIX, to become multithreaded and cause unexpected behavior.

but it only says that for SIGINT. And it seems to me that it's not so much about the signal number (SIGINT or SIGFPE or whatever) but more about how it was raised (by pressing CTRL-C, by calling raise(), by an actual floating-point exception occuring).

So maybe you are overcomplicating things with the sig_mapped_to_FPE. You might even be able to just raise(SIGINT) from the SIGINT-handling thread.

jdemeyer · 2018-12-21T11:50:30Z

That page also says

Do not use longjmp unless the interrupt is caused by a floating-point exception (that is, sig is SIGFPE).

but it doesn't really say why. For SIGINT, it's clear because another thread is involved. But for the other signals, we don't know why it shouldn't work.

vinklein · 2018-12-21T13:55:25Z

OK, new attempt. Does this work:

diff --git a/example/cysignals_example.pyx b/example/cysignals_example.pyx
index ef3a3b5..414b7a1 100644
--- a/example/cysignals_example.pyx
+++ b/example/cysignals_example.pyx
@@ -1,5 +1,8 @@
 # distutils: language = c++
 # cython: language_level = 3
+#
+# The next line works around https://github.com/python/cpython/pull/880
+# distutils: define_macros = _hypot=hypot
 
 from cysignals.signals cimport sig_check
 from cysignals.memory cimport check_allocarray

I'm asking because this looks like a simpler solution than changing setup.py.

Yes it does work !

vinklein · 2018-12-21T15:10:04Z

It seems to me that you are disabling almost all tests on Windows. That's not acceptable. I certainly don't mind to disable the more advanced tests, but basic things like handling SIGINT should still be tested.

The most of disabled tests seems to be that way because of signal_after_delay or signals_after_delay usage. Since i now have an implementation for signal_after_delay, i will try to enable the one using this function.

vinklein · 2019-01-07T14:40:18Z

I suggest this to deal with the sigsetjmp compatibility:

diff --git a/src/cysignals/implementation.c b/src/cysignals/implementation.c
index a422f62..be904f9 100644
--- a/src/cysignals/implementation.c
+++ b/src/cysignals/implementation.c
@@ -83,6 +83,11 @@ static sigset_t default_sigmask;
 
 /* default_sigmask with SIGHUP, SIGINT, SIGALRM added. */
 static sigset_t sigmask_with_sigint;
+#else
+/* If we don't have sigprocmask(), assume that we don't have siglongjmp() either */
+#define sigjmp_buf jmp_buf
+#define sigsetjmp(env, savesigs) setjmp(env)
+#define siglongjmp longjmp
 #endif
 
 /* A trampoline to jump to after handling a signal. */

Currently (in windows_dev branch) siglongjmp and sigjmp_buf are defined in struct_signals.h do you think it is more consistent to have that in implementation.c ?

jdemeyer · 2019-01-07T15:04:49Z

Currently (in windows_dev branch) siglongjmp and sigjmp_buf are defined in struct_signals.h do you think it is more consistent to have that in implementation.c ?

I would rather not have it in struct_signals.h since that's an installed header. So it would affect everything using cysignals.

vinklein · 2019-01-07T16:49:20Z

General comment: is there a particular reason that you are not using the trampoline jump? I can understand that this is not really required on Windows, but for consistency it would be good to use it.

setup_trampoline use pthread_attr_setstack which doesn't exist on windows system, i am looking for a workaround (https://stackoverflow.com/questions/35962507/can-i-create-a-thread-with-its-stack-at-a-specific-address).

jdemeyer · 2019-01-09T12:56:43Z

setup_trampoline use pthread_attr_setstack which doesn't exist on windows system, i am looking for a workaround (https://stackoverflow.com/questions/35962507/can-i-create-a-thread-with-its-stack-at-a-specific-address).

Just commenting here: the reason that I'm using pthread_attr_setstack is because the thread stack should not be deallocated when the thread exists. If there is any way in Windows to keep the thread stack alive after the thread exists, that would be fine too.

vinklein · 2019-01-11T14:40:12Z

Rebase on 582dbf6 and many modifications :

Work around for hypot problem [2.7] bpo-11566: Extension build errors on Windows for _hypot python/cpython#880
Fix indentation and whitespace errors.
Remove #define PTHREAD_STACK_MIN 65536
Remove unecessary #include <float.h>
simplify "int mapped_sig =" with "sig ="
define sigjmp, sigsetjmp and siglongjmp in implementation.c

Some remaining issues:

Having a working signal_after_delay implementation on windows. I am still trying to do that. The firsts implementation tested is using thread to raise the signal. For now i have inconsistent results :
raise(SIGINT) by a test program do what is expected but with signal_after_delay it's like if nothing is called.
Concerning SIGABRT

>>> from cysignals.tests import *
>>> test_abort

works on windows but running test_signal_abrt() will stop the python process. In theory raise(SIGABRT) is called in both cases.

siglongjmp and trampoline usage instead of reset_CPU/longjmp (pthread_attr_setstack issue).
win32ctrlc usefulness ?
Try to run make distcheck on at least one platform but not all in order to remove the ulimit -s 1024 trick

vinklein · 2019-01-11T15:00:00Z

Does Windows have anything like the kill or pthread_kill function? Is there a way to send a signal to a different process or different thread? I am trying to write a portable version of signal_pid_after_delay but I need to know what Windows supports.

For what i know there is no clean way to do that (look at terminateprocess and terminatethread), the existing functions can cause resources to not be released.

vinklein · 2019-01-15T17:29:46Z

Rebase on 50ba45a (Wich resolve the signal_after_delay issue)
Map SIGHUP, SIGBUS, SIGALRM, SIGQUIT with existing signals.
Remove +SKIP_WINDOWS for tests using signal_after_delay with windows compatible signals
Remove kill_thread_group declaration

vinklein · 2019-01-15T17:36:37Z

One point to clarify : running test_sig_retry_and_signal currently provoke a timeout on windows.

jdemeyer · 2019-01-16T10:20:12Z

src/cysignals/tests.pyx

@@ -165,19 +165,19 @@ def interrupt_after_delay(ms_delay=500):
    """
    signal_after_delay(SIGINT, ms_delay)

+IF UNAME_SYSNAME != 'Windows':


This should not be needed anymore

jdemeyer · 2019-01-16T10:20:40Z

src/cysignals/tests.pyx

@@ -639,7 +639,14 @@ def unguarded_abort():
    We run Python in a subprocess and make it call abort()::

        >>> from cysignals.tests import subpython_err
-        >>> subpython_err('from cysignals.tests import *; unguarded_abort()')
+        >>> subpython_err('from cysignals.tests import *; unguarded_abort()')  # doctest: +SKIP_WINDOWS


Why is this test duplicated?

jdemeyer · 2019-01-16T10:22:11Z

* Fix indentation and whitespace errors.

You are still introducing various needless whitespace changes, please have a look at the diff.

vinklein · 2019-01-16T10:25:48Z

* Fix indentation and whitespace errors.
You are still introducing various needless whitespace changes, please have a look at the diff.

Yes. Some have already been fixed under vinklein:appveyor_test branch

Modify cysignals for windows compatibility

jdemeyer · 2019-02-12T17:36:11Z

For easier testing, I copied this to a branch windows in cysignals. See #104.

jdemeyer changed the title ~~Cysignals for windows. Proposal for issue #63~~ Cysignals for windows Mar 15, 2018

jdemeyer reviewed Mar 15, 2018

View reviewed changes

setup.py Outdated

]

scripts = []

if os.name is not 'nt':

Copy link

Collaborator

jdemeyer Mar 15, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very bad. Compare strings with !=

jdemeyer reviewed Mar 15, 2018

View reviewed changes

jdemeyer mentioned this pull request Mar 16, 2018

Stub implementation for Windows? #30

Closed

jdemeyer reviewed Mar 16, 2018

View reviewed changes

jdemeyer mentioned this pull request Mar 16, 2018

[2.7] bpo-11566: Extension build errors on Windows for _hypot python/cpython#880

Merged

jdemeyer reviewed Mar 16, 2018

View reviewed changes

jdemeyer reviewed Dec 21, 2018

View reviewed changes

vinklein force-pushed the windows_dev branch from 062650c to 7d4a8e5 Compare January 11, 2019 14:39

vinklein force-pushed the windows_dev branch from 7d4a8e5 to 0aec15e Compare January 15, 2019 17:26

vinklein force-pushed the windows_dev branch from 0aec15e to 24ef608 Compare January 15, 2019 17:31

vinklein force-pushed the windows_dev branch from 24ef608 to 46a246d Compare January 16, 2019 09:39

jdemeyer reviewed Jan 16, 2019

View reviewed changes

vinklein force-pushed the windows_dev branch from 3944012 to 342c1b5 Compare January 17, 2019 15:16

jdemeyer mentioned this pull request Jan 17, 2019

Cygwin builds on AppVeyor #102

Merged

vinklein force-pushed the windows_dev branch from f03011b to 6468177 Compare January 18, 2019 10:41

Cysignals for windows

64d07c4

Modify cysignals for windows compatibility

vinklein force-pushed the windows_dev branch from 6468177 to 64d07c4 Compare January 18, 2019 12:36

jdemeyer closed this Feb 12, 2019

		flag_skip_posix = OPTIONFLAGS_BY_NAME["SKIP_POSIX"]


		class WindowsSkipDocTestParser(DocTestParser):

Cysignals for windows #76

Cysignals for windows #76

Conversation

vinklein commented Mar 15, 2018 • edited Loading

jdemeyer commented Mar 15, 2018

jdemeyer Mar 15, 2018 • edited Loading

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

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

jdemeyer commented Mar 16, 2018

jdemeyer commented Mar 16, 2018

Choose a reason for hiding this comment

vinklein Mar 16, 2018 • edited by jdemeyer Loading

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

vinklein Mar 20, 2018 • edited Loading

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

jdemeyer Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdemeyer commented Mar 16, 2018

jdemeyer commented Mar 16, 2018

vinklein commented Mar 16, 2018

jdemeyer commented Mar 16, 2018

vinklein commented Mar 16, 2018 • edited Loading

jdemeyer commented Mar 16, 2018

jdemeyer commented Mar 16, 2018

vinklein commented Mar 16, 2018

vinklein commented Mar 16, 2018

Choose a reason for hiding this comment

vinklein Mar 16, 2018 • edited Loading

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

Choose a reason for hiding this comment

jdemeyer commented Dec 21, 2018

jdemeyer commented Dec 21, 2018

vinklein commented Dec 21, 2018 • edited Loading

vinklein commented Dec 21, 2018

vinklein commented Jan 7, 2019

jdemeyer commented Jan 7, 2019

vinklein commented Jan 7, 2019 • edited Loading

jdemeyer commented Jan 9, 2019

vinklein commented Jan 11, 2019

vinklein commented Jan 11, 2019

vinklein commented Jan 15, 2019

vinklein commented Jan 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdemeyer commented Jan 16, 2019

vinklein commented Jan 16, 2019 • edited Loading

jdemeyer commented Feb 12, 2019

vinklein commented Mar 15, 2018 •

edited

Loading

jdemeyer Mar 15, 2018 •

edited

Loading

vinklein Mar 16, 2018 •

edited by jdemeyer

Loading

vinklein Mar 20, 2018 •

edited

Loading

jdemeyer Mar 19, 2018 •

edited

Loading

vinklein commented Mar 16, 2018 •

edited

Loading

vinklein Mar 16, 2018 •

edited

Loading

vinklein commented Dec 21, 2018 •

edited

Loading

vinklein commented Jan 7, 2019 •

edited

Loading

vinklein commented Jan 16, 2019 •

edited

Loading