encoding/protobuf: dependency on glog adds some flags to a program's std flag list

[myitcv]

Originally posted by knieriem August 8, 2021 in https://github.com/cue-lang/cue/discussions/1196

When upgrading a program importing a package from cue v0.3.0-beta* to v0.4.0 I noted that CUE's new dependency github.com/protocolbuffers/txtpbfmt adds another dependency, github.com/golang/glog (leveled execution logs for Go). go mod why lists, for instance:

cuelang.org/go/cue/load
  cuelang.org/go/internal/encoding
    cuelang.org/go/encoding/protobuf/textproto
       github.com/protocolbuffers/txtpbfmt/parser
         github.com/golang/glog

Glog, as a package, in its init function defines some command line flags using Go's standard flag package:

-alsologtostderr
-log_backtrace_at value
-log_dir string
-logtostderr
-stderrthreshold value
-v value
-vmodule value

As a result, any program importing, for example, cuelang.org/go/cue/load will automatically inherit these flags. In my program, which makes use of the standard flag package, I had been defining an own flag.Uint("v", 0, "verbosity level"). With cue v0.4.0 I'm getting a panic: ... flag redefined: v error now.

Since the cue program itself is using cobra+pflag, this effect is not visible when using cue itself, since flags defined using Go's standard flag package are ignored then.

An old thread at golang-nuts, glog and testing flag "pollution" mentions a workaround: defining one's own flag.FlagSet instead of using the default flag.CommandLine. But this also means, that all flags defined by a program's internal packages using e.g. flag.Bool must be adapted to the new FlagSet. An alternative to this workaround would be to make textproto depend on a modified txtpbfmt that does not depend on glog. I think for my use-case I will apply the method described in that golang-nuts thread.

[myitcv]

This seems unfortunate at best, but also something that we can fix via a PR to the github.com/protocolbuffers/txtpbfmt module.

[ying-jeanne]

having the same problem when trying to update to version 0.4.0

[verdverm]

Of note, using flag.FlagSet (https://pkg.go.dev/flag#FlagSet) can solve this issue.

Without it, you are using a "global" scope for flags, where you can have name collisions or extra flags added from deps that use the "global" flag space.

[gurjeet]

@myitcv can you please describe what's the difference between Discuss and zzz Discuss labels. Perhaps that'll explain why you change the label on this issue.

[myitcv]

@gurjeet ah that was just me trying to recategorise issues we needed to discuss prior to FOSDEM. The zzz Discuss category is now defunct so I will revert everything back to discuss.

[gurjeet]

The good first issue label made this problem look deceptively simple; false advertising :-)

But I have take a crack at solving this problem, so if the fix is merged, all users of the glog package will be able to override these flags at build time. See https://github.com/golang/glog/pull/54

[mvdan]

I think an easier way to fix this issue might be to change txtpbfmt to not import a CLI logging library in its relatively low level parser package:

https://github.com/protocolbuffers/txtpbfmt/blob/74888fd59c2b19248a51d8a5954db1588e18948f/parser/parser.go#L19

I personally think that's reasonable, because the user of the library should be in control of the logging - such as choosing what log library to use, or whether to use one at all. It seems like the parser only uses the library to make calls to Infof and Errorf, so perhaps those could be turned into a small interface to fit any similar logger into the parser API.

[mpvl]

I agree with @mvdan. It is overbearing for txtpbfmt to be importing glog, so ideally it should be fixed there.

We may also consider using a different package or forking txtpbfmt into the cue-lang org and removing it there. The added benefit of that is that we may create some stability and control around the use of text protos, for which there is no real standard. But ideally the changes would be made upstream.

[mvdan]

I've gone ahead and asked nicely: https://github.com/protocolbuffers/txtpbfmt/issues/70

[mvdan]

Took a long time to agree with upstream and get the changes reviewed, but it finally looks like both of my PRs are being merged. We should be able to update txtpbfmt and drop glog as a dependency soon.

[mvdan]

Now merged :) The glog dependency should be gone in master and the upcoming v0.6.0-alpha.1 release.