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

Please remove the hard dependency on glog #70

Closed
mvdan opened this issue Aug 2, 2022 · 6 comments · Fixed by #77
Closed

Please remove the hard dependency on glog #70

mvdan opened this issue Aug 2, 2022 · 6 comments · Fixed by #77

Comments

@mvdan
Copy link
Contributor

mvdan commented Aug 2, 2022

The parser imports https://github.com/golang/glog directly, for calls such as:

log.Errorf("format should be %s=<int>, got: %s", name, mc)

p.log.Infof("inlineComment: %q", strings.Join(inlineComment, "\n"))

It's understandable to want to have some verbose logging, but I think it's a bad idea for a "pure" Go library such as a parser to make direct calls to a logging library:

  • It forces all importers to also include glog as part of their build and binary, even if they don't want nor need any logging.
  • Logging shouldn't be enabled by default in generic libraries. The caller or main program should be in charge of logging, just like any input/output with the user.
  • Hard-coding a logging library makes it very hard or near impossible to use a single log implementation across an entire program using multiple library packages.

I think the best and simplest API fix would be to declare a logging interface and use it, such as:

type Logger interface {
    Infof(format string, args ...interface{}) // used for verbose logs
    Errorf(format string, args ...interface{})
}

One could also argue that error logs should be returned as error values instead, but I'm not very familiar with how the glog API is designed in detail.

I'm filing this because I'm the maintainer of a relatively large open source program where we use and ipmort txtpbfmt, but we pull glog indirectly too, which unfortunately affects us in multiple ways (e.g. cue-lang/cue#1199, but also binary size etc).

@stapelberg
Copy link

Hey Daniel! 👋

Agreed on all points. For historical context: we needed to get a number of ducks in a row for the initial open-sourcing of txtpbfmt, and doing something better than just swapping our internal log package for glog wasn’t in the cards back then.

Would you be willing to send a pull request which implements the interface you suggested? cc me on it, and we can get this improved.

Thanks

@mvdan
Copy link
Contributor Author

mvdan commented Aug 3, 2022

Oh, I wasn't aware you were involved :) I can certainly send a quick PR.

@Nexucis
Copy link

Nexucis commented Nov 14, 2022

Hi,

I was wondering if there is an on going fix. As a cue user, I was quite annoyed to see the glog flags poping in my binary.
If there is none, perhaps I could give a try.

@stapelberg
Copy link

Sending the PR is the fix. AFAICT, there was no PR yet

@mvdan
Copy link
Contributor Author

mvdan commented Nov 14, 2022

Thanks for the pings - I completely forgot about this. I can send the PR today.

mvdan added a commit to mvdan/txtpbfmt that referenced this issue Nov 14, 2022
As I was working on my patch for protocolbuffers#70, I was wondering if we really
needed a logger interface with both Infof and Errorf. After all, a
library should report its errors as values, not via a logger.

Turns out it's relatively straightforward to do so. All functions that
used to call log.Errorf are themselves called from top-level APIs which
are able to return an error, so we just need to thread them.

One special case is getMetaCommentStringValues, which used to report
each error and continue rather than stop. We currently do not have an
idiomatic way to report multiple errors at once, so return the first
error for now. In the future, we could use https://go.dev/issue/53435.

Updates protocolbuffers#70.
@Nexucis
Copy link

Nexucis commented Nov 14, 2022

ooh I was going to do it as well haha. Well thank you for doing it @mvdan !

mvdan added a commit to mvdan/txtpbfmt that referenced this issue Nov 14, 2022
As I was working on my patch for protocolbuffers#70, I was wondering if we really
needed a logger interface with both Infof and Errorf. After all, a
library should report its errors as values, not via a logger.

Turns out it's relatively straightforward to do so. All functions that
used to call log.Errorf are themselves called from top-level APIs which
are able to return an error, so we just need to thread them.

One special case is getMetaCommentStringValues, which used to report
each error and continue rather than stop. We currently do not have an
idiomatic way to report multiple errors at once, so return the first
error for now. In the future, we could use https://go.dev/issue/53435.

Updates protocolbuffers#70.
mvdan added a commit to mvdan/txtpbfmt that referenced this issue Nov 23, 2022
All calls go through a logger in the config.

The logger may be nil, so we check that it's non-nil everywhere.
We want the conditional in every location, as otherwise we would
evaluate all the format arguments even when logging is disabled.
Some of them are relatively expensive, like strings.Join.

We also keep the behavior in cmd/txtpbfmt unchanged.
Calls to Errorf still end up going to glog.Errorf,
and calls to Infof still end up going to glog.V(2).Infof.

Fixes protocolbuffers#70.
mvdan added a commit to mvdan/txtpbfmt that referenced this issue Nov 29, 2022
All calls go through a logger in the config.

The logger may be nil, so we check that it's non-nil everywhere.

We also keep the behavior in cmd/txtpbfmt unchanged.
Calls to Errorf still end up going to glog.Errorf,
and calls to Infof still end up going to glog.V(2).Infof.

Fixes protocolbuffers#70.
mvdan added a commit to mvdan/txtpbfmt that referenced this issue Jan 7, 2023
All calls go through a logger in the config.

The logger may be nil, so we check that it's non-nil everywhere.

We also keep the behavior in cmd/txtpbfmt unchanged.
Calls to Errorf still end up going to glog.Errorf,
and calls to Infof still end up going to glog.V(2).Infof.

Note that the library used the boolean value of glog.V(2) to tell
whether the "info" log level was enabled or not.
When disabled, this allowed us to not compute some log arguments,
such as calls to strings.Join or string copies.
Since we can't assume an interface to be of boolean type,
add an extra InfoLevel method to perform the same duty.

Fixes protocolbuffers#70.
kssilveira pushed a commit that referenced this issue Jan 9, 2023
As I was working on my patch for #70, I was wondering if we really
needed a logger interface with both Infof and Errorf. After all, a
library should report its errors as values, not via a logger.

Turns out it's relatively straightforward to do so. All functions that
used to call log.Errorf are themselves called from top-level APIs which
are able to return an error, so we just need to thread them.

One special case is getMetaCommentStringValues, which used to report
each error and continue rather than stop. We currently do not have an
idiomatic way to report multiple errors at once, so return the first
error for now. In the future, we could use https://go.dev/issue/53435.

Updates #70.
mvdan added a commit to mvdan/txtpbfmt that referenced this issue Jan 12, 2023
Second take, as the first merge was undone by a bot.

As I was working on my patch for protocolbuffers#70, I was wondering if we really
needed a logger interface with both Infof and Errorf. After all, a
library should report its errors as values, not via a logger.

Turns out it's relatively straightforward to do so. All functions that
used to call log.Errorf are themselves called from top-level APIs which
are able to return an error, so we just need to thread them.

One special case is getMetaCommentStringValues, which used to report
each error and continue rather than stop. We currently do not have an
idiomatic way to report multiple errors at once, so return the first
error for now. In the future, we could use https://go.dev/issue/53435.

Updates protocolbuffers#70.
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 a pull request may close this issue.

3 participants