Skip to content

mage: Improve error messages and resource handling #10446

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

mage: Improve error messages and resource handling #10446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb5dcd0
mage: Improve error messages and resource handling
orestisfl Oct 9, 2025
c003fb1
Merge branch 'main' into devtools-nitpicks
orestisfl Oct 9, 2025
62f1634
fix test
orestisfl Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dev-tools/mage/downloads/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func verifyChecksum(checksumFile string) error {
}

if expectedChecksum != actualChecksum {
return fmt.Errorf("checksum of file %s does not match expected checksum of %s", fileName, actualChecksum)
return fmt.Errorf("%s checksum mismatch: expected=%s actual=%s", fileName, expectedChecksum, actualChecksum)
}

return nil
Expand Down
3 changes: 2 additions & 1 deletion dev-tools/mage/downloads/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ func TestVerifyChecksum(t *testing.T) {
require.NoError(t, os.WriteFile(checksumPath, []byte(checksumContent), 0644))

err := verifyChecksum(checksumPath)
assert.ErrorContains(t, err, "does not match expected checksum")
contains := fmt.Sprintf("%s checksum mismatch: expected=%s", fileName, hashHex)
assert.ErrorContains(t, err, contains)
})
}
31 changes: 14 additions & 17 deletions dev-tools/mage/pkgtypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"archive/zip"
"bytes"
"compress/gzip"
"errors"
"fmt"
"hash/fnv"
"io"
Expand Down Expand Up @@ -666,27 +667,15 @@ func PackageTarGz(spec PackageSpec) error {
if err != nil {
return err
}
defer func() {
if err := outFile.Close(); err != nil {
log.Printf("failed to close output file: %v", err)
}
}()
defer closeOrLog(outFile, "output file")

// Create a gzip writer to our output file
gzWriter := gzip.NewWriter(outFile)
defer func() {
if err := gzWriter.Close(); err != nil {
log.Printf("failed to close gzip writer: %v", err)
}
}()
defer closeOrLog(gzWriter, "gzip writer")

// Create a new tar archive.
w := tar.NewWriter(gzWriter)
defer func() {
if err := w.Close(); err != nil {
log.Printf("failed to close tar writer: %v", err)
}
}()
defer closeOrLog(gzWriter, "tar writer")

// // Replace the darwin-universal by darwin-x86_64 and darwin-arm64. Also
// // keep the other files.
Expand Down Expand Up @@ -759,6 +748,14 @@ func PackageTarGz(spec PackageSpec) error {
return nil
}

func closeOrLog(closer io.Closer, what string) {
err := closer.Close()
if err == nil || errors.Is(err, os.ErrClosed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in behavior: you are explicitly ignoring os.ErrClosed. Wouldn't it be better to figure out where we are closing the same io.Closer twice ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be done as well but I figured cleanliness is slightly preferred over having to set nil to the file pointers, at least for this part of the code. Other parts don't even log the Close errors.

return
}
log.Printf("failed to close %s: %v", what, err)
}

// PackageDeb packages a deb file. This requires Docker to execute FPM.
func PackageDeb(spec PackageSpec) error {
return runFPM(spec, Deb)
Expand Down Expand Up @@ -953,7 +950,7 @@ func addFileToZip(ar *zip.Writer, baseDir string, pkgFile PackageFile) error {
if err != nil {
return err
}
defer file.Close()
defer closeOrLog(file, "zip input file")

if _, err = io.Copy(w, file); err != nil {
return err
Expand Down Expand Up @@ -1034,7 +1031,7 @@ func addFileToTar(ar *tar.Writer, baseDir string, pkgFile PackageFile) error {
if err != nil {
return err
}
defer file.Close()
defer closeOrLog(file, "tar input file")

if _, err = io.Copy(ar, file); err != nil {
return err
Expand Down