Skip to content

Commit 5f355e3

Browse files
committed
refactor(builder): eliminate code smells in builder module
- Remove dead Enabled() helper function from pointer.go - Fix resource leak: move ticker.Stop() outside retry loop in download.go - Flatten single-element slice loop in state.go - Replace magic string sentinels with pointer identity checks in libraries.go - Extract duplicated SDK path extraction logic into extractSDKPath() helper Signed-off-by: Martin Wimpress <code@wimpress.io>
1 parent 6bdd1fb commit 5f355e3

6 files changed

Lines changed: 34 additions & 66 deletions

File tree

internal/builder/buildsystems.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"os"
77
"path/filepath"
88
"runtime"
9-
"strings"
109
)
1110

1211
// stagingDir derives the staging/install directory from a build directory path.
@@ -85,12 +84,8 @@ func (a *AutoconfBuild) Configure(lib *Library, srcPath, buildDir, installDir st
8584
// Extract just the -isysroot part from CGO_CFLAGS
8685
// CGO_CFLAGS = "-isysroot /path/to/sdk -I/path/to/clang/include"
8786
// We only want the -isysroot portion for CPPFLAGS
88-
parts := strings.Fields(cgoCflags)
89-
for i, part := range parts {
90-
if part == "-isysroot" && i+1 < len(parts) {
91-
cppflags = fmt.Sprintf("%s -isysroot %s", cppflags, parts[i+1])
92-
break
93-
}
87+
if sdkPath := extractSDKPath(cgoCflags); sdkPath != "" {
88+
cppflags = fmt.Sprintf("%s -isysroot %s", cppflags, sdkPath)
9489
}
9590
}
9691

internal/builder/download.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ func DownloadFile(url, dest string, logger io.Writer) error {
5454

5555
// Monitor progress - update every 2 seconds to avoid log spam
5656
ticker := time.NewTicker(2 * time.Second)
57-
defer ticker.Stop()
5857

5958
lastProgress := 0.0
6059
for !resp.IsComplete() {
@@ -70,6 +69,8 @@ func DownloadFile(url, dest string, logger io.Writer) error {
7069
}
7170
}
7271

72+
ticker.Stop()
73+
7374
// Check for errors
7475
if err := resp.Err(); err != nil {
7576
lastErr = err

internal/builder/libraries.go

Lines changed: 11 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -94,15 +94,15 @@ func buildLibraryOrder() []*Library {
9494
}
9595

9696
var result []*Library
97-
var ffmpegLib *Library
97+
ffmpegSeen := false
9898
for len(queue) > 0 {
9999
// Pop from queue
100100
current := queue[0]
101101
queue = queue[1:]
102102

103103
// Hold FFmpeg aside to add at the end
104-
if current.Name == "ffmpeg" {
105-
ffmpegLib = current
104+
if current == ffmpeg {
105+
ffmpegSeen = true
106106
} else {
107107
result = append(result, current)
108108
}
@@ -126,8 +126,8 @@ func buildLibraryOrder() []*Library {
126126
for _, lib := range result {
127127
processed[lib] = true
128128
}
129-
if ffmpegLib != nil {
130-
processed[ffmpegLib] = true
129+
if ffmpegSeen {
130+
processed[ffmpeg] = true
131131
}
132132

133133
fmt.Fprintf(os.Stderr, "\nLibraries stuck in cycle:\n")
@@ -152,8 +152,8 @@ func buildLibraryOrder() []*Library {
152152
}
153153

154154
// Always add FFmpeg last as it depends on all other libraries
155-
if ffmpegLib != nil {
156-
result = append(result, ffmpegLib)
155+
if ffmpegSeen {
156+
result = append(result, ffmpeg)
157157
}
158158

159159
return result
@@ -616,15 +616,7 @@ var rav1e = &Library{
616616
// On macOS, add SDK library path for any native dependencies
617617
if runtime.GOOS == "darwin" {
618618
cgoCflags := os.Getenv("CGO_CFLAGS")
619-
// Extract SDK path from CGO_CFLAGS (-isysroot <path>)
620-
var sdkPath string
621-
parts := strings.Fields(cgoCflags)
622-
for i, p := range parts {
623-
if p == "-isysroot" && i+1 < len(parts) {
624-
sdkPath = parts[i+1]
625-
break
626-
}
627-
}
619+
sdkPath := extractSDKPath(cgoCflags)
628620

629621
if sdkPath != "" {
630622
sdkLibPath := filepath.Join(sdkPath, "usr", "lib")
@@ -840,28 +832,16 @@ var ffmpeg = &Library{
840832
// CollectFFmpegEnables collects --enable-* flags from all enabled external libraries
841833
// This must be called AFTER AllLibraries is initialized to inject the enables into ffmpeg's ConfigureArgs
842834
func CollectFFmpegEnables() {
843-
// Find the ffmpeg library
844-
var ffmpegLib *Library
845-
for _, lib := range AllLibraries {
846-
if lib.Name == "ffmpeg" {
847-
ffmpegLib = lib
848-
break
849-
}
850-
}
851-
if ffmpegLib == nil {
852-
return
853-
}
854-
855835
// Wrap the original ConfigureArgs function
856-
originalConfigureArgs := ffmpegLib.ConfigureArgs
857-
ffmpegLib.ConfigureArgs = func(os string) []string {
836+
originalConfigureArgs := ffmpeg.ConfigureArgs
837+
ffmpeg.ConfigureArgs = func(os string) []string {
858838
// Get base args from original function
859839
args := originalConfigureArgs(os)
860840

861841
// Collect and add --enable-* flags from all enabled external libraries
862842
for _, lib := range AllLibraries {
863843
// Skip ffmpeg itself and libraries that are disabled or shouldn't build on current platform
864-
if lib.Name == "ffmpeg" || !lib.ShouldBuild() {
844+
if lib == ffmpeg || !lib.ShouldBuild() {
865845
continue
866846
}
867847
// Add all FFmpeg enable flags for this library

internal/builder/library.go

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,18 @@ import (
1212
"strings"
1313
)
1414

15+
// extractSDKPath returns the path following -isysroot in a flags string,
16+
// or empty string if not found.
17+
func extractSDKPath(flags string) string {
18+
parts := strings.Fields(flags)
19+
for i, p := range parts {
20+
if p == "-isysroot" && i+1 < len(parts) {
21+
return parts[i+1]
22+
}
23+
}
24+
return ""
25+
}
26+
1527
// Library represents a third-party library to build
1628
type Library struct {
1729
Name string
@@ -273,14 +285,7 @@ func buildEnv(installDir string) []string {
273285

274286
// Extract SDK path from CGO_CFLAGS (-isysroot <path>) for LDFLAGS
275287
// This ensures cargo/rustc can find SDK libraries like libiconv
276-
var sdkPath string
277-
parts := strings.Fields(cgoCflags)
278-
for i, p := range parts {
279-
if p == "-isysroot" && i+1 < len(parts) {
280-
sdkPath = parts[i+1]
281-
break
282-
}
283-
}
288+
sdkPath := extractSDKPath(cgoCflags)
284289
if sdkPath != "" {
285290
ldExtra := "-L" + filepath.Join(sdkPath, "usr", "lib")
286291
updatedLdflags := false

internal/builder/pointer.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,3 @@ func Ptr[T any](v T) *T {
1111
func Disabled() *bool {
1212
return Ptr(false)
1313
}
14-
15-
// Enabled is a convenience helper that returns a pointer to true
16-
// Usage: Enabled: Enabled()
17-
func Enabled() *bool {
18-
return Ptr(true)
19-
}

internal/builder/state.go

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -88,20 +88,13 @@ func (s *BuildState) CanSkip(installDir string) bool {
8888
}
8989

9090
// For libraries with LinkLibs, check that all expected .a files exist
91-
for _, dir := range []string{"lib"} {
92-
libDir := filepath.Join(installDir, dir)
93-
allFound := true
94-
for _, libName := range s.lib.LinkLibs {
95-
libPath := filepath.Join(libDir, libName+".a")
96-
if !fileExists(libPath) {
97-
allFound = false
98-
break
99-
}
100-
}
101-
if allFound {
102-
return true
91+
libDir := filepath.Join(installDir, "lib")
92+
for _, libName := range s.lib.LinkLibs {
93+
libPath := filepath.Join(libDir, libName+".a")
94+
if !fileExists(libPath) {
95+
return false
10396
}
10497
}
10598

106-
return false
99+
return true
107100
}

0 commit comments

Comments
 (0)