- 
                Notifications
    
You must be signed in to change notification settings  - Fork 9
 
Bugfix: prevent spk build failing if completed builds disk usage calcs error #1280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…so have no size Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
          Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know!  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did run into this with some of my source builds as well, thanks for the fix
        
          
                crates/spk-storage/src/disk_usage.rs
              
                Outdated
          
        
      | /// components) in the given repo(s). | ||
| /// | ||
| /// Note: builds with no files (at all) will return a | ||
| /// DiskUsageBuildNotFound error because at least on file is needed to | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// DiskUsageBuildNotFound error because at least on file is needed to | |
| /// DiskUsageBuildNotFound error because at least one file is needed to | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though IMO it should return a success of 0 rather than "build not found" in this case. I don't consider this situation an error, and the error name doesn't fit the situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is not intuitive returning "build not found" when it clearly is present, it just has no files on disk to get disk usage sizes from.
        
          
                crates/spk-storage/src/disk_usage.rs
              
                Outdated
          
        
      | /// Note: builds with no files (at all) will return a | ||
| /// DiskUsageBuildNotFound error because at least on file is needed to | ||
| /// calculate a walked disk usage entry. For example, commands like | ||
| /// 'spk pip convert ...' can generate /src builds for python | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// 'spk pip convert ...' can generate /src builds for python | |
| /// 'spk convert pip ...' can generate /src builds for python | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this and the one above.
Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
This fixes a bug that causes
spk buildcommands to appear to fail when gather disk usage encounters an error during the "Completed builds: ..." output at the end of thespk build. The builds haven't failed, but the command errors out. The fix is to catch the disk uage error and use zero size for that build (or version).The situation can happen when using
spk pip conver ...on a python module. The generated (placeholder) source build for the python module contains no files in its component layers, and thus nothing to measure for disk usage, which generates a disk usage error.