From f7ed7c58bc701cf2c8f76917f200f628a41c3861 Mon Sep 17 00:00:00 2001 From: Martin Wimpress Date: Thu, 12 Mar 2026 23:39:36 +0000 Subject: [PATCH] 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 --- internal/builder/buildsystems.go | 9 ++----- internal/builder/download.go | 3 ++- internal/builder/libraries.go | 42 +++++++++----------------------- internal/builder/library.go | 21 ++++++++++------ internal/builder/pointer.go | 6 ----- internal/builder/state.go | 19 +++++---------- 6 files changed, 34 insertions(+), 66 deletions(-) diff --git a/internal/builder/buildsystems.go b/internal/builder/buildsystems.go index 2f663ff..2a2ac8f 100644 --- a/internal/builder/buildsystems.go +++ b/internal/builder/buildsystems.go @@ -6,7 +6,6 @@ import ( "os" "path/filepath" "runtime" - "strings" ) // 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 // Extract just the -isysroot part from CGO_CFLAGS // CGO_CFLAGS = "-isysroot /path/to/sdk -I/path/to/clang/include" // We only want the -isysroot portion for CPPFLAGS - parts := strings.Fields(cgoCflags) - for i, part := range parts { - if part == "-isysroot" && i+1 < len(parts) { - cppflags = fmt.Sprintf("%s -isysroot %s", cppflags, parts[i+1]) - break - } + if sdkPath := extractSDKPath(cgoCflags); sdkPath != "" { + cppflags = fmt.Sprintf("%s -isysroot %s", cppflags, sdkPath) } } diff --git a/internal/builder/download.go b/internal/builder/download.go index 48e202d..f8f9800 100644 --- a/internal/builder/download.go +++ b/internal/builder/download.go @@ -54,7 +54,6 @@ func DownloadFile(url, dest string, logger io.Writer) error { // Monitor progress - update every 2 seconds to avoid log spam ticker := time.NewTicker(2 * time.Second) - defer ticker.Stop() lastProgress := 0.0 for !resp.IsComplete() { @@ -70,6 +69,8 @@ func DownloadFile(url, dest string, logger io.Writer) error { } } + ticker.Stop() + // Check for errors if err := resp.Err(); err != nil { lastErr = err diff --git a/internal/builder/libraries.go b/internal/builder/libraries.go index 8fd39bf..6118099 100644 --- a/internal/builder/libraries.go +++ b/internal/builder/libraries.go @@ -94,15 +94,15 @@ func buildLibraryOrder() []*Library { } var result []*Library - var ffmpegLib *Library + ffmpegSeen := false for len(queue) > 0 { // Pop from queue current := queue[0] queue = queue[1:] // Hold FFmpeg aside to add at the end - if current.Name == "ffmpeg" { - ffmpegLib = current + if current == ffmpeg { + ffmpegSeen = true } else { result = append(result, current) } @@ -126,8 +126,8 @@ func buildLibraryOrder() []*Library { for _, lib := range result { processed[lib] = true } - if ffmpegLib != nil { - processed[ffmpegLib] = true + if ffmpegSeen { + processed[ffmpeg] = true } fmt.Fprintf(os.Stderr, "\nLibraries stuck in cycle:\n") @@ -152,8 +152,8 @@ func buildLibraryOrder() []*Library { } // Always add FFmpeg last as it depends on all other libraries - if ffmpegLib != nil { - result = append(result, ffmpegLib) + if ffmpegSeen { + result = append(result, ffmpeg) } return result @@ -616,15 +616,7 @@ var rav1e = &Library{ // On macOS, add SDK library path for any native dependencies if runtime.GOOS == "darwin" { cgoCflags := os.Getenv("CGO_CFLAGS") - // Extract SDK path from CGO_CFLAGS (-isysroot ) - var sdkPath string - parts := strings.Fields(cgoCflags) - for i, p := range parts { - if p == "-isysroot" && i+1 < len(parts) { - sdkPath = parts[i+1] - break - } - } + sdkPath := extractSDKPath(cgoCflags) if sdkPath != "" { sdkLibPath := filepath.Join(sdkPath, "usr", "lib") @@ -840,28 +832,16 @@ var ffmpeg = &Library{ // CollectFFmpegEnables collects --enable-* flags from all enabled external libraries // This must be called AFTER AllLibraries is initialized to inject the enables into ffmpeg's ConfigureArgs func CollectFFmpegEnables() { - // Find the ffmpeg library - var ffmpegLib *Library - for _, lib := range AllLibraries { - if lib.Name == "ffmpeg" { - ffmpegLib = lib - break - } - } - if ffmpegLib == nil { - return - } - // Wrap the original ConfigureArgs function - originalConfigureArgs := ffmpegLib.ConfigureArgs - ffmpegLib.ConfigureArgs = func(os string) []string { + originalConfigureArgs := ffmpeg.ConfigureArgs + ffmpeg.ConfigureArgs = func(os string) []string { // Get base args from original function args := originalConfigureArgs(os) // Collect and add --enable-* flags from all enabled external libraries for _, lib := range AllLibraries { // Skip ffmpeg itself and libraries that are disabled or shouldn't build on current platform - if lib.Name == "ffmpeg" || !lib.ShouldBuild() { + if lib == ffmpeg || !lib.ShouldBuild() { continue } // Add all FFmpeg enable flags for this library diff --git a/internal/builder/library.go b/internal/builder/library.go index 1ae9148..41cccb1 100644 --- a/internal/builder/library.go +++ b/internal/builder/library.go @@ -12,6 +12,18 @@ import ( "strings" ) +// extractSDKPath returns the path following -isysroot in a flags string, +// or empty string if not found. +func extractSDKPath(flags string) string { + parts := strings.Fields(flags) + for i, p := range parts { + if p == "-isysroot" && i+1 < len(parts) { + return parts[i+1] + } + } + return "" +} + // Library represents a third-party library to build type Library struct { Name string @@ -273,14 +285,7 @@ func buildEnv(installDir string) []string { // Extract SDK path from CGO_CFLAGS (-isysroot ) for LDFLAGS // This ensures cargo/rustc can find SDK libraries like libiconv - var sdkPath string - parts := strings.Fields(cgoCflags) - for i, p := range parts { - if p == "-isysroot" && i+1 < len(parts) { - sdkPath = parts[i+1] - break - } - } + sdkPath := extractSDKPath(cgoCflags) if sdkPath != "" { ldExtra := "-L" + filepath.Join(sdkPath, "usr", "lib") updatedLdflags := false diff --git a/internal/builder/pointer.go b/internal/builder/pointer.go index c6efeeb..15806c1 100644 --- a/internal/builder/pointer.go +++ b/internal/builder/pointer.go @@ -11,9 +11,3 @@ func Ptr[T any](v T) *T { func Disabled() *bool { return Ptr(false) } - -// Enabled is a convenience helper that returns a pointer to true -// Usage: Enabled: Enabled() -func Enabled() *bool { - return Ptr(true) -} diff --git a/internal/builder/state.go b/internal/builder/state.go index e106dd0..0493987 100644 --- a/internal/builder/state.go +++ b/internal/builder/state.go @@ -88,20 +88,13 @@ func (s *BuildState) CanSkip(installDir string) bool { } // For libraries with LinkLibs, check that all expected .a files exist - for _, dir := range []string{"lib"} { - libDir := filepath.Join(installDir, dir) - allFound := true - for _, libName := range s.lib.LinkLibs { - libPath := filepath.Join(libDir, libName+".a") - if !fileExists(libPath) { - allFound = false - break - } - } - if allFound { - return true + libDir := filepath.Join(installDir, "lib") + for _, libName := range s.lib.LinkLibs { + libPath := filepath.Join(libDir, libName+".a") + if !fileExists(libPath) { + return false } } - return false + return true }