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

breaking,log: set stderr as default log output, add .set_output_stream() #23444

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gechandesu
Copy link
Contributor

@gechandesu gechandesu commented Jan 12, 2025

This PR is second try of #23296

In short:

  • Makes possible set log output to io.Writer via new .set_output_stream() fn
  • Sets os.stderr() as default log output.

Copy link

Connected to Huly®: V_0.6-21875

@JalonSolov
Copy link
Contributor

Not so sure setting stderr as default is a good idea, since it will be a "breaking change" to the current.

If someone has things set up to capture stdout, they "suddenly" won't be capturing anything, any longer. Etc.

@spytheman
Copy link
Member

It is true that it will be a breaking change in that regard.
I also do think, that it is a better long term default, compared to logging to stdout.

I was more concerned about it being able to pass the valgrind checks, and it indeed does this time:
image

@spytheman
Copy link
Member

Personally, I think it is a good change, and should be merged.

@medvednikov what do you think?

@JalonSolov
Copy link
Contributor

I know that I would be royally P.O.'d if something changed like this out from under me, if I already had other programs/scripts set up to parse stdout, and it all of a sudden wasn't there. I'd be yelling about how you broke my stuff.

@gechandesu
Copy link
Contributor Author

Is the impact of this change really that big? Stdout is not used as default by loggers in any logging library I know (Go stdlib as relevant example) and command line applications usually always log to stderr to avoid mixing it with useful program output. I think stdout in the V logger was a bug and it's good to fix it before we reach V 1.0 and even wider adoption.

@JalonSolov
Copy link
Contributor

Change is good. Unannounced change is not.

In other words, don't just change the default out from under people. Let them know it is going to change, first, then change it. Just as V has done for other breaking changes that were deprecated first.

@spytheman
Copy link
Member

We can add breaking to the merge description.
Also we can have an example to the docs, that uses the new API
.set_output_stream() to change the output back to os.stdout()

In that way, for people that do depend on the existing behavior, restoring it will be a one line change, and the need for that will be clearly documented/visible, in both the docs and from the commit line.

@spytheman spytheman changed the title log: set stderr as default log output, add .set_output_stream() breaking,log: set stderr as default log output, add .set_output_stream() Jan 13, 2025
@JalonSolov
Copy link
Contributor

That's fine, and it's good documentation to have.

Of course, you're still "breaking" them until they either go look up the merge description/commit, or read the new docs.

I think it's a good change. Output the logs to stderr. Most things do tend to do that, unless their default is to log to a file.

However, having other things break is annoying at best, and catastrophic at worst.

Imagine if the default output of cat changed to stderr with no warning, and you tried to do a standard Linux chain of commands expecting the output to go to stdout to feed into the next command... sure, you could then Google and maybe find out it was a change someone decided to make, but it won't help what you're doing until you figure out what happened.

@spytheman
Copy link
Member

spytheman commented Jan 13, 2025

However, having other things break is annoying at best, and catastrophic at worst.

I agree, but the language/vlib version is still < 1.0, and we do not have a mechanism to mark it more clearly than that, and I also can not think of one, while still keeping the same API and module name.

Do you think that we should introduce logger, and deprecate log?

@spytheman
Copy link
Member

cat is not a good analogy in my mind, since cat was introduced in the 1970s and at this point, its external behavior is practically set in stone - what it reads and what it writes are clear to everyone.

@spytheman
Copy link
Member

To clarify, if someone decided to make a program that behaved like cat, but with output set to stderr, that program should be named differently, and the existing cat should stay as it is.

@JalonSolov
Copy link
Contributor

I simply used cat as an example of something that could cause complete chaos if the output changed without some sort of deprecation warning.

Specifically for log, how about just running your program with > log.txt 2>/dev/null (because you don't expect error output, just logs). That'll work with the current default of stdout, but will give you an empty file with this change. Empty file means you don't have the log to look at after the program exits, which can be especially annoying if something went wrong.

Argue all you want about whether it's a good idea to have 2>/dev/null, again, this is just an example.

@spytheman
Copy link
Member

I already agreed that it is a breaking change.

However, it is a good breaking change.
I am currently searching for ways to make it more clear and easier for people, not for how it could break their expectations - that was already obvious from the start.

One way is adding a breaking tag - already done.
Another is adding an example in the docs - not done yet.
A third would be to deprecate the whole module, and add a new one, named differently (for example logger), which I think is overkill, given the stage of the language, and that it is still < 1.0 .

If you have other ideas about how to make the transition easier, while keeping the same API, go ahead and propose them.
We have time for that, since I am not going to merge it anyway, before Alex approves it.

@JalonSolov
Copy link
Contributor

How about a notice about the change? If your code doesn't explicitly set the output to anything, give a notice that the default has changed to stderr.

The notice can be removed after a while, leaving the new default in place.

@spytheman
Copy link
Member

A compiler notice, or a runtime one, by the module itself?

@JalonSolov
Copy link
Contributor

I don't know which would be seen more. :-\

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.

3 participants