Add support for writing to ERROR_OUTPUT from kernel code #3043

embray · 2018-11-22T10:05:22Z

#2824 added a global variable called ERROR_OUTPUT to lib/error.g, which can be manipulated to control the file/stream that error output is sent to by the error library. This works for most cases; however, code in the kernel that outputs error messages ought also to write to whatever ERROR_OUTPUT points to, rather than defaulting to `"errout".

The one case I found this to be an issue was in printing syntax errors, though there may be other cases.

This is my first attempt to work around it, and my first addition of new code to GAP, so please be kind--I've tried my best to observe and follow local conventions, etc.

One problem with this PR I'm aware of currently is that when using ImportGVarFromLibrary, it sets that global variable as read-only by default, which was not previously the case for ERROR_OUTPUT. I'm not exactly sure why it does this, as it is still possible to set the variable read-write afterwards. This may be a good change though. It would prevent accidental clobbering of ERROR_OUTPUT: In order to change it you really have to mean it. I've seen this pattern in use in a couple of packages when overriding ErrorInner as well, so there is precedent for it. But I don't know whether or not that's considered an anti-pattern.

file/stream referenced by the ERROR_OUTPUT global variable

codecov · 2018-11-22T11:54:59Z

Codecov Report

Merging #3043 into master will decrease coverage by <.01%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #3043      +/-   ##
==========================================
- Coverage   83.78%   83.78%   -0.01%     
==========================================
  Files         685      685              
  Lines      343377   343389      +12     
==========================================
+ Hits       287694   287702       +8     
- Misses      55683    55687       +4

Impacted Files	Coverage Δ
src/error.h	`100% <ø> (ø)`	⬆️
src/scanner.c	`91.64% <100%> (ø)`	⬆️
src/error.c	`85.27% <66.66%> (-0.91%)`	⬇️

fingolfin

Thank you! On a first glance, this seems quite sensible to me, but I'd like to think a bit more about. Also, I hope @markuspf and @ChrisJefferson and others will have a look, too :-).

fingolfin · 2018-11-22T12:04:29Z

src/error.c

+        /* It may be we already tried and failed to open *errout* above but
+         * but this is an extreme case so it can't hurt to try again
+         * anyways */
+        if ((ret = OpenOutput("*errout*"))) {


Please don't assign inside the if, i.e., first assign, then test.

fingolfin · 2018-11-22T12:06:48Z

src/error.c

+            ret = OpenOutput(CONST_CSTR_STRING(ERROR_OUTPUT));
+        }
+        else {
+            ret = OpenOutputStream(ERROR_OUTPUT);


We started to add more validation, and here, one could verify that ERROR_OUTPUT actually is a stream; see e.g. PRINT_OR_APPEND_TO_FILE_OR_STREAM.

On that note, perhaps it would be "better" to put this new function into streams.c or io.c...; but I have no strong conviction in either direction, and in any case, it's not hard to move a function to another file later on, should we feel the need for it :-).

I wanted to check that it was actually a stream, but I couldn't quickly find the "right" way to check that. Is there a TNUM for stream objects? I'll look at those functions you mentioned.

I agree this function might make more sense in a io.c. I put it in error.c because that was already loading another global variable from error.g. I don't know if that entirely matters though: The connection between the C-based modules and the GAP language library modules is one that I haven't deeply explored yet and is unclear to me.

In general, TNUMs only cover a very small, hard-wired list of types with special representations in the GAP kernel (things like integers, booleans, permutations). For things like streams, you want to use the more general IsOutputStream. As Max mentioned, you can see how to get this GAP level variable, and copy a reference to it into C, in PRINT_OR_APPEND_TO_FILE_OR_STREAM

Yes, clearly I did not even test my last commit. I'm just going to delete it.

embray · 2018-11-23T14:00:35Z

I noticed that the JupyterKernel package already relies on setting ERROR_OUTPUT in order to capture error output. So the fact that ImportGVarFromLibrary sets that global variable to read-only by default would break that.

I still think it's reasonable behavior that it does that. But if this change is accepted we should either updated the JupyterKernel code, or just keep this variable read/write by default (and in that case, what would be the best way to do that after ImportGVarFromLibrary?)

… to open it

ssiccha · 2018-11-23T18:59:09Z

Hi @embray. I like this. I didn't know how the scanner works, so I only took care of ErrorInner in #2824. :) Since the new function is equivalent to what happens at the beginning of FuncPRINT_CURRENT_STATEMENT, we can then also delete that code and call the new function there once this is merged.

ssiccha · 2018-11-23T19:21:19Z

About the role of ERROR_OUTPUT: There is no particular reason that I introduced it as a GAP-level global variable. Unless there is some reason against it, maybe a better way of protecting it would be to let it be a C-level Obj later and provide a SET_ERROR_OUTPUT function? Then for now we could adapt the JupyterKernel.

embray · 2018-11-26T08:38:33Z

@ssiccha

Since the new function is equivalent to what happens at the beginning of FuncPRINT_CURRENT_STATEMENT

It isn't exactly equivalent, since that looks at an arbitrary stream object (in practice it is often the ERROR_INPUT which is passed here by ErrorInner in error.g, but that need not be the case for sure.

But I could refactor a bit more to eliminate that duplicate code. I don't know how much you all care about moving around GAP kernel internal functions that aren't directly accessible from GAP code, given that there is not officially an "API" as such (outside of the the new one that's being added...)

fingolfin · 2018-11-26T13:24:02Z

Don't worry about FuncPRINT_CURRENT_STATEMENT. That's a function that grew from a minor, very crash hack to a much bigger thing, that we now should refactor (e.g. I'd like to make it resp. a variant of it easy to call from within gdb to help with debugging). But all that can and should be done in a separate PR.

markuspf · 2018-11-27T09:43:21Z

One problem with this PR I'm aware of currently is that when using ImportGVarFromLibrary, it sets that global variable as read-only by default, which was not previously the case for ERROR_OUTPUT. I'm not exactly sure why it does this, as it is still possible to set the variable read-write afterwards.

I think this is in some sense entirely coincidental. ERROR_OUTPUT is introduced in the library by assignment (i.e. ERROR_OUTPUT := "*errout*"), which makes it a read-write global. ImportGVarFromLibrary just enters the global variable into a list of global variables which are by default read only.
One could view this as a bug in the library which should really have used BindGlobal("ERROR_OUTPUT", "*errout*"); to introduce this variable as a proper global.

This may be a good change though. It would prevent accidental clobbering of ERROR_OUTPUT: In order to change it you really have to mean it. I've seen this pattern in use in a couple of packages when overriding ErrorInner as well, so there is precedent for it. But I don't know whether or not that's considered an anti-pattern.

There might even be a point in introducing a setter function for error output (later). It might indeed be a good idea to have ERROR_OUTPUT read only by default.

I'd just like someone (@ssicha?) to make a matching PR for JupyterKernel as to not break it, some people seem to be using it now.

ssiccha

I've tried it with jupyter and got it to work. All other points raised in the discussion can be addressed in different issues and PRs. LGTM 👍

ssiccha · 2018-11-27T16:56:06Z

Also I opened a corresponding PR gap-packages/JupyterKernel#85.

markuspf · 2018-11-28T12:56:50Z

I just merged #78 on JupyterKernel so I am fine with merging this.

fingolfin

Fine then, let's merge this once the build queue for master has cleared.

olexandr-konovalov · 2018-12-14T12:10:58Z

Did we agree to backport this to stable-4.10 branch?

fingolfin · 2018-12-14T12:11:34Z

Backported via de7b9b9

kernel: add helper function for writing error messages to the

cd61c7f

file/stream referenced by the ERROR_OUTPUT global variable

fingolfin added the topic: kernel label Nov 22, 2018

fingolfin reviewed Nov 22, 2018

View reviewed changes

fix objectionable assignment expression

3eb2f2a

explicitly check if ERROR_OUTPUT is a stream object before attempting…

2949b24

… to open it

embray force-pushed the syntax-error-output branch from afa635f to 2949b24 Compare November 23, 2018 14:01

ssiccha approved these changes Nov 27, 2018

View reviewed changes

ssiccha mentioned this pull request Nov 27, 2018

Syntax errors shouldn't printed as normal output gap-packages/JupyterKernel#78

Closed

markuspf approved these changes Nov 28, 2018

View reviewed changes

fingolfin approved these changes Nov 29, 2018

View reviewed changes

fingolfin merged commit 458a4fd into gap-system:master Nov 29, 2018

olexandr-konovalov mentioned this pull request Dec 14, 2018

Don't store environ argument to GAP_Initialize if it is the same as the system environ #3106

Closed

olexandr-konovalov added this to the GAP 4.10.1 milestone Dec 14, 2018

fingolfin pushed a commit to fingolfin/gap that referenced this pull request Dec 14, 2018

Backport PR gap-system#3043 (handle ERROR_OUTPUT in kernel)

de7b9b9

olexandr-konovalov added the backport-to-4.10-DONE label Dec 14, 2018

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Feb 23, 2019

nthiery mentioned this pull request Jan 13, 2023

Upgrade to GAP 4.10 sagemath/sage#22626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for writing to ERROR_OUTPUT from kernel code #3043

Add support for writing to ERROR_OUTPUT from kernel code #3043

embray commented Nov 22, 2018 •

edited

Loading

codecov bot commented Nov 22, 2018 •

edited

Loading

fingolfin left a comment

fingolfin Nov 22, 2018

fingolfin Nov 22, 2018

embray Nov 23, 2018

ChrisJefferson Nov 23, 2018

embray Nov 23, 2018

embray commented Nov 23, 2018

ssiccha commented Nov 23, 2018 •

edited

Loading

ssiccha commented Nov 23, 2018 •

edited

Loading

embray commented Nov 26, 2018

fingolfin commented Nov 26, 2018

markuspf commented Nov 27, 2018

ssiccha left a comment

ssiccha commented Nov 27, 2018

markuspf commented Nov 28, 2018

fingolfin left a comment

olexandr-konovalov commented Dec 14, 2018

fingolfin commented Dec 14, 2018

Add support for writing to ERROR_OUTPUT from kernel code #3043

Add support for writing to ERROR_OUTPUT from kernel code #3043

Conversation

embray commented Nov 22, 2018 • edited Loading

codecov bot commented Nov 22, 2018 • edited Loading

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Nov 22, 2018

Choose a reason for hiding this comment

fingolfin Nov 22, 2018

Choose a reason for hiding this comment

embray Nov 23, 2018

Choose a reason for hiding this comment

ChrisJefferson Nov 23, 2018

Choose a reason for hiding this comment

embray Nov 23, 2018

Choose a reason for hiding this comment

embray commented Nov 23, 2018

ssiccha commented Nov 23, 2018 • edited Loading

ssiccha commented Nov 23, 2018 • edited Loading

embray commented Nov 26, 2018

fingolfin commented Nov 26, 2018

markuspf commented Nov 27, 2018

ssiccha left a comment

Choose a reason for hiding this comment

ssiccha commented Nov 27, 2018

markuspf commented Nov 28, 2018

fingolfin left a comment

Choose a reason for hiding this comment

olexandr-konovalov commented Dec 14, 2018

fingolfin commented Dec 14, 2018

embray commented Nov 22, 2018 •

edited

Loading

codecov bot commented Nov 22, 2018 •

edited

Loading

ssiccha commented Nov 23, 2018 •

edited

Loading

ssiccha commented Nov 23, 2018 •

edited

Loading