Skip vendor directories for go.mod extraction

[sauyon]

Sanity check dist-compare shows no result or performance changes as expected.

Should I also exclude directories starting with [._] and testdata like ./... does?

[max-schaefer]

Should I also exclude directories starting with [._] and testdata like ./... does?

Matching the behaviour of ./... sounds like a good idea.

In fact, taking a step back, is there a way we could determine the go.mod files to extract by looking at the module root of each package the extractor processes?

[sauyon]

In fact, taking a step back, is there a way we could determine the go.mod files to extract by looking at the module root of each package the extractor processes?

Right, this would be much nicer. I'll implement this.

[sauyon]

This now uses the code from #170 to detect module roots and only extract go.mod files that are relevant.

An ad-hoc dist-compare shows 369 removed results, which seem to be mostly results from inside vendor directories. (From a very quick look through of the sarif).

[max-schaefer]

This now uses the code from #170 (...)

OK, then I'll hold off on reviewing this until that PR has landed.

[max-schaefer]

Could you rebase this now that #170 has been merged?

[max-schaefer]

Ping @sauyon, I think this should be a shoo-in once rebased.

[sauyon]

Sorry about that, I've rebased it.

[sauyon]

Somewhat cleaner diff than what GitHub seems to generate:

diff --git a/extractor/extractor.go b/extractor/extractor.go
index 4bcbe142..d4465b3c 100644
--- a/extractor/extractor.go+++ b/extractor/extractor.go@@ -161,49 +161,29 @@ func ExtractWithFlags(buildFlags []string, patterns []string) error {
}
extractPackage(pkg, &wg, goroutineSem, fdSem)
++ if pkgRoots[pkg.PkgPath] != "" {+ modPath := filepath.Join(pkgRoots[pkg.PkgPath], "go.mod")+ if util.FileExists(modPath) {+ log.Printf("Extracting %s", modPath)+ start := time.Now()++ err := extractGoMod(modPath)+ if err != nil {+ log.Printf("Failed to extract go.mod: %s", err.Error())+ }++ end := time.Since(start)+ log.Printf("Done extracting %s (%dms)", modPath, end.Nanoseconds()/1000000)+ }+ }+
return
}
})
wg.Wait()
-	cwd, err := os.Getwd()-	if err != nil {- log.Printf("Warning: unable to get working directory: %s", err.Error())- log.Println("Skipping go.mod extraction")-	}-	rcwd, err := filepath.EvalSymlinks(cwd)-	if err == nil {- cwd = rcwd-	}--	goModPaths := make([]string, 0, 10)--	filepath.Walk(cwd, func(path string, info os.FileInfo, err error) error {- if filepath.Base(path) == "go.mod" && info != nil && info.Mode().IsRegular() {- if err != nil {- log.Printf("Found go.mod with path %s, but encountered error %s", path, err.Error())- }-- goModPaths = append(goModPaths, path)- }-- return nil-	})--	for _, path := range goModPaths {- log.Printf("Extracting %s", path)- start := time.Now()-- err := extractGoMod(path)- if err != nil {- log.Printf("Failed to extract go.mod: %s", err.Error())- }-- end := time.Since(start)- log.Printf("Done extracting %s (%dms)", path, end.Nanoseconds()/1000000)-	}-
return nil
}

[max-schaefer]

Thanks, that diff is indeed much nicer! I would have said LGTM, but the test failures seem to suggest that we now fail to extract many (all?) go.mod files.

[sauyon]

Yep, I just took a look, it's because we didn't have a building Go project around the go.mod files. I assume we aren't worried about that case?

I'm fixing it now, though it's a bit more annoying than I expected for the dependencies test.

[max-schaefer]

it's because we didn't have a building Go project around the go.mod files

You mean they are free-floating go.mod files without any Go files next to them? If so, then I'm not worried.

I'm fixing it now

Thanks!

[sauyon]

So I've run into an issue with this:

It seems like local imports don't play very nicely with the build system; they don't seem to work at all when importing those packages, which means having a project that just imports them with some replaces doesn't work.

Just building with the ./... pattern doesn't work because it doesn't automatically find Go files under different modules.

Also, because we must vendor dependencies (and even local replacements are vendored), go.mod files that are dependencies might not be extracted anyway.

My current plan is to create a mock go.mod file along with fake vendored dependencies to go with it.

[max-schaefer]

Looking at your comment above again, I must confess that I have lost all context, and am no longer able to make sense of it. I thought the reason we weren't extracting the go.mod files was that there weren't any Go source files next to them, so it would be a simple matter of adding some trivial source files. I take it the situation is more complex than that, and "local imports" are somehow a part of the problem?

[sauyon]

...so it would be a simple matter of adding some trivial source files. I take it the situation is more complex than that, and "local imports" are somehow a part of the problem?

I did as well, but it turns out that ./... does not actually cross Go module boundaries, so only the top-level go.mod file was being extracted.

Is there a way to force custom build commands? (EDIT: for tests?)

[max-schaefer]

Is there a way to force custom build commands? (EDIT: for tests?)

Not at the moment, no.

[sauyon]

I believe tests should finally be working now. I'm currently reworking the GoModExpr tests to be comment-based.

[sauyon]

This should be ready for review now.

[smowton]

Looks good -- what's the evaluation status? Has this changed enough since your distcompare at the top that it ought to be rerun? Is this likely enough to break extraction that we should run an lgtm distribution test?

[smowton]

Also are there user-visible changes worthy of a change-note?

[sauyon]

Looks good -- what's the evaluation status? Has this changed enough since your distcompare at the top that it ought to be rerun? Is this likely enough to break extraction that we should run an lgtm distribution test?

The only real changes here are test changes (and the rebase), so hopefully nothing should have changed.

Also are there user-visible changes worthy of a change-note?

I'll add a change note.

[sauyon]

I think this should be ready for review now.

[smowton]

Haven't looked at the test changes in great detail but the extractor change itself looks good.