Prepare for possible move to go modules #970
Conversation
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com> Change-Id: I44a43db4bc848f27cf9598fce8189328ac2c2c90
The module path should be exercised for package and install but is not needed when building chaincode with the external binary builder. - Replace most use of chaincode/module with chaincode/simple - Move signal handling from chaincode/module to chaincode/simple Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com> Change-Id: I241f3cf9dce1812bc03205a59960ab35b2fcd8b5
There was a problem hiding this comment.
I think the gomod/gopath determination for finding the file is elegant and correct for this situation.
I can't speak to the simplecc change. Glancing at the directory in the location, I don't know how to figure out what it is.The directory could use a Readme or a comment to explain it's existence in the location.
Agree as to being a separable change in preparation. 👍
| path, err := gomodDevConfigDir() | ||
| if err != nil { | ||
| path, err = gopathDevConfigDir() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| } | ||
| return path |
There was a problem hiding this comment.
The barest of nits. This reads better straight line with no indentation. I don't think the single exit point helps in a function this short. I think it's clearer in terms of, use gomod first, use gopath second.
path, err := gomodDevConfigDir()
if err == nil {
return path
}
path, err = gopathDevConfigDir()
if err == nil {
return path
}
panic(err)
It can even be collapsed with the assignable-if
var err error
if path, err := gomodDevConfigDir(); err == nil {
return path
}
if path, err := gopathDevConfigDir(); err == nil {
return path
}
panic(err)
There was a problem hiding this comment.
I thought about what you suggested above. But, we need three return path statement and especially the last return path which comes after panic(err) is a dead code. IMO, the current structure looks good and no changes required.
There was a problem hiding this comment.
@MHBauer If the panic(err) can be avoided, what you suggested is applicable and makes the code more readable.
|
|
||
| func gomodDevConfigDir() (string, error) { | ||
| buf := bytes.NewBuffer(nil) | ||
| cmd := exec.Command("go", "env", "GOMOD") |
There was a problem hiding this comment.
This makes sense to me and seems more flexible.
| @@ -27,7 +27,7 @@ if [ -f "$SOURCE/src/go.mod" ]; then | |||
| cd "$SOURCE/src" | |||
| go build -v -mod=readonly -o "$OUTPUT/chaincode" "$GO_PACKAGE_PATH" | |||
| else | |||
| GO111MODULE=off go build -v -o "$OUTPUT/chaincode" "$GO_PACKAGE_PATH" | |||
| GOPATH="$SOURCE" GO111MODULE=off go build -v -o "$OUTPUT/chaincode" "$GO_PACKAGE_PATH" | |||
There was a problem hiding this comment.
So before this gopath was completely unset, and was the default set depending on go version, and the fact that it built was because everything was already checked out in the gopath. I think this makes sense.
| @@ -38,15 +43,55 @@ func dirExists(path string) bool { | |||
| // maintained with the source tree. This should only be used in a | |||
| // test/development context. | |||
| func GetDevConfigDir() string { | |||
There was a problem hiding this comment.
I like the structure -- highly readable.
Just one minor suggestion:
In the formatting rules section, I have specified the following:
- Closely related concepts should not be kept far within a file; Should never be kept in a different file.
- If one function calls another, they should be vertically close. The caller should be placed above the callee.
- Certain bits of code which has a conceptual affinity towards other code, they should be kept together. Affinity might be caused because a group of functions performs similar operations.
- The most important concept first and the low-level details in the bottom.
While the current code structure satisfies these rules somewhat, it would be nice to have gomodDevConfigDir() immediately after the GetDevConfigDir() followed by gopathDevConfigDir(). The structure can look similar to how various function calls are made in the top-level function that is GetDevConfigDir()
When I read the top-level function, I searched for gomodDevConfigDir and had to browse the code a bit more to find out where it is located.
As these points are highly debatable, I will leave the decision to you.
There was a problem hiding this comment.
I'd say feel free to create a change on top of this if you like @cendhu
There was a problem hiding this comment.
Yeah, this is simple to do. Will push a PR.
| } | ||
| } | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
This is the top-level function in this file. IMO, this should be at the beginning of this file. The reason is that, first, I read handleSignals, then I read handleSigTerm followed by main(). Finally, I had to again read handleSignals and handleSigTerm by keeping main() in the context. Probably, I need to improve my code reading skills. May be the rearrangement might help me.
These changes isolate GOPATH dependencies and enable a possible move to go modules. These changes are focused on two areas:
Discovery of the sampleconfig resources during test
If the build is run in module mode, the
sampleconfigdirectory is calculated as relative to the module root of the test. If the build is run in GOPATH mode, the directory is calculated as relative to theGOPATHUse "simple" chaincode instead of a module in most cases
There were a number of places in the integration tests where we would attempt to build a chaincode module from an import path reference. This works in GOPATH mode but fails in module mode. These changes replace the use of the
modulechaincode withsimplechaincode where appropriate assimplechaincode could be built within the context of the module hosting the integration tests.