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

Jump to bottom

x/tools/go/analysis/singlechecker: don't use flag.CommandLine #30231

Closed
mvdan opened this issue Feb 14, 2019 · 5 comments
Closed

x/tools/go/analysis/singlechecker: don't use flag.CommandLine #30231

mvdan opened this issue Feb 14, 2019 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@mvdan
Copy link
Member

mvdan commented Feb 14, 2019

Using package flag's global flagset is convenient, but shouldn't happen in packages like singlechecker and unitchecker which may be used in many different ways.

In particular, lots of packages like to abuse the same global flagset, such as testing and net/http/pprof, among many others outside the standard library.

Granted that a linter should generally not import those packages to begin with. But sometimes you just can't control your dependencies. And there's the case which I hit - I'm running my tool as part of a test, so testing is in the build import graph. So one of my tests is failing as the testing flags are getting mixed into the usage output.

I presume we could have unitchecker export a global flagset, and singlechecker could use it too.
@gopherbot gopherbot added this to the Unreleased milestone Feb 14, 2019
@rogpeppe rogpeppe mentioned this issue Feb 18, 2019
Closed
@bcmills
Copy link
Contributor

bcmills commented Feb 28, 2019

I'm running my tool as part of a test, so testing is in the build import graph.

Um... How does that follow? (Shouldn't the test exec the tool, the same as users will?)

(CC @ianthehat @alandonovan)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 28, 2019
@mvdan
Copy link
Member Author

mvdan commented Feb 28, 2019

Yes, the test uses exec to run the tool as a binary. But it doesn't require a separate binary to be somewhere on disk; instead, it adds some code in TestMain so that running the test binary with a certain env var will run that tool's main function instead of the tests.

I know this is a bit weird to try to summarize. Search for TestMain in https://godoc.org/github.com/rogpeppe/go-internal/testscript.

@alandonovan
Copy link
Contributor

This seems like a variation on #30219. The single and multichecker packages are not designed to be used in this way. Please fork/exec a program for which they define the main function.

@mvdan
Copy link
Member Author

mvdan commented Mar 3, 2019

Yes, it is quite similar. Feel free to close it as a duplicate. The reason I opened it separately is because not using flag.CommandLine is quite easy, and there's no apparent benefit to sticking to it.

@mvdan
Copy link
Member Author

mvdan commented Mar 21, 2019

Seems like we're converging on adding a "cover" package in the standard library in the other thread; closing this one.

@mvdan mvdan closed this as completed Mar 21, 2019
@golang golang locked and limited conversation to collaborators Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@mvdan @bcmills @alandonovan @gopherbot