kubernetes-sigs / logtools

Tools related to log calls for Kubernetes.
Apache License 2.0
13 stars 16 forks source link

-V is messed up #9

Open MikeSpreitzer opened 1 year ago

MikeSpreitzer commented 1 year ago

logcheck -h says

...
  -V    print version and exit
...

but when I try it something else happens.

(base) mspreitz@mjs12 logcheck-test % logcheck -V
logcheck: unsupported flag value: -V=true
pohly commented 1 year ago

That comes from https://github.com/golang/tools/blob/6546d82b229aa5bd9ebcc38b09587462e34b48b6/go/analysis/internal/analysisflags/flags.go#L197 in golang.org/x/tools. I don't think we can do anything about it.

pohly commented 1 year ago

According to the source, only go run . -V=full works:

/tmp/go-build1772735640/b001/exe/logcheck version devel comments-go-here buildID=26e6823bdfa8019bf8b28b2db15a458f3b02c4eab7bca826a47c51be75fcc208
MikeSpreitzer commented 1 year ago

The output of logcheck -h gives no clue about how to successfully use -V. That is a problem. Maybe the code in https://github.com/golang/tools/blob/6546d82b229aa5bd9ebcc38b09587462e34b48b6/go/analysis/internal/analysisflags/flags.go#L197 is correct for the context where it appears. The calling of that code from logcheck makes for a baffling logcheck user experience.

pohly commented 1 year ago

The calling of that code from logcheck makes for a baffling logcheck user experience.

But logcheck is not calling it, is it? Its main.go is just doing this:

https://github.com/kubernetes-sigs/logtools/blob/0d66178d0c69b17a158d6de47c758ce94ac51c47/logcheck/main.go#L26

I agree that this is bad, but it would have to be fixed in golang.org/x/tools/go/analysis/singlechecker.

MikeSpreitzer commented 1 year ago

Is there problem here common to all uses of singlechecker.Main or is logcheck somehow special in this regard?

pohly commented 1 year ago

I don't think logcheck is special.

https://grep.app/search?q=singlechecker.Main shows plenty of other examples that use the same main.go as logcheck.

I checked out https://github.com/google/gvisor/blob/master/tools/checklocks/cmd/checklocks/main.go and got the same failure:

/tmp/gvisor/tools/checklocks/cmd/checklocks$ go run ./ -V
checklocks: unsupported flag value: -V=true
exit status 1
logicalhan commented 1 year ago

/triage accepted /assign @pohly

MikeSpreitzer commented 1 year ago

@pohly is right, logcheck is not special. I pursued this issue upstream and got it mostly resolved, see https://github.com/golang/go/issues/57716 .

Can we get a new point release to pick up this improvement?

pohly commented 1 year ago

I looked at the solution and believe that further work is needed there. See my comments in https://github.com/golang/go/issues/57716 and https://go-review.googlesource.com/c/tools/+/461496?tab=comments.

pohly commented 1 year ago

@MikeSpreitzer: is the current solution in Go good enough for you? if yes, then I can update once it is in a tagged release and do a new release of logtools itself.

MikeSpreitzer commented 1 year ago

@pohly : I agree with you that it would be better to fix -h.

k8s-triage-robot commented 8 months ago

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

dashpole commented 7 months ago

@pohly is this still relevant? /triage accepted

pohly commented 7 months ago

@MikeSpreitzer: my understanding was that like me you wanted to get -h fixed upstream, which IMHO hasn't happened.

Can you clarify what we should be doing here?