Don't panic on missing pkg.Module when GOPACKAGESDRIVER set

[podtserkovskiy]

GOPACKAGESDRIVER's JSON doesn't provide Module field.
I've changed panic to an error message to enable usage of scip-go with third-party build-systems like Buck2 or Bazel.

Related: #3

[varungandhi-src]

1. Could you elaborate on why we need to work around this here instead of filing and fixing the upstream issue where the JSON struct doesn't provide a Module field?
2. What exactly is the workflow for using GOPACKAGESDRIVER?
3. I believe that field is used for cross-repo navigation. Is the expectation that cross-repo navigation will not work when using GOPACKAGESDRIVER?

I'm not opposed to merging this PR, but I'd like to better understand how we're supposed to test and maintain this, even if that requires manual steps.

[podtserkovskiy]

Thanks for reviewing this PR

1. Could you elaborate on why we need to work around this here instead of filing and fixing the upstream issue where the JSON struct doesn't provide a Module field?

I think this is implemented only for go.mod because "module" is go mod specific thing. @Tatskaari explained his use-case in the issue and we have a similar use-case a single giant monorepo with all the dependencies vendored. We don't need cross-repo navigation because we don't have multiple repos.

2. What exactly is the workflow for using GOPACKAGESDRIVER?

GOPACKAGESDRIVER is an adapter between a specific build-system and Go tools, it's used under the hood of packages.Load() (instead of go list) and retrieves the package information from the build-system.

Then it can be used with Go tools like this

GOPACKAGESDRIVER=mydriver scip-go
GOPACKAGESDRIVER=mydriver gopls serve
GOPACKAGESDRIVER=mydriver golangci_lint run

3. I believe that field is used for cross-repo navigation. Is the expectation that cross-repo navigation will not work when using GOPACKAGESDRIVER?

It won't work and this is okay the goal of issue #3 is to enable scip-go to work on monorepos using Buck/Bazel/Please/etc.

[varungandhi-src]

Understood. Could you please do the following:

1. In README.md, break up the section for "Indexing a Go project" into two sub-sections -- one for go.mod and one for without go.mod.
2. In the second sub-section, document:
   • A small example of how GOPACKAGESDRIVER should be used. It would be useful to link an OSS Go repo which uses Bazel/Buck2/Please and doesn't have go.mod if possible.
   • Document that cross-repo navigation may not work when GOPACKAGESDRIVER is present (or "will not work" if that'd be more accurate)
3. Log a warning in the code if the module information is not found and GOPACKAGESDRIVER was not set, stating that cross-repo navigation may not work.

If you don't have time to implement the above, let me know, and I'll work on adding these.

Other question: we have this section in the README.

scip-go by default uses a few different go commands from the command line to gain information about the project and module. To avoid running go directly (perhaps you have some other build system), you will need to supply the folling args.

Do we need to update the code shelling out to the go binary as well?

[podtserkovskiy]

Do we need to update the code shelling out to the go binary as well?

I think go binary should not be called when GOPACKAGESDRIVER provided.
We probably should add an extra if in the code for this case.

UPD: Actually I feel like "Indexing without shelling to go binary" section can be removed if we disable the shelling when GOPACKAGESDRIVER set

[podtserkovskiy]

I did few changes:

• Added new field config.IndexOpts.IsGoPackagesDriverSet
• The constructor config.New checks env-var GOPACKAGESDRIVER and sets IsGoPackagesDriverSet
• The loader.normalizePackage function doesn't change its behaviour unless GOPACKAGESDRIVER set. So all the technically I returned the panic back and can rename this PR 😀
• Updated Readme.md as you advised

I haven't touched the code related to shelling go binary yet.
The config.IndexOpts doesn't exist when main.go calls modules.ModuleName so it can't access IsGoPackagesDriverSet. We probably can call modules.ModuleName from config.New to avoid this problem.

[varungandhi-src]

Thanks for addressing the comments so far.

The config.IndexOpts doesn't exist when main.go calls modules.ModuleName so it can't access IsGoPackagesDriverSet. We probably can call modules.ModuleName from config.New to avoid this problem.

There is also a code path initializing defaultGoVersion which needs a Go stdlib version for cross-repo navigation. Given that supporting cross-repo navigation with a custom driver is not a goal here, it's fine to use a placeholder value here with an explicit TODO.

[varungandhi-src]

Suggested change

	if opts.IsGoPackagesDriverSet {
	// As of golang.org/x/tools v0.26, the JSON object for representing packages
	// does not have a Module field:
	// https://github.com/golang/tools/blob/v0.26.0/go/packages/packages.go#L609-L627
	// which means that cross-repo navigation may not work out-of-the-box
	// for 3rd party build systems like Bazel, Buck2, Please etc. using GOPACKAGESDRIVER
	if opts.IsGoPackagesDriverSet {

[podtserkovskiy]

Thanks, I applied the code suggestion.

There is also a code path initializing defaultGoVersion which needs a Go stdlib version for cross-repo navigation. Given that supporting cross-repo navigation with a custom driver is not a goal here, it's fine to use a placeholder value here with an explicit TODO.

I guess we can use defaultGoVersion for now, if someone needs to have a specific value here, they can use --go-version flag

[podtserkovskiy]

Thank you for the review&merge 😀

[JamyDev]

FWIW around adding the module field, we asked about this a year ago: golang/go#62601

[podtserkovskiy]

@JamyDev Thanks for the heads up, but I'm not sure what what is the closest analogue of "Module" we can have in Buck. The module seems to be the go tool specific thing, since this tool combines a build-system and package manager.

We definitely can emit module information for third-party packages, but I don't have a clear picture of a first-party module implementation in Buck.