fix: rebuild binary if it has been deleted or modified #712
Conversation
|
I've got an issue.
|
You mean you introduce a bug, and it is not caught on a second serve? |
|
Syntax error in a handler of the default module. |
|
Yes, when I |
|
Good catch👍 |
There was a problem hiding this comment.
This is good, but I still think that we should try to match source's hash with the binary's hash in addition to this impl in order to secure a determenistic behavior. Otherwise, starport may use a totally different binary because it has the same name. ~this can be done in a seperate PR also.
starport/services/chain/build.go
Outdated
| @@ -185,3 +185,20 @@ func (c *Chain) buildProto(ctx context.Context) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func (c *Chain) isBuilt() (bool, error) { | |||
There was a problem hiding this comment.
We can use https://golang.org/pkg/os/exec/#LookPath instead.
That's a good point. It can become a problem if the user work with different branch or if we implement a workspace system in the future so let's address it in this PR. I don't think we even need to implement a mapping of a source checksum to binary. We just store the binary checksum along with the source checksum and save it at each build and build again each time this checksum is different. So it will also return true if the binary don't exist. I'm also thinking, based on this problematic, we can consider that if the binary is different. Then it may be possible that the user has done some eventual side-effect actions that make the current database eventually incorrect. It this case we must reimport the genesis as well instead of just building the binary |
| if strings.Contains(err.Error(), "file not found") { | ||
| binaryModified = true | ||
| } else { | ||
| return err |
There was a problem hiding this comment.
Can we refactor the whole section by using early returns? I think code becomes much more readable that way.
There was a problem hiding this comment.
What do you mean with the "whole section"?
This part can actually be reversed for better clarity, tell me if it's good like this
There was a problem hiding this comment.
I mean something like this:
binaryName, err := c.Binary()
if err != nil {
return err
}
binaryPath, err := exec.LookPath(binaryName)
if !errors.Is(err, exec.ErrNotFound) {
return err
}
binaryModified := err != nil
if err == nil {
binaryModified, err = dirchange.HasDirChecksumChanged("", []string{binaryPath}, saveDir, binaryChecksum)
if err != nil {
return err
}
}There was a problem hiding this comment.
I personally find it a bit less clear. We check the err even if it's nil and binaryModified := err != nil is a bit ambiguous IMO
There was a problem hiding this comment.
What about the moving errors.Is to upper level part?
There was a problem hiding this comment.
What do you think of:
binaryPath, err := exec.LookPath(binaryName)
if err != nil && !errors.Is(err, exec.ErrNotFound) {
return err
}
if errors.Is(err, exec.ErrNotFound) {
binaryModified = true
} else {
binaryModified, err = dirchange.HasDirChecksumChanged("", []string{binaryPath}, saveDir, binaryChecksum)
if err != nil {
return err
}
}There was a problem hiding this comment.
I think current way is just fine. 👍
Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com>
…into fix/no-binary
From the discussion: #707
Build the binary and skip the initialization phase if the binary has been deleted but the source code has not been modified.