Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restore useful compiler errors on dumb terminals #17158

Closed

Conversation

winterqt
Copy link
Contributor

@winterqt winterqt commented Sep 15, 2023

This reverts a part of 7cc4a69, and closes #16376.

While the "correct" behavior on dumb terminals is still to be determined, this is arguably a major regression from 0.10.0 to 0.11.0, leaving users of various tools and CI platforms without any useful build logs.

This change mirrors the behavior of basically every other compiler out there, which is to just not color the output (done implicitly in std.io.tty).

Of course, the idea for machine-readable compiler errors still has merit, and with things like the to-be-implemented Zig compiler server 0 and ZLS, there are just better ways to go about it than to print somewhat useless error messages that a good portion of users will see.

This reverts a part of 7cc4a69, and
closes ziglang#16376.

While the "correct" behavior on dumb terminals is still to be determined,
this is arguably a major regression from 0.10.0 to 0.11.0, leaving users
of various tools and CI platforms without any useful build logs.

This change mirrors the behavior of basically every other compiler out
there, which is to just not color the output (done implicitly in `std.io.tty`).

Of course, the idea for machine-readable compiler errors still has
merit, and with things like the to-be-implemented Zig compiler
server [0] and ZLS, there are just better ways to go about it than to
print somewhat useless error messages that a good portion of users will
see.

[0]: ziglang#615
@winterqt winterqt force-pushed the restore-dumb-term-compiler-errors branch from 04b3acf to 7d90233 Compare September 15, 2023 00:48
@winterqt
Copy link
Contributor Author

cc @andrewrk as the main proponent against this 🙂

@winterqt
Copy link
Contributor Author

winterqt commented Sep 15, 2023

There's an alternative to this, now that I look at it more.

Given the following file:

const std = @import("std");

pub fn main() void {
    std.log.debug("");
}

TERM=dumb on master outputs:

foo.zig:4:12: error: expected 2 argument(s), found 1
/Users/winter/src/zig/lib/std/log.zig:193:13: note: function declared here

which looks good enough! However, the example in #16376 still shows the case detailed there due to comptime shenanigans, which... is not great.

We could move to displaying that example's error message to something like:

C:\zig\lib\std\mem.zig:455:65: error: expected type 'u32', found '*const [5:0]u8'
main.zig:6:21: note: references ^
C:\zig\lib\std\start.zig:598:17: note: references ^

I'm not really thrilled about it, but we can always compromise if you're insistent on keeping the current behavior...

@Vexu Vexu requested a review from andrewrk September 26, 2023 08:16
@ilohmar
Copy link

ilohmar commented Oct 15, 2023

I've been wrestling with this a few times. My scenario is zig build being run as a compilation sub-process inside Emacs.
Either I keep the default (TERM=dumb) and lose information I would like to see (as a human), or I set a different TERM=dumb-emacs-ansi (because Emacs does handle ANSI color codes) or force zig build --color on and get run-away lines from semantic analysis (b/c color also turns on cursor navigation codes that Emacs does not interpret) and escape code line noise for graphical chars in the summary. I understand that it was intentional that coloring and the amount of information are switched as one (though, for the record, I do not agree that this is a good idea).

Please let me add a different aspect: The main issue is that zig "derives" capabilities of the terminal from the TERM var itself, not from terminfo entries. Unfortunately, many programs do that, but on unix-y systems that has never been the "correct" way. Eg, on my system, I have proper entries for the above dumb-emacs-ansi that announce that this terminal handles ANSI color codes, but not cursor motion or graphical chars.

So there seem to be two ways that would solve all the different use cases:

  1. Handle TERM by asking for terminfo capabilities (ie, cursor motion, coloring, graphical chars all indendently). This adds dependencies and code complexity.
  2. Offer switches that turn on and off separately: what info is output (eg, --format human|machine), and escape codes for --color, --cursor-motion and --graphical-characters. This would not be elegant code, but straightforward, I suppose.

I would be grateful for any feedback, whether either of these approaches would be acceptable.. In the case of 2., I might give it a try myself. If neither is, I will (once again) have to try and hack into Emacs to deal with more sequences that it did not want to handle.

@erikarvstedt
Copy link
Contributor

erikarvstedt commented Oct 15, 2023

@ilohmar, meanwhile, here's a workaround to make Zig usable with Emacs shell-mode:

  1. Apply this patch to Zig to enable rendering of box drawings (like ├─) in Emacs.
  2. Apply this zig-mode PR to enable parsing cursor back (CUB) ANSI sequences in Emacs.

@ilohmar
Copy link

ilohmar commented Oct 15, 2023

@ilohmar, meanwhile, here's a workaround to make Zig usable in with Emacs shell-mode:

1. Apply [this patch](https://github.com/erikarvstedt/zig/commit/25af50542790293d3b680ed4ce45ba1b3a3fc385) to Zig to enable rendering of box drawings (like `├─`) in Emacs.

2. Apply [this `zig-mode` PR](https://github.com/ziglang/zig-mode/pull/70) to enable parsing cursor back (CUB) ANSI sequences in Emacs.

Thanks, I will try that! The patch to zig to use unicode chars will help me for all possible scenarios, anyway.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Please see #16376 (comment). This does not resolve the issue.

@andrewrk andrewrk closed this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing referenced-by information in machine-readable compile error output
4 participants