Logging: Add log sink system to replace ostreams

[Logikable]

This PR adds a LogSink class that aims to be more versatile than ostream sinks. See https://yosyshq.discourse.group/t/cleaning-up-log-sinks/86 for the discussion.

The old description can be found below:

We'd like to subscribe to only the most critical logs. Currently, there is no way to specify a log level threshold. This PR provides a stopgap solution.

In addition to the log_streams vector that new log sinks can push to, this PR adds a log_warning_streams vector that behaves similarly, but ostreams in this new vector only receive warning and error logs.

[mikesinouye]

Hey @widlarizer, please take a look when you get a chance, thank you!

[widlarizer]

Seems like ERROR is predefined somewhere in Windows specific headers. You can rename the enum variand or #ifdef #undef it

[Logikable]

Renamed, thanks :)

[widlarizer]

I'm in favor of the LogSeverity struct and an overloaded log function seems like a decent way to safely retrofit it. Is there an intended followup that exposes log_warning_streams somehow? As a side note, please take care to respect established whitespace, C++ is indented with tabs

[Logikable]

I've rewritten this PR in light of the log changes by @rocallahan. One notable consequence of this PR is that log_formatted_warning and log_error_with_prefix now call log_formatted_string instead of log. I've also switched to tabs.

Regarding the last comment (@widlarizer): log_warning_streams is already exposed the same way log_streams is exposed. Today, to subscribe a sink, you append an ostream to the global Yosys::log_streams; the same can be done with log_warning_streams.

[widlarizer]

There is no code in this PR appending to log_warning_streams

[widlarizer]

Also, this breaks tests/various/logger_cmd_error.sh

[Logikable]

Sorry -- I misunderstood. We'll be appending to log_warning_streams in a downstream change. I felt it better to contribute the public parts of our change upstream; is that ok?

[Logikable]

gentle bump

[widlarizer]

It took some internal alignment on what we want to do in situations like this. I'm also going to be updating contributing guielines and PR/issue templates to match what we've arrived at. Let me explain on the two sides of this PR:

The problem with adding warning streams that are only appended to in your patches is that it is impossible to maintain for us something that we can't cover with tests locally. Our regular testing requires exposing features to regular Yosys users too. In this case, that could be done via a driver option, like we already have -l / --logfile. Please add such an option (--warnfile? no need for a single letter spelling) and tests that use it to cover that feature

As for the log.h change, it's okay to change public headers like this if we agree on this being worth whatever the benefit is. For that, it would be best to discuss what direction you're taking Yosys in your patches on Discourse ahead of time. The fact you declare this a stopgap measure even suggests a lack of faith in this being the long-term right thing to do. I wouldn't block it on just this aspect, but even for this, I'd prefer first to hear more about the user story and your view on the alternate approaches we could be taking

Ultimately, my previous review still applies. If that is resolved, I don't mind merging this.

• expose warning streams in the driver (see above)

[Logikable]

Thank you! I've created https://yosyshq.discourse.group/t/cleaning-up-log-sinks/86.

[Logikable]

I've rewritten this PR to contain a more permanent solution. It's not perfect -- there are still places in log.cc where one API log call turns into multiple logv_string calls, and log sinks will see these as separate logs. That said, this was already an issue with the existing ostream system.

[jix]

The issue with multiple log calls used for a single message only extended to the log message matching used as part of our test suite, since with the stream based approach there isn't really a user-visible concept of individual log messages, as there are just lines ending in newlines and nothing to indicate where individual log calls start and end.

As I mentioned in my reply to your discourse thread, we do also have existing log emitting code that splits single lines of single messages across multiple log calls at the source, so any plan for improving the logging needs to include a plan for what to do about those. My suggestion would be to add line buffering when translating from stream based log sources to message based log sinks.

Repeating what I wrote on discourse, it's not a requirement that all of this is addressed in a first PR, but what is a requirement is that we can see how the PR is a step towards something that makes sense for the project. And in cases where we do have some ideas for improvements we'd like to see, also to ensure it's aligned or at least not in conflict with that.

@mmicko: I also want to note that this PR now contains changes to the CI workflows and adds a new build time dependency. (cc @widlarizer): I feel like part of our contribution guidelines should include that these are things that should be noted in the PR description, done separately from code changes, and ideally also discussed before being submitted for review.

What is still missing to move forward with this is:

• Responding to the discourse thread, so we have an idea how the plan behind this relates to the ideas that we already have for logging and whether it is headed in the same direction or at least can be extended to support those use cases.
• As already explained by @widlarizer, exposing the added functionality from the CLI by adding a driver option.