New GAP-level variable ErrorOutput for "errout" #2824

ssiccha · 2018-09-18T15:22:04Z

This is a "revival" of #1815.

Changes in comparison to PR #1815

The changes to markus' PR are

I got rid of the C level declarations
I'm using "*errout*" instead of wrapping it in an OutputTextFile(). The reason being that when using OutputTextFile("*errout*", bool) (with bool true or false) make testinstall fails due to the error messages vanishing into thin air. Actually they still appear in the terminal, but before all of the "testing: ..." output. I guess this may be somehow related to Out of order printing when using OutputTextFile("*stdout*", false) #2823?
I did not define StandardInput and StandardOutput. If GAP's i/o is going to
be refactored by using GAP level IsOutputTextStream objects we can still define them then.

I did not do changes to colorprompt.g since I think STDOut is only defined in this file because there is no method installed for WriteAll which can take "*stdout*" as an argument. Also I have the feeling defining ErrorOutput should be a temporary solution only.

fingolfin · 2018-09-18T16:22:47Z

lib/streams.gi

@@ -642,7 +642,7 @@ function( str )
    if fid = fail  then
        return fail;
    else
-        AddSet( InputTextFileStillOpen, fid );
+        ADD_SET( InputTextFileStillOpen, fid );


Ha, this was needed in markus' PR. But since i'm now using "*errout*" directly this is not necessary anymore. I'll fix this.

fingolfin · 2018-09-18T16:24:14Z

lib/error.g

-        PRINT_CURRENT_STATEMENT("*errout*", context);
-        PrintTo("*errout*", " called from\n");
+        PRINT_CURRENT_STATEMENT(ErrorOutput, context);
+        PrintTo(ErrorOutput, " called from\n");


I am feeling a bit conflicted about the name ErrorOutput; it'll show up in tab completion easily, esp. if one is writing code that uses Error. Since it is deeply internal, I'd prefer calling it something like ERROR_OUTPUT.

Good point. Will do that.

ssiccha · 2018-09-18T16:34:02Z

Also this is breaking HPCGAP. I'll have a look.

ssiccha · 2018-09-19T09:49:07Z

@fingolfin Checks pass now.

codecov · 2018-09-19T12:51:27Z

Codecov Report

Merging #2824 into master will increase coverage by <.01%.
The diff coverage is 10%.

@@            Coverage Diff             @@
##           master    #2824      +/-   ##
==========================================
+ Coverage   76.09%    76.1%   +<.01%     
==========================================
  Files         480      480              
  Lines      240969   240969              
==========================================
+ Hits       183375   183380       +5     
+ Misses      57594    57589       -5

Impacted Files	Coverage Δ
lib/error.g	`38.2% <10%> (ø)`	⬆️
hpcgap/lib/hpc/stdtasks.g	`64.7% <0%> (+1.27%)`	⬆️

fingolfin

Shall we just merge this as is, or is there any reason not to? @markuspf @ssiccha ?

ssiccha · 2018-09-20T14:16:58Z

~~@fingolfin it can be merged like this 👍~~

ssiccha · 2018-09-20T15:39:25Z

I have just added that "ERROR_OUTPUT" is a readwrite global from the start.

If you're ok with this you can merge it like this.

markuspf · 2018-09-20T20:01:46Z

I would merge this but tests don't run it seems.

fingolfin · 2018-09-20T20:03:30Z

We have completly overloaded Travis and AppVeyor. I recommend that we just wait till tomorrow with further merges, to let them finish of the dozen or so pending builds on each.

markuspf · 2018-09-21T08:30:47Z

@ssiccha are you still making changes to this or can i merge once tests cycled?

markuspf · 2018-09-21T08:31:26Z

Also do you mind doing a PR for JupyterKernel? Unfortunately we'll have to keep it 4.10 compatible, unless we backport this to stable-4.10.

ssiccha · 2018-09-21T08:36:11Z

I broke the PR yesterday, but I didn't notice since I accidentally used the wrong gap on my machine. If this round of tests pass then it can be merged. Unless I shouldn't define ERROR_OUTPUT in the error.g file? I moved the definition to error.g since streams.gi seems to get loaded after error.g.

Yepp, I'll do the Jupyter PR. It would be nice if this got backported.

fingolfin · 2018-09-21T05:53:30Z

lib/streams.gd

@@ -1350,6 +1350,7 @@ DeclareGlobalFunction( "UnInstallCharReadHookFunc" );
 DeclareGlobalFunction( "InputFromUser" );


+


Pleas remove this change

fingolfin · 2018-09-21T05:56:26Z

hpcgap/lib/streams.gi

+##
+## Default stream for error messages.
+BindGlobal("ERROR_OUTPUT", "*errout*");
+MakeReadWriteGlobal("ERROR_OUTPUT", "*errout*");


This must be in the .gd file not the .gi!

Also, if you want this writeable (apparently I misunderstood you yesterday), just use ERROR_OUTPUT:=...

fingolfin · 2018-09-21T12:16:35Z

lib/error.g

+##
+## Default stream for error messages.
+BindGlobal("ERROR_OUTPUT", "*errout*");
+MakeReadWriteGlobal("ERROR_OUTPUT");


Why even both, why not just do: ERROR_OUTPUT := "*errout*";?

fingolfin · 2018-09-21T12:17:18Z

@markuspf From my POV we could backport this PR to stable-4.10

ssiccha · 2018-09-21T15:58:07Z

I'm now using ERROR_OUTPUT := "*errout*";. Should I leave it in the error.g file then or move it somewhere else?

@fingolfin

This commit introduces a new global variable ERROR_OUTPUT. As a next step one can make it possible to change the targets of the magic strings "*stdout*", "*stdin*", and "*errout*", as suggested by @fingolfin in gap-system#1815. See gap-system#2822.

Makes all functions in error.g use ERROR_OUTPUT instead of the magic string "*errout*". Then the error messages can be redirected by overwriting ERROR_OUTPUT. This should be temporary. If the target of "*errout*" itself can be changed, this can be reverted again. See gap-system#2822.

fingolfin · 2018-09-21T18:32:30Z

Since it is used in error.g and only there, that indeed seems sensible.

ssiccha · 2018-09-21T20:38:00Z

@mtorpey you should be able to use this in your package manager now. :)

ssiccha · 2018-09-21T20:38:55Z

It will not redirect syntax errors though since these are printed from somewhere within the kernel.

dimpase · 2018-09-26T13:52:27Z

While using libgap, I get hit by

gap: panic, could not open *errout*!

error message, and then the whole thing bails out. Should I use functionality from this PR, and if yes, how?

Or, perhaps, this is the result merging of this PR ?

ChrisJefferson · 2018-09-26T14:02:51Z

I don't think this PR should have made any difference in that area, unless there was a bug.

When using libgap, are you closing the errout socket (usually 2 at the C level)?

dimpase · 2018-09-26T16:15:49Z

the code I am porting was written for GAP 4.8.0 or so. It seems that in the meantime GAP's error handling has been changed a lot.
The old code grabs GAP's stderr_buffer and makes it available for error reporting at Python level, via an error-handler callback.

How would one do this with GAP 4.10?

ChrisJefferson · 2018-09-26T16:57:56Z

I'm not sure there is a nice way, and either way we probably shouldn't be discussing it down here. It could/should be part of libgap I imagine,

ssiccha added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: error handling labels Sep 18, 2018

ssiccha requested review from fingolfin, markuspf and frankluebeck September 18, 2018 15:22

ssiccha force-pushed the ss/gap-level-stdin-stdout-errout branch from 9645f68 to f99f240 Compare September 18, 2018 15:24

fingolfin reviewed Sep 18, 2018

View reviewed changes

ssiccha force-pushed the ss/gap-level-stdin-stdout-errout branch 3 times, most recently from 34a9e7e to e04b261 Compare September 18, 2018 16:40

ssiccha force-pushed the ss/gap-level-stdin-stdout-errout branch from e04b261 to f08fcb5 Compare September 19, 2018 12:51

fingolfin added the gapdays2018-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2018-fall label Sep 19, 2018

fingolfin approved these changes Sep 20, 2018

View reviewed changes

ssiccha added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Sep 20, 2018

ssiccha force-pushed the ss/gap-level-stdin-stdout-errout branch from f08fcb5 to 5912047 Compare September 20, 2018 15:37

ssiccha force-pushed the ss/gap-level-stdin-stdout-errout branch 3 times, most recently from 28bd1c9 to b43200f Compare September 20, 2018 16:51

ssiccha force-pushed the ss/gap-level-stdin-stdout-errout branch from b43200f to 7e11276 Compare September 21, 2018 07:56

markuspf added the backport-to-4.10 label Sep 21, 2018

ssiccha mentioned this pull request Sep 21, 2018

Adapt to use ERROR_OUTPUT gap-packages/JupyterKernel#79

Merged

fingolfin reviewed Sep 21, 2018

View reviewed changes

ssiccha added 2 commits September 21, 2018 17:58

ssiccha force-pushed the ss/gap-level-stdin-stdout-errout branch from 7e11276 to 7a4ede8 Compare September 21, 2018 15:58

fingolfin merged commit d49eb09 into gap-system:master Sep 21, 2018

ssiccha deleted the ss/gap-level-stdin-stdout-errout branch September 22, 2018 08:20

markuspf added backport-to-4.10-DONE and removed backport-to-4.10 labels Sep 22, 2018

ssiccha mentioned this pull request Sep 24, 2018

WIP: Exchange PrintExpr by StringExpr #2858

Closed

dimpase mentioned this pull request Sep 27, 2018

manipulate output streams (esp. errout) in libgap #2874

Open

embray mentioned this pull request Nov 22, 2018

Add support for writing to ERROR_OUTPUT from kernel code #3043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New GAP-level variable ErrorOutput for "errout" #2824

New GAP-level variable ErrorOutput for "errout" #2824

ssiccha commented Sep 18, 2018 •

edited

Loading

fingolfin Sep 18, 2018

ssiccha Sep 18, 2018

fingolfin Sep 18, 2018

ssiccha Sep 18, 2018

ssiccha commented Sep 18, 2018

ssiccha commented Sep 19, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

fingolfin left a comment

ssiccha commented Sep 20, 2018 •

edited

Loading

ssiccha commented Sep 20, 2018

markuspf commented Sep 20, 2018

fingolfin commented Sep 20, 2018

markuspf commented Sep 21, 2018

markuspf commented Sep 21, 2018

ssiccha commented Sep 21, 2018

fingolfin Sep 21, 2018

fingolfin Sep 21, 2018

fingolfin Sep 21, 2018

fingolfin commented Sep 21, 2018

ssiccha commented Sep 21, 2018

fingolfin commented Sep 21, 2018

ssiccha commented Sep 21, 2018

ssiccha commented Sep 21, 2018

dimpase commented Sep 26, 2018

ChrisJefferson commented Sep 26, 2018

dimpase commented Sep 26, 2018

ChrisJefferson commented Sep 26, 2018

		@@ -1350,6 +1350,7 @@ DeclareGlobalFunction( "UnInstallCharReadHookFunc" );
		DeclareGlobalFunction( "InputFromUser" );

New GAP-level variable ErrorOutput for "*errout*" #2824

New GAP-level variable ErrorOutput for "*errout*" #2824

Conversation

ssiccha commented Sep 18, 2018 • edited Loading

Changes in comparison to PR #1815

fingolfin Sep 18, 2018

Choose a reason for hiding this comment

ssiccha Sep 18, 2018

Choose a reason for hiding this comment

fingolfin Sep 18, 2018

Choose a reason for hiding this comment

ssiccha Sep 18, 2018

Choose a reason for hiding this comment

ssiccha commented Sep 18, 2018

ssiccha commented Sep 19, 2018

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

ssiccha commented Sep 20, 2018 • edited Loading

ssiccha commented Sep 20, 2018

markuspf commented Sep 20, 2018

fingolfin commented Sep 20, 2018

markuspf commented Sep 21, 2018

markuspf commented Sep 21, 2018

ssiccha commented Sep 21, 2018

fingolfin Sep 21, 2018

Choose a reason for hiding this comment

fingolfin Sep 21, 2018

Choose a reason for hiding this comment

fingolfin Sep 21, 2018

Choose a reason for hiding this comment

fingolfin commented Sep 21, 2018

ssiccha commented Sep 21, 2018

fingolfin commented Sep 21, 2018

ssiccha commented Sep 21, 2018

ssiccha commented Sep 21, 2018

dimpase commented Sep 26, 2018

ChrisJefferson commented Sep 26, 2018

dimpase commented Sep 26, 2018

ChrisJefferson commented Sep 26, 2018

New GAP-level variable ErrorOutput for "errout" #2824

New GAP-level variable ErrorOutput for "errout" #2824

ssiccha commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading

ssiccha commented Sep 20, 2018 •

edited

Loading