go-fuzz-build fix: not-package dirs #311
Conversation
guzenok
force-pushed
the
fix/cgo-build
branch
2 times, most recently
from
48e365b to
f9104df
Compare
go-fuzz-build/pkgs.go
Outdated
| // Copyright 2015 go-fuzz project authors. All rights reserved. | ||
| // Use of this source code is governed by Apache 2 LICENSE that can be found in the LICENSE file. | ||
|
|
||
| package main |
There was a problem hiding this comment.
I don't see that these functions warrant a new file. Please place them in main.go along with the rest, near where they are used.
go-fuzz-build/pkgs.go
Outdated
| ) | ||
|
|
||
| // packageDir returns local directory with package source files. | ||
| func packageDir(p *packages.Package) string { |
There was a problem hiding this comment.
This doesn't need to be a function. Using filepath.Dir(p.GoFiles[0]) inline, the way it was, is better. The comment that p.GoFiles is non-empty is useful and should stay, but it could be shortened to // p.GoFiles is always non-empty.
go-fuzz-build/pkgs.go
Outdated
| return dir | ||
| } | ||
|
|
||
| // isNotPackage checks if dir contains go source files. |
There was a problem hiding this comment.
Negatives are harder to read at call sites. I'd prefer isPackage, and using !isPackage at call sites if needed. This comment should read: // isPackage reports whether dir contains Go source files.. However, that's not really the right condition, because whether a Go source file is relevant depends on whether it is excluded by current build tags, etc. It'd be better to rely on go/packages to tell us whether a directory is a package or not. If it is a package that we care about, then go/packages will return a Package for it. So perhaps a better implementation is to extract filepath.Dir for every Package returned by go/packages and then check whether dir is present in that set.
The reason I'm not sure whether that's a better implementation is that I still don't understand why this PR is necessary. The description claims that it is necessary, but it doesn't explain:
- why we should expect non-package directories to be important (what do they contain)
- where we should look for them (all subdirs of package dirs? recursively in all subdirs of package dirs?) and why we should look for them there
- how to tell when we should be looking for non-package directories
- why this won't cause us to accidentally copy large unnecessary unrelated data directories that might be present in other code bases
- why this needs to be fixed in go-fuzz-build instead of in the code base that encountered the issue
Before writing any more code, please answer this set of questions, because it will guide what the code should do. Thank you.
There was a problem hiding this comment.
why we should expect non-package directories to be important (what do they contain) ?
As it was shown by example, such non-package dirs could contain C-code files. And go-code includes they by CGO.
There was a problem hiding this comment.
where we should look for them (all subdirs of package dirs? recursively in all subdirs of package dirs?) and why we should look for them there?
There is no 100% right answer - they could be anywhere. I use the assumption that all non-go code the go-repo needs is in git-repo. So we should copy all subdirs tree of the repo. But it leads us to the next question:
why this won't cause us to accidentally copy large unnecessary unrelated data directories that might be present in other code bases?
Of cause, this could cause to. But it is better than build fail. I try to minimize data copying by crawling only by non-package subdirs of go-package (not whole go-repo which could could contain a lot of go-packages). It is next level assumption and compromise.
There was a problem hiding this comment.
how to tell when we should be looking for non-package directories?
We cannot anticipate all cases when non-package dirs are neccesary for build, so we must always copy them.
There was a problem hiding this comment.
why this needs to be fixed in go-fuzz-build instead of in the code base that encountered the issue?
The code base that encountered the issue is in his own right. Go-code can include C-code. CGO is a property of the go.
There was a problem hiding this comment.
Did my answers explain the change reason?
guzenok
force-pushed
the
fix/cgo-build
branch
from
f9104df to
a8e90a2
Compare
The fix makes Context.clonePackage() also copies not-package dirs, which are needed for build.
Not-package dir is a subdirectory of go-package directory, which doesn't contains any go-file. I.e. it is not a go-package, it can't be imported and it is not reflected by "golang.org/x/tools/go/packages".
The ethereum libsecp256k1 is an example of such not-package dir. It is needed for "github.com/ethereum/go-ethereum/crypto/secp256k1" building but ignored by current Context.clonePackage(). PR fixes it.