gap-packages / JupyterKernel

Native Jupyter kernel for GAP
https://gap-packages.github.io/JupyterKernel/
BSD 3-Clause "New" or "Revised" License
19 stars 12 forks source link

ErrorInner override is broken in GAP 4.10 #77

Closed fingolfin closed 5 years ago

fingolfin commented 5 years ago

This package replaces ErrorInner with a modified function.

As part of ErrorInner, PRINT_CURRENT_STATEMENT is called. But that went from taking 1 argument in GAP <= 4.9 to taking two arguments in GAP >= 4.10. As a result, if an error is triggered which prints a backtrace, an infinite recursion of Error calls ensues, which (depending on your system) either results in a "recursion depth exceeded" error, or a plain segfault.

The fix for GAP 4.10 is clear, just add the second argument to the call. If you want to write code that works in GAP 4.9 and 4.10, you could check NumberArgumentsFunction(PRINT_CURRENT_STATEMENT) and adapt the call accordingly.

Of course even better would be if ErrorInner was not replaced... As far as I can tell, the only change is to replace "*errout*" by GAP_ERROR_STREAM, which would be trivial to apply to GAP, with BindGlobal("GAP_ERROR_STREAM", "*errout*"); in there; then you'd just have to re-bind GAP_ERROR_STREAM in your package.

CC @alex-konovalov

olexandr-konovalov commented 5 years ago

Is ErrorInner replaced whenever the package is loaded? Would be useful to load it only when the package is loaded via opening a new Jupyter notebook, but not when it is loaded via LoadPackage for testing.

markuspf commented 5 years ago

Note that this is a hack and due to go away "soon" (4.11, pretty please?)

ssiccha commented 5 years ago

Actually, I forgot about my PR #70 which uses the new error handling in GAP 4.10. I've rebased and tested it and it can be merged in now. As markus said, the next step would be to get rid of the hack.

markuspf commented 5 years ago

This is now resolved thanks to @ssicha's work.