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

x/tools/packages: analysis fails completely if a module version cannot be determined #40187

Closed
sauyon opened this issue Jul 13, 2020 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@sauyon
Copy link

sauyon commented Jul 13, 2020

$ go version
ql/src/semmle/go/frameworks/Testing.qll

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/sauyon/.cache/go-build"
GOENV="/home/sauyon/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/sauyon/devel/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/sauyon/devel/codeql-go/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build836052876=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Try to run tools/packages.Load on any source package that has broken modules.

What did you expect to see?

Some, albeit incomplete, information, especially AST for files that are in the package (and can be resolved).

What did you see instead?

An error.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 13, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jul 13, 2020
@andybons andybons changed the title x/tools/packages: Analysis fails completely if a module version cannot be determined x/tools/packages: analysis fails completely if a module version cannot be determined Jul 13, 2020
@andybons
Copy link
Member

Can you please provide a simple reproduction that has more detail? It’s unclear what you mean by “broken modules” and saying “An error” is also quite cryptic.

Thanks.

/cc @matloob

@andybons andybons added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jul 13, 2020
@sauyon
Copy link
Author

sauyon commented Jul 14, 2020

Any time modules cannot be resolved, such as with private modules or the following:

module test

go 1.14

require github.com/nonexistent/package v1.1

Running Load will fail with something like

[2020-07-14 09:15:17] [build-err] 2020/07/14 09:15:17 go [-e -json -compiled=true -test=false -export=false -deps=true -find=false -- ./...]: exit status 1: go: errors parsing go.mod:
[2020-07-14 09:15:17] [build-err] /home/sauyon/devel/test/go.mod:5: require github.com/nonexistent/package: git ls-remote -q origin in /home/sauyon/devel/go/pkg/mod/cache/vcs/139553494be65d19324b2f01a845543af52f84ae4003b210e1d50bdd18e263c9: exit status 128:
[2020-07-14 09:15:17] [build-err] 	fatal: could not read Username for 'https://github.com': terminal prompts disabled
[2020-07-14 09:15:17] [build-err] Confirm the import path was entered correctly.

This is probably because this is the behavior of go list -e; perhaps this issue should have been filed for the go list command instead?

@matloob
Copy link
Contributor

matloob commented Jul 14, 2020

@bcmills @jayconrod

Maybe I'm missing all the details, but this seems to be working as intended? In general, loading packages depends on getting the dependencies for the packages being analyzed. If we can't fetch the modules in the dependency tree, we can't get the build list and determine what gets built, which in turn means that we can't do the build.

There might be a way around this if you just want to determine the set of files in a package with the appropriate flags to load (no export data, type information, etc.), but even that is difficult and error-prone.

@jayconrod
Copy link
Contributor

Building on what @matloob said, a working go.mod file is necessary for loading almost any information about packages. It's necessary just for translating package paths to directories. I'm sure there are cases where we could fail a little more gracefully, but it would take a lot of effort (significant changes in the go command).

I'm not sure I understand the motivation here. Why is it important that this work when go.mod refers to modules that don't exist? Why not fix go.mod first?

@sauyon
Copy link
Author

sauyon commented Jul 14, 2020

I only reported this as a bug because it's a regression from pre-module behavior, which would still give you partial information about the dependencies that were resolved. In this case deleting the go.mod file gives more info, which feels like it shouldn't be the case.

@sauyon
Copy link
Author

sauyon commented Jul 14, 2020

I'm not sure I understand the motivation here. Why is it important that this work when go.mod refers to modules that don't exist? Why not fix go.mod first?

We use packages in our CodeQL Go analysis, and our goal is to extract as much information as we can without user input. This sometimes means that dependencies aren't resolved properly (particularly with proprietary codebases). I'd understand if this isn't a priority, but I think it would still be helpful in general from a static analysis tool standpoint.

@matloob
Copy link
Contributor

matloob commented Jul 24, 2020

Okay, thanks for the context. This use case is unfortunately in the category of being difficult enough to support that the effort required to support it wouldn't be worth the gains: we'd have to make major changes to cmd/go to try to continue working even if module loading fails and those changes wouldn't be justified by this use case.

One option you could take if you want incomplete data would be to use the -modfile flag: you could make a version of the go.mod file that either adds replaces or completely removes the broken modules, and use -modfile with that alternative go.mod file. Then module loading will succeed, and your build will fail later on. You might be able to get more information that way.

I'm going to close this as "unfortunate".

@matloob matloob closed this as completed Jul 24, 2020
@sauyon
Copy link
Author

sauyon commented Jul 28, 2020

That's understandable. I think the workaround should work well enough for our use-case (and frankly if somebody wants full analysis they should figure out their builds anyway...). Thanks!

@matloob
Copy link
Contributor

matloob commented Jul 29, 2020

Glad to hear that the workaround should work. Thanks for the feedback!

@golang golang locked and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

5 participants