Make error output use the same stream consistently #2529

ssiccha · 2018-06-07T11:11:56Z

PRINT_CURRENT_STATEMENT now takes a stream as additional input.
Adjust its tests accordingly.
Make ErrorInner, Where, and WHERE consistently pass "errout"
to print functions

ssiccha · 2018-06-07T11:19:59Z

I just realized that Where and WHERE also should take a stream ad their first argument. But OnBreak is bound to Where. I'll also have to adjust calls to OnBreak then.

ssiccha · 2018-06-07T11:28:09Z

OnBreak is documented to have only one argument though. I will define it as function() Where("errout"); end;

sebasguts · 2018-06-07T12:55:55Z

lib/error.g

@@ -204,7 +204,7 @@ BIND_GLOBAL("ErrorInner",
    if printThisStatement then 
        if context <> GetBottomLVars() then
            PrintTo("*errout*"," in\n  \c");
-            PRINT_CURRENT_STATEMENT(context);
+            PRINT_CURRENT_STATEMENT("*errout*", context);
            Print("\c");


Shouldn't this also be printed to *errout*?

Ofc. Thanks :)

ssiccha · 2018-06-07T13:28:54Z

And Where is also documented. I'll let them take a stream ad an optional first argument then.

fingolfin · 2018-06-07T13:53:55Z

I actually don't think it's that important or useful for Where to have a stream argument. Do you really need that for jupyter?

fingolfin · 2018-06-07T13:54:28Z

lib/error.g

-            PRINT_CURRENT_STATEMENT(context);
-            Print("\c");
+            PRINT_CURRENT_STATEMENT("*errout*", context);
+            PrintFto("*errout*", "\c");


Typo: PrintFto

fingolfin · 2018-06-07T13:55:16Z

src/error.c

+    int openedOutput = 1;
+    if ((IsStringConv(stream) && !OpenOutput(CSTR_STRING(stream))) ||
+        (!IS_STRING(stream) && !OpenOutputStream(stream))) {
+        Pr("Can't open output stream\n", 0L, 0L);


Can't -> Cannot

ssiccha · 2018-06-07T14:10:08Z

@fingolfin I will introduce a global variable that tells Where, WHERE, and ErrorInner where to send all error output.
The jupyter kernel can then set that variable instead of e.g. rebinding Error

markuspf · 2018-06-07T16:19:17Z

src/error.c

+        Pr("Can not open output stream\n", 0L, 0L);
+        openedOutput = 0;
+    }
+


So when you did not manage to open the output, you still print an error message to the current output and set a flag?

Did you try just raising an error using ErrorMayQuit?

I thought calling other Error functions may lead to an infinite loop. ErrorMayQuit itself also calls ErrorInner.

I will change it to print to *errout* - or quit if that also fails - to prevent that GAP could silently write to e.g. a file when encountering errors.

Is there a better option than quitting if I also can't open errout?

Ah, I can tell ErrorMayQuit to not go into a break loop. I'll try that.

If you can't open errout, I would just exit. For example, the C level function PRINT_OR_APPEND_TO, which is used for printing to streams, if it tries to print to errout and fails, just exits GAP.

gah, the two arguments to ErrorMayQuit are forwarded to Pr. I thought they were arguments that toggle the behaviour. :/

Thanks for the hint.

markuspf · 2018-06-07T16:24:09Z

lib/error.g

+# If an interactive SHELL is started by a break loop it still listens
+# and prints to "*errin*" and "*errout*" respectively.
+GAP_ERROR_STREAM := "*errout*";
+


We are currently talking about #1815, which could potentially replace this variable with another one (or remove the need for it); I think having it around right now would make it easy to search and replace, so not much of a problem.

markuspf · 2018-06-07T16:25:00Z

lib/error.g

-            PRINT_CURRENT_STATEMENT(context);
-            Print("\c");
-            PrintTo("*errout*"," called from \n");
+            PrintTo(GAP_ERROR_STREAM, " in\n  \c");


strictly speaking the \c is not necessary anymore now.

I wanted to refactor all of ErrorInner once this PR is merged. I will remove "all" of them then.

codecov · 2018-06-07T20:25:25Z

Codecov Report

Merging #2529 into master will decrease coverage by <.01%.
The diff coverage is 29.62%.

@@            Coverage Diff             @@
##           master    #2529      +/-   ##
==========================================
- Coverage    74.6%    74.6%   -0.01%     
==========================================
  Files         481      481              
  Lines      242402   242409       +7     
==========================================
+ Hits       180855   180859       +4     
- Misses      61547    61550       +3

Impacted Files	Coverage Δ
lib/error.g	`36% <16.66%> (+0.2%)`	⬆️
src/error.c	`78.86% <55.55%> (-0.98%)`	⬇️
src/hpc/threadapi.c	`43.48% <0%> (-0.11%)`	⬇️
src/stats.c	`89.6% <0%> (+0.19%)`	⬆️

fingolfin · 2018-06-07T21:55:26Z

src/error.c

+        } else {
+            int ret = fputs("gap: panic, can not open *errout*!\n", stderr);
+            // If that failed, try printing to stdout
+            if(ret == EOF) {


Please run clang-format resp. git clang-format on this PR.

ssiccha · 2018-06-07T22:31:39Z

As long as #1815 is not merged, GAP_ERROR_STREAM can be used to feed weird streams to OpenOutputStream. E.g. if the variable is set to a closed stream GAP segfaults (maybe tries to write to it, or maybe OpenOutputStream itself crashes).

ssiccha · 2018-06-12T15:28:54Z

I just realized that one travis test-suite is failing..

ssiccha · 2018-06-12T22:29:09Z

I fixed the bug. I forgot to do CloseOutputStream when returning early.

fingolfin · 2018-06-12T22:42:42Z

src/error.c

 {
-    if (context == STATE(BottomLVars))
-        return 0;


How about leaving this check here at the start, before trying to open the output stream? That seems simpler than opening the stream, then perform a trivial check, then close it again. Am I missing something?

That's a lot better. I'll adjust it.

ssiccha · 2018-06-12T22:48:02Z

I also got rid of all flush characters in ErrorInner. These are unnecessary because now we always print to the same stream.

fingolfin · 2018-06-13T11:35:52Z

src/error.c

+        }
+        else {
+            int ret = fputs("gap: panic, can not open *errout*!\n", stderr);
+            /* If that failed, try printing to stdout */


Hmm, why do this? Is there a specific problem case you have in mind there? Why would printing to stderr fail? And if it fails, would printing to stdout still work? And, are we really that desperate to print this message?

I didn't have any specific situation in mind. I thought a fallback option for when errout can't be openend would make sense. As to why first try stderr and then stdout: I saw similar code in PRINT_OR_APPEND_TO in streams.c and thought "better be safe than sorry".
Should I change this to just print to stderr and exit directly?

I changed this to be consistent with basically any other "GAP panic".

fingolfin · 2018-06-15T12:59:05Z

src/error.c

+        }
+        else {
+            fputs("gap: panic, can not open *errout*!\n", stderr);
+            SyExit(1);


These two lines could be changed to use the new Panic helper function. But that's not very important.

fingolfin · 2018-06-15T13:00:11Z

src/error.c

+    if ((IsStringConv(stream) && !OpenOutput(CSTR_STRING(stream))) ||
+        (!IS_STRING(stream) && !OpenOutputStream(stream))) {
+        if (OpenOutput("*errout*")) {
+            Pr("PRINT_CURRENT_STATEMENT: can not open error stream\n", 0, 0);


Hmm, maybe "could not" or "failed to" instead of "can not? Same in the message a few lines below?

Sounds good.

- ErrorInner, Where, and WHERE consistently print their output to "*errout*" - Get rid of now unnecessary flush character \c in ErrorInner - PRINT_CURRENT_STATEMENT now takes a stream as additional input. - Adjust its tests accordingly.

ssiccha requested review from fingolfin and markuspf June 7, 2018 11:11

sebasguts reviewed Jun 7, 2018

View reviewed changes

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch from bedb0f1 to 8fab742 Compare June 7, 2018 13:30

fingolfin requested changes Jun 7, 2018

View reviewed changes

fingolfin added topic: kernel topic: library labels Jun 7, 2018

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch from 8fab742 to fa4d1ce Compare June 7, 2018 15:37

markuspf reviewed Jun 7, 2018

View reviewed changes

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch from fa4d1ce to 297ddfc Compare June 7, 2018 20:24

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch 2 times, most recently from d1c6456 to e987cfb Compare June 7, 2018 21:30

ssiccha mentioned this pull request Jun 7, 2018

Enable JupyterKernel to print traceback on error #2530

Closed

fingolfin reviewed Jun 7, 2018

View reviewed changes

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch from e987cfb to e2700e9 Compare June 7, 2018 22:22

olexandr-konovalov added the gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 label Jun 8, 2018

fingolfin mentioned this pull request Jun 11, 2018

kernel: don't include debug.h everywhere; and more #2536

Merged

ssiccha changed the title ~~Make error output use *errout* consistently~~ Make error output use the same stream consistently Jun 12, 2018

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch from e2700e9 to 0ff91a5 Compare June 12, 2018 22:25

fingolfin reviewed Jun 12, 2018

View reviewed changes

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch 2 times, most recently from 49ffb8c to 58f12d2 Compare June 12, 2018 22:47

ssiccha mentioned this pull request Jun 13, 2018

Use GAP library functions to handle errors (DONT MERGE) gap-packages/JupyterKernel#56

Closed

fingolfin reviewed Jun 13, 2018

View reviewed changes

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch 4 times, most recently from bb45e33 to 03583e3 Compare June 14, 2018 23:13

fingolfin approved these changes Jun 15, 2018

View reviewed changes

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch from 03583e3 to d071366 Compare June 16, 2018 09:53

Make error output use "*errout*" consistently

4e59eff

- ErrorInner, Where, and WHERE consistently print their output to "*errout*" - Get rid of now unnecessary flush character \c in ErrorInner - PRINT_CURRENT_STATEMENT now takes a stream as additional input. - Adjust its tests accordingly.

ssiccha force-pushed the ss/make-print-curr-stat-redirectable branch from d071366 to 4e59eff Compare June 18, 2018 09:05

markuspf merged commit b19e0cc into gap-system:master Jun 18, 2018

fingolfin mentioned this pull request Jun 28, 2018

HPC-GAP: error printing broken due to PR #2529 #2587

Closed

ssiccha deleted the ss/make-print-curr-stat-redirectable branch June 28, 2018 22:40

fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jul 31, 2018

Make error output use the same stream consistently #2529

Make error output use the same stream consistently #2529

Conversation

ssiccha commented Jun 7, 2018

ssiccha commented Jun 7, 2018

ssiccha commented Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssiccha commented Jun 7, 2018

fingolfin commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssiccha commented Jun 7, 2018

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

ssiccha Jun 7, 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

codecov bot commented Jun 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssiccha commented Jun 7, 2018

ssiccha commented Jun 12, 2018

ssiccha commented Jun 12, 2018

fingolfin Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssiccha commented Jun 12, 2018

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

ssiccha commented Jun 7, 2018 •

edited

Loading

ssiccha Jun 7, 2018 •

edited

Loading

codecov bot commented Jun 7, 2018 •

edited

Loading

fingolfin Jun 12, 2018 •

edited

Loading