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

-V is messed up #9

Open
MikeSpreitzer opened this issue Dec 22, 2022 · 14 comments
Open

-V is messed up #9

MikeSpreitzer opened this issue Dec 22, 2022 · 14 comments
Assignees
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@MikeSpreitzer
Copy link
Contributor

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
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 22, 2022
@pohly
Copy link
Contributor

pohly commented Dec 22, 2022

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
Copy link
Contributor

pohly commented Dec 22, 2022

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

/tmp/go-build1772735640/b001/exe/logcheck version devel comments-go-here buildID=26e6823bdfa8019bf8b28b2db15a458f3b02c4eab7bca826a47c51be75fcc208

@MikeSpreitzer
Copy link
Contributor Author

MikeSpreitzer commented Jan 5, 2023

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
Copy link
Contributor

pohly commented Jan 9, 2023

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:

singlechecker.Main(pkg.Analyser())

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

@MikeSpreitzer
Copy link
Contributor Author

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

@pohly
Copy link
Contributor

pohly commented Jan 9, 2023

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
Copy link

/triage accepted
/assign @pohly

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 12, 2023
@MikeSpreitzer
Copy link
Contributor Author

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

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

@pohly
Copy link
Contributor

pohly commented Feb 1, 2023

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

@pohly
Copy link
Contributor

pohly commented Feb 1, 2023

@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
Copy link
Contributor Author

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

@k8s-triage-robot
Copy link

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

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

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

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Feb 3, 2024
@dashpole
Copy link

dashpole commented Mar 7, 2024

@pohly is this still relevant?
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 7, 2024
@pohly
Copy link
Contributor

pohly commented Mar 7, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

6 participants