legacy: Make a lot of Builder internals private (#2325)

* Removed logger as a field of BuildOptionManager

* Made BuildOptionsManager a field of Builder

* Renamed BuildOptionsManager -> BuildOptions

* Made WipeBuildPath a method of Builder

* Removed unused currentBuildOptionsJSON field

* Separated wipeBuildPath and createBuildOptionsJSON actions

* Made wipe() a proper method -> Buidler.wipeBuildPath()

* Made BuildOptions private

* Made SketchLibrariesDetector a private field of Buidler

* Made BuildArtifacts private

* Made all builder subpackages internal

* Moved initialization of private structs inside NewBuilder

Author: cmaglie

arduino/builder/build_options_manager.go

@@ -20,19 +20,17 @@ import (
 	"path/filepath"
 	"strings"
 
-	"github.com/arduino/arduino-cli/arduino/builder/logger"
-	"github.com/arduino/arduino-cli/arduino/builder/utils"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/utils"
 	"github.com/arduino/arduino-cli/arduino/cores"
 	"github.com/arduino/arduino-cli/arduino/sketch"
 	"github.com/arduino/go-paths-helper"
 	properties "github.com/arduino/go-properties-orderedmap"
 	"github.com/pkg/errors"
 )
 
-// BuildOptionsManager fixdoc
-type BuildOptionsManager struct {
-	currentOptions          *properties.Map
-	currentBuildOptionsJSON []byte
+// buildOptions fixdoc
+type buildOptions struct {
+	currentOptions *properties.Map
 
 	hardwareDirs              paths.PathList
 	builtInToolsDirs          paths.PathList
@@ -45,11 +43,10 @@ type BuildOptionsManager struct {
 	customBuildProperties     []string
 	compilerOptimizationFlags string
 	clean                     bool
-	builderLogger             *logger.BuilderLogger
 }
 
-// NewBuildOptionsManager fixdoc
-func NewBuildOptionsManager(
+// newBuildOptions fixdoc
+func newBuildOptions(
 	hardwareDirs, builtInToolsDirs, otherLibrariesDirs paths.PathList,
 	builtInLibrariesDirs, buildPath *paths.Path,
 	sketch *sketch.Sketch,
@@ -58,8 +55,7 @@ func NewBuildOptionsManager(
 	clean bool,
 	compilerOptimizationFlags string,
 	runtimePlatformPath, buildCorePath *paths.Path,
-	buildLogger *logger.BuilderLogger,
-) *BuildOptionsManager {
+) *buildOptions {
 	opts := properties.NewMap()
 
 	opts.Set("hardwareFolders", strings.Join(hardwareDirs.AsStrings(), ","))
@@ -85,7 +81,7 @@ func NewBuildOptionsManager(
 	}
 	opts.Set("additionalFiles", strings.Join(additionalFilesRelative, ","))
 
-	return &BuildOptionsManager{
+	return &buildOptions{
 		currentOptions:            opts,
 		hardwareDirs:              hardwareDirs,
 		builtInToolsDirs:          builtInToolsDirs,
@@ -98,46 +94,39 @@ func NewBuildOptionsManager(
 		customBuildProperties:     customBuildProperties,
 		compilerOptimizationFlags: compilerOptimizationFlags,
 		clean:                     clean,
-		builderLogger:             buildLogger,
 	}
 }
 
-// WipeBuildPath fixdoc
-func (m *BuildOptionsManager) WipeBuildPath() error {
-	buildOptionsJSON, err := json.MarshalIndent(m.currentOptions, "", "  ")
+func (b *Builder) createBuildOptionsJSON() error {
+	buildOptionsJSON, err := json.MarshalIndent(b.buildOptions.currentOptions, "", "  ")
 	if err != nil {
 		return errors.WithStack(err)
 	}
-	m.currentBuildOptionsJSON = buildOptionsJSON
-
-	if err := m.wipeBuildPath(); err != nil {
-		return errors.WithStack(err)
-	}
-	return m.buildPath.Join("build.options.json").WriteFile(buildOptionsJSON)
+	return b.buildOptions.buildPath.Join("build.options.json").WriteFile(buildOptionsJSON)
 }
 
-func (m *BuildOptionsManager) wipeBuildPath() error {
-	wipe := func() error {
-		// FIXME: this should go outside legacy and behind a `logrus` call so users can
-		// control when this should be printed.
-		// logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_BUILD_OPTIONS_CHANGED + constants.MSG_REBUILD_ALL)
-		if err := m.buildPath.RemoveAll(); err != nil {
-			return errors.WithMessage(err, tr("cleaning build path"))
-		}
-		if err := m.buildPath.MkdirAll(); err != nil {
-			return errors.WithMessage(err, tr("cleaning build path"))
-		}
-		return nil
+func (b *Builder) wipeBuildPath() error {
+	// FIXME: this should go outside legacy and behind a `logrus` call so users can
+	// control when this should be printed.
+	// logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_BUILD_OPTIONS_CHANGED + constants.MSG_REBUILD_ALL)
+	if err := b.buildOptions.buildPath.RemoveAll(); err != nil {
+		return errors.WithMessage(err, tr("cleaning build path"))
 	}
+	if err := b.buildOptions.buildPath.MkdirAll(); err != nil {
+		return errors.WithMessage(err, tr("cleaning build path"))
+	}
+	return nil
+}
 
-	if m.clean {
-		return wipe()
+func (b *Builder) wipeBuildPathIfBuildOptionsChanged() error {
+	if b.buildOptions.clean {
+		return b.wipeBuildPath()
 	}
 
 	// Load previous build options map
 	var buildOptionsJSONPrevious []byte
 	var _err error
-	if buildOptionsFile := m.buildPath.Join("build.options.json"); buildOptionsFile.Exist() {
+	if buildOptionsFile := b.buildOptions.buildPath.Join("build.options.json"); buildOptionsFile.Exist() {
 		buildOptionsJSONPrevious, _err = buildOptionsFile.ReadFile()
 		if _err != nil {
 			return errors.WithStack(_err)
@@ -150,12 +139,12 @@ func (m *BuildOptionsManager) wipeBuildPath() error {
 
 	var prevOpts *properties.Map
 	if err := json.Unmarshal(buildOptionsJSONPrevious, &prevOpts); err != nil || prevOpts == nil {
-		m.builderLogger.Info(tr("%[1]s invalid, rebuilding all", "build.options.json"))
-		return wipe()
+		b.logger.Info(tr("%[1]s invalid, rebuilding all", "build.options.json"))
+		return b.wipeBuildPath()
 	}
 
 	// Since we might apply a side effect we clone it
-	currentOptions := m.currentOptions.Clone()
+	currentOptions := b.buildOptions.currentOptions.Clone()
 	// If SketchLocation path is different but filename is the same, consider it equal
 	if filepath.Base(currentOptions.Get("sketchLocation")) == filepath.Base(prevOpts.Get("sketchLocation")) {
 		currentOptions.Remove("sketchLocation")
@@ -167,10 +156,10 @@ func (m *BuildOptionsManager) wipeBuildPath() error {
 		// check if any of the files contained in the core folders has changed
 		// since the json was generated - like platform.txt or similar
 		// if so, trigger a "safety" wipe
-		targetCoreFolder := m.runtimePlatformPath
-		coreFolder := m.buildCorePath
+		targetCoreFolder := b.buildOptions.runtimePlatformPath
+		coreFolder := b.buildOptions.buildCorePath
 		realCoreFolder := coreFolder.Parent().Parent()
-		jsonPath := m.buildPath.Join("build.options.json")
+		jsonPath := b.buildOptions.buildPath.Join("build.options.json")
 		coreUnchanged, _ := utils.DirContentIsOlderThan(realCoreFolder, jsonPath, ".txt")
 		if coreUnchanged && targetCoreFolder != nil && !realCoreFolder.EqualsTo(targetCoreFolder) {
 			coreUnchanged, _ = utils.DirContentIsOlderThan(targetCoreFolder, jsonPath, ".txt")
@@ -180,5 +169,5 @@ func (m *BuildOptionsManager) wipeBuildPath() error {
 		}
 	}
 
-	return wipe()
+	return b.wipeBuildPath()
 }

arduino/builder/builder.go

@@ -18,14 +18,17 @@ package builder
 import (
 	"errors"
 	"fmt"
+	"io"
 
-	"github.com/arduino/arduino-cli/arduino/builder/compilation"
-	"github.com/arduino/arduino-cli/arduino/builder/detector"
-	"github.com/arduino/arduino-cli/arduino/builder/logger"
-	"github.com/arduino/arduino-cli/arduino/builder/progress"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/compilation"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/detector"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/logger"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/progress"
 	"github.com/arduino/arduino-cli/arduino/cores"
+	"github.com/arduino/arduino-cli/arduino/libraries"
 	"github.com/arduino/arduino-cli/arduino/libraries/librariesmanager"
 	"github.com/arduino/arduino-cli/arduino/sketch"
+	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/arduino/go-paths-helper"
 	"github.com/arduino/go-properties-orderedmap"
 )
@@ -77,14 +80,15 @@ type Builder struct {
 	targetPlatform *cores.PlatformRelease
 	actualPlatform *cores.PlatformRelease
 
-	buildArtifacts *BuildArtifacts
+	buildArtifacts *buildArtifacts
 
-	*detector.SketchLibrariesDetector
-	*BuildOptionsManager
+	buildOptions *buildOptions
+
+	libsDetector *detector.SketchLibrariesDetector
 }
 
-// BuildArtifacts contains the result of various build
-type BuildArtifacts struct {
+// buildArtifacts contains the result of various build
+type buildArtifacts struct {
 	// populated by BuildCore
 	coreArchiveFilePath *paths.Path
 	coreObjectsFiles    paths.PathList
@@ -115,8 +119,8 @@ func NewBuilder(
 	useCachedLibrariesResolution bool,
 	librariesManager *librariesmanager.LibrariesManager,
 	libraryDirs paths.PathList,
-	logger *logger.BuilderLogger,
-	progressStats *progress.Struct,
+	stdout, stderr io.Writer, verbose bool, warningsLevel string,
+	progresCB rpc.TaskProgressCB,
 ) (*Builder, error) {
 	buildProperties := properties.NewMap()
 	if boardBuildProperties != nil {
@@ -165,10 +169,7 @@ func NewBuilder(
 		return nil, ErrSketchCannotBeLocatedInBuildPath
 	}
 
-	if progressStats == nil {
-		progressStats = progress.New(nil)
-	}
-
+	logger := logger.New(stdout, stderr, verbose, warningsLevel)
 	libsManager, libsResolver, verboseOut, err := detector.LibrariesLoader(
 		useCachedLibrariesResolution, librariesManager,
 		builtInLibrariesDirs, libraryDirs, otherLibrariesDirs,
@@ -196,18 +197,18 @@ func NewBuilder(
 		sourceOverrides:               sourceOverrides,
 		onlyUpdateCompilationDatabase: onlyUpdateCompilationDatabase,
 		compilationDatabase:           compilation.NewDatabase(buildPath.Join("compile_commands.json")),
-		Progress:                      progressStats,
+		Progress:                      progress.New(progresCB),
 		executableSectionsSize:        []ExecutableSectionSize{},
-		buildArtifacts:                &BuildArtifacts{},
+		buildArtifacts:                &buildArtifacts{},
 		targetPlatform:                targetPlatform,
 		actualPlatform:                actualPlatform,
-		SketchLibrariesDetector: detector.NewSketchLibrariesDetector(
+		libsDetector: detector.NewSketchLibrariesDetector(
 			libsManager, libsResolver,
 			useCachedLibrariesResolution,
 			onlyUpdateCompilationDatabase,
 			logger,
 		),
-		BuildOptionsManager: NewBuildOptionsManager(
+		buildOptions: newBuildOptions(
 			hardwareDirs, builtInToolsDirs, otherLibrariesDirs,
 			builtInLibrariesDirs, buildPath,
 			sk,
@@ -217,7 +218,6 @@ func NewBuilder(
 			buildProperties.Get("compiler.optimization_flags"),
 			buildProperties.GetPath("runtime.platform.path"),
 			buildProperties.GetPath("build.core.path"), // TODO can we buildCorePath ?
-			logger,
 		),
 	}, nil
 }
@@ -237,6 +237,11 @@ func (b *Builder) ExecutableSectionsSize() ExecutablesFileSections {
 	return b.executableSectionsSize
 }
 
+// ImportedLibraries fixdoc
+func (b *Builder) ImportedLibraries() libraries.List {
+	return b.libsDetector.ImportedLibraries()
+}
+
 // Preprocess fixdoc
 func (b *Builder) Preprocess() error {
 	b.Progress.AddSubSteps(6)
@@ -249,7 +254,10 @@ func (b *Builder) preprocess() error {
 		return err
 	}
 
-	if err := b.BuildOptionsManager.WipeBuildPath(); err != nil {
+	if err := b.wipeBuildPathIfBuildOptionsChanged(); err != nil {
+		return err
+	}
+	if err := b.createBuildOptionsJSON(); err != nil {
 		return err
 	}
 	b.Progress.CompleteStep()
@@ -268,7 +276,7 @@ func (b *Builder) preprocess() error {
 	b.Progress.PushProgress()
 
 	b.logIfVerbose(false, tr("Detecting libraries used..."))
-	err := b.SketchLibrariesDetector.FindIncludes(
+	err := b.libsDetector.FindIncludes(
 		b.buildPath,
 		b.buildProperties.GetPath("build.core.path"),
 		b.buildProperties.GetPath("build.variant.path"),
@@ -284,12 +292,12 @@ func (b *Builder) preprocess() error {
 	b.Progress.CompleteStep()
 	b.Progress.PushProgress()
 
-	b.warnAboutArchIncompatibleLibraries(b.SketchLibrariesDetector.ImportedLibraries())
+	b.warnAboutArchIncompatibleLibraries(b.libsDetector.ImportedLibraries())
 	b.Progress.CompleteStep()
 	b.Progress.PushProgress()
 
 	b.logIfVerbose(false, tr("Generating function prototypes..."))
-	if err := b.preprocessSketch(b.SketchLibrariesDetector.IncludeFolders()); err != nil {
+	if err := b.preprocessSketch(b.libsDetector.IncludeFolders()); err != nil {
 		return err
 	}
 	b.Progress.CompleteStep()
@@ -327,18 +335,18 @@ func (b *Builder) Build() error {
 
 	buildErr := b.build()
 
-	b.SketchLibrariesDetector.PrintUsedAndNotUsedLibraries(buildErr != nil)
+	b.libsDetector.PrintUsedAndNotUsedLibraries(buildErr != nil)
 	b.Progress.CompleteStep()
 	b.Progress.PushProgress()
 
-	b.printUsedLibraries(b.SketchLibrariesDetector.ImportedLibraries())
+	b.printUsedLibraries(b.libsDetector.ImportedLibraries())
 	b.Progress.CompleteStep()
 	b.Progress.PushProgress()
 
 	if buildErr != nil {
 		return buildErr
 	}
-	if err := b.exportProjectCMake(b.SketchLibrariesDetector.ImportedLibraries(), b.SketchLibrariesDetector.IncludeFolders()); err != nil {
+	if err := b.exportProjectCMake(b.libsDetector.ImportedLibraries(), b.libsDetector.IncludeFolders()); err != nil {
 		return err
 	}
 	b.Progress.CompleteStep()
@@ -362,7 +370,7 @@ func (b *Builder) build() error {
 	b.Progress.CompleteStep()
 	b.Progress.PushProgress()
 
-	if err := b.BuildSketch(b.SketchLibrariesDetector.IncludeFolders()); err != nil {
+	if err := b.BuildSketch(b.libsDetector.IncludeFolders()); err != nil {
 		return err
 	}
 	b.Progress.CompleteStep()
@@ -381,13 +389,13 @@ func (b *Builder) build() error {
 	b.Progress.CompleteStep()
 	b.Progress.PushProgress()
 
-	if err := b.removeUnusedCompiledLibraries(b.SketchLibrariesDetector.ImportedLibraries()); err != nil {
+	if err := b.removeUnusedCompiledLibraries(b.libsDetector.ImportedLibraries()); err != nil {
 		return err
 	}
 	b.Progress.CompleteStep()
 	b.Progress.PushProgress()
 
-	if err := b.buildLibraries(b.SketchLibrariesDetector.IncludeFolders(), b.SketchLibrariesDetector.ImportedLibraries()); err != nil {
+	if err := b.buildLibraries(b.libsDetector.IncludeFolders(), b.libsDetector.ImportedLibraries()); err != nil {
 		return err
 	}
 	b.Progress.CompleteStep()

arduino/builder/core.go

@@ -23,7 +23,7 @@ import (
 	"strings"
 
 	"github.com/arduino/arduino-cli/arduino/builder/cpp"
-	"github.com/arduino/arduino-cli/arduino/builder/utils"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/utils"
 	"github.com/arduino/arduino-cli/buildcache"
 	f "github.com/arduino/arduino-cli/internal/algorithms"
 	"github.com/arduino/go-paths-helper"

arduino/builder/export_cmake.go

@@ -24,12 +24,11 @@ import (
 	"slices"
 	"strings"
 
-	"github.com/arduino/go-paths-helper"
-	properties "github.com/arduino/go-properties-orderedmap"
-
-	"github.com/arduino/arduino-cli/arduino/builder/utils"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/utils"
 	"github.com/arduino/arduino-cli/arduino/globals"
 	"github.com/arduino/arduino-cli/arduino/libraries"
+	"github.com/arduino/go-paths-helper"
+	properties "github.com/arduino/go-properties-orderedmap"
 )
 
 var lineMatcher = regexp.MustCompile(`^#line\s\d+\s"`)

arduino/builder/compilation/database.go renamed to arduino/builder/internal/compilation/database.go

@@ -25,9 +25,9 @@ import (
 	"strings"
 	"time"
 
-	"github.com/arduino/arduino-cli/arduino/builder/logger"
-	"github.com/arduino/arduino-cli/arduino/builder/preprocessor"
-	"github.com/arduino/arduino-cli/arduino/builder/utils"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/logger"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/preprocessor"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/utils"
 	"github.com/arduino/arduino-cli/arduino/cores"
 	"github.com/arduino/arduino-cli/arduino/globals"
 	"github.com/arduino/arduino-cli/arduino/libraries"

arduino/builder/compilation/database_test.go renamed to arduino/builder/internal/compilation/database_test.go

@@ -18,7 +18,7 @@ package detector_test
 import (
 	"testing"
 
-	"github.com/arduino/arduino-cli/arduino/builder/detector"
+	"github.com/arduino/arduino-cli/arduino/builder/internal/detector"
 	"github.com/stretchr/testify/require"
 )