From 4bf18d613612e0d5eec7a42c91f47fee6551ff5d Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Fri, 25 Aug 2023 08:39:49 +0200 Subject: [PATCH] [skip-changelog] Some refactorings on library resolution code (#1766) * Transformed FailIfImportedLibraryIsWrong into a functon There was no need to have it encapsulated in a Command * Move variable near the place it belongs * Dramatically simplified SourceFile object Removed dependency from types.Context * Added comments about utility folder role in library discovery * Simplified if construct * Made RelativePath private * Skip includes detection of precompiled libraries early Instead of skipping include detection later, avoid to add the sources in the queue right from the beginning. * Keep extra-include dirs due to "utility" folder in SourceFile object Also remove the reference to the original Library object because it's no more needed. --- legacy/builder/constants/constants.go | 1 - legacy/builder/container_find_includes.go | 84 +++++++++++-------- .../fail_if_imported_library_is_wrong.go | 52 ------------ legacy/builder/types/types.go | 82 +++++++++--------- 4 files changed, 93 insertions(+), 126 deletions(-) delete mode 100644 legacy/builder/fail_if_imported_library_is_wrong.go diff --git a/legacy/builder/constants/constants.go b/legacy/builder/constants/constants.go index a91f454ffcf..19a828584e3 100644 --- a/legacy/builder/constants/constants.go +++ b/legacy/builder/constants/constants.go @@ -39,7 +39,6 @@ const FOLDER_TOOLS = "tools" const FOLDER_LIBRARIES = "libraries" const LIBRARY_ALL_ARCHS = "*" const LIBRARY_EMAIL = "email" -const LIBRARY_FOLDER_ARCH = "arch" const LIBRARY_FOLDER_SRC = "src" const LOG_LEVEL_DEBUG = "debug" const LOG_LEVEL_ERROR = "error" diff --git a/legacy/builder/container_find_includes.go b/legacy/builder/container_find_includes.go index 69bf06dc9ac..4c78e18ce18 100644 --- a/legacy/builder/container_find_includes.go +++ b/legacy/builder/container_find_includes.go @@ -175,7 +175,7 @@ func (s *ContainerFindIncludes) findIncludes(ctx *types.Context) error { } } - if err := runCommand(ctx, &FailIfImportedLibraryIsWrong{}); err != nil { + if err := failIfImportedLibraryIsWrong(ctx); err != nil { return errors.WithStack(err) } @@ -198,15 +198,6 @@ func appendIncludeFolder(ctx *types.Context, cache *includeCache, sourceFilePath cache.ExpectEntry(sourceFilePath, include, folder) } -func runCommand(ctx *types.Context, command types.Command) error { - PrintRingNameIfDebug(ctx, command) - err := command.Run(ctx) - if err != nil { - return errors.WithStack(err) - } - return nil -} - type includeCacheEntry struct { Sourcefile *paths.Path Include string @@ -318,10 +309,10 @@ func writeCache(cache *includeCache, path *paths.Path) error { func findIncludesUntilDone(ctx *types.Context, cache *includeCache, sourceFileQueue *types.UniqueSourceFileQueue) error { sourceFile := sourceFileQueue.Pop() - sourcePath := sourceFile.SourcePath(ctx) + sourcePath := sourceFile.SourcePath() targetFilePath := paths.NullPath() - depPath := sourceFile.DepfilePath(ctx) - objPath := sourceFile.ObjectPath(ctx) + depPath := sourceFile.DepfilePath() + objPath := sourceFile.ObjectPath() // TODO: This should perhaps also compare against the // include.cache file timestamp. Now, it only checks if the file @@ -342,28 +333,21 @@ func findIncludesUntilDone(ctx *types.Context, cache *includeCache, sourceFileQu first := true for { - var missingIncludeH string cache.ExpectFile(sourcePath) + // Libraries may require the "utility" directory to be added to the include + // search path, but only for the source code of the library, so we temporary + // copy the current search path list and add the library' utility directory + // if needed. includeFolders := ctx.IncludeFolders - if library, ok := sourceFile.Origin.(*libraries.Library); ok && library.UtilityDir != nil { - includeFolders = append(includeFolders, library.UtilityDir) - } - - if library, ok := sourceFile.Origin.(*libraries.Library); ok { - if library.Precompiled && library.PrecompiledWithSources { - // Fully precompiled libraries should have no dependencies - // to avoid ABI breakage - if ctx.Verbose { - ctx.Info(tr("Skipping dependencies detection for precompiled library %[1]s", library.Name)) - } - return nil - } + if extraInclude := sourceFile.ExtraIncludePath(); extraInclude != nil { + includeFolders = append(includeFolders, extraInclude) } var preprocErr error var preprocStderr []byte + var missingIncludeH string if unchanged && cache.valid { missingIncludeH = cache.Next().Include if first && ctx.Verbose { @@ -376,14 +360,11 @@ func findIncludesUntilDone(ctx *types.Context, cache *includeCache, sourceFileQu ctx.WriteStdout(preprocStdout) } // Unwrap error and see if it is an ExitError. - _, isExitErr := errors.Cause(preprocErr).(*exec.ExitError) if preprocErr == nil { // Preprocessor successful, done missingIncludeH = "" - } else if !isExitErr || preprocStderr == nil { - // Ignore ExitErrors (e.g. gcc returning - // non-zero status), but bail out on - // other errors + } else if _, isExitErr := errors.Cause(preprocErr).(*exec.ExitError); !isExitErr || preprocStderr == nil { + // Ignore ExitErrors (e.g. gcc returning non-zero status), but bail out on other errors return errors.WithStack(preprocErr) } else { missingIncludeH = IncludesFinderWithRegExp(string(preprocStderr)) @@ -426,9 +407,16 @@ func findIncludesUntilDone(ctx *types.Context, cache *includeCache, sourceFileQu // include scanning ctx.ImportedLibraries = append(ctx.ImportedLibraries, library) appendIncludeFolder(ctx, cache, sourcePath, missingIncludeH, library.SourceDir) - sourceDirs := library.SourceDirs() - for _, sourceDir := range sourceDirs { - queueSourceFilesFromFolder(ctx, sourceFileQueue, library, sourceDir.Dir, sourceDir.Recurse) + + if library.Precompiled && library.PrecompiledWithSources { + // Fully precompiled libraries should have no dependencies to avoid ABI breakage + if ctx.Verbose { + ctx.Info(tr("Skipping dependencies detection for precompiled library %[1]s", library.Name)) + } + } else { + for _, sourceDir := range library.SourceDirs() { + queueSourceFilesFromFolder(ctx, sourceFileQueue, library, sourceDir.Dir, sourceDir.Recurse) + } } first = false } @@ -499,3 +487,29 @@ func ResolveLibrary(ctx *types.Context, header string) *libraries.Library { return selected } + +func failIfImportedLibraryIsWrong(ctx *types.Context) error { + if len(ctx.ImportedLibraries) == 0 { + return nil + } + + for _, library := range ctx.ImportedLibraries { + if !library.IsLegacy { + if library.InstallDir.Join("arch").IsDir() { + return errors.New(tr("%[1]s folder is no longer supported! See %[2]s for more information", "'arch'", "http://goo.gl/gfFJzU")) + } + for _, propName := range libraries.MandatoryProperties { + if !library.Properties.ContainsKey(propName) { + return errors.New(tr("Missing '%[1]s' from library in %[2]s", propName, library.InstallDir)) + } + } + if library.Layout == libraries.RecursiveLayout { + if library.UtilityDir != nil { + return errors.New(tr("Library can't use both '%[1]s' and '%[2]s' folders. Double check in '%[3]s'.", "src", "utility", library.InstallDir)) + } + } + } + } + + return nil +} diff --git a/legacy/builder/fail_if_imported_library_is_wrong.go b/legacy/builder/fail_if_imported_library_is_wrong.go deleted file mode 100644 index 34a69d8d9c2..00000000000 --- a/legacy/builder/fail_if_imported_library_is_wrong.go +++ /dev/null @@ -1,52 +0,0 @@ -// This file is part of arduino-cli. -// -// Copyright 2020 ARDUINO SA (http://www.arduino.cc/) -// -// This software is released under the GNU General Public License version 3, -// which covers the main part of arduino-cli. -// The terms of this license can be found at: -// https://www.gnu.org/licenses/gpl-3.0.en.html -// -// You can be released from the requirements of the above licenses by purchasing -// a commercial license. Buying such a license is mandatory if you want to -// modify or otherwise use the software for commercial activities involving the -// Arduino software without disclosing the source code of your own applications. -// To purchase a commercial license, send an email to license@arduino.cc. - -package builder - -import ( - "errors" - - "github.com/arduino/arduino-cli/arduino/libraries" - "github.com/arduino/arduino-cli/legacy/builder/constants" - "github.com/arduino/arduino-cli/legacy/builder/types" -) - -type FailIfImportedLibraryIsWrong struct{} - -func (s *FailIfImportedLibraryIsWrong) Run(ctx *types.Context) error { - if len(ctx.ImportedLibraries) == 0 { - return nil - } - - for _, library := range ctx.ImportedLibraries { - if !library.IsLegacy { - if library.InstallDir.Join(constants.LIBRARY_FOLDER_ARCH).IsDir() { - return errors.New(tr("%[1]s folder is no longer supported! See %[2]s for more information", "'arch'", "http://goo.gl/gfFJzU")) - } - for _, propName := range libraries.MandatoryProperties { - if !library.Properties.ContainsKey(propName) { - return errors.New(tr("Missing '%[1]s' from library in %[2]s", propName, library.InstallDir)) - } - } - if library.Layout == libraries.RecursiveLayout { - if library.UtilityDir != nil { - return errors.New(tr("Library can't use both '%[1]s' and '%[2]s' folders. Double check in '%[3]s'.", "src", "utility", library.InstallDir)) - } - } - } - } - - return nil -} diff --git a/legacy/builder/types/types.go b/legacy/builder/types/types.go index 938b9658476..2ebf92bae5a 100644 --- a/legacy/builder/types/types.go +++ b/legacy/builder/types/types.go @@ -24,69 +24,75 @@ import ( ) type SourceFile struct { - // Sketch or Library pointer that this source file lives in - Origin interface{} // Path to the source file within the sketch/library root folder - RelativePath *paths.Path + relativePath *paths.Path + + // ExtraIncludePath contains an extra include path that must be + // used to compile this source file. + // This is mainly used for source files that comes from old-style libraries + // (Arduino IDE <1.5) requiring an extra include path to the "utility" folder. + extraIncludePath *paths.Path + + // The source root for the given origin, where its source files + // can be found. Prepending this to SourceFile.RelativePath will give + // the full path to that source file. + sourceRoot *paths.Path + + // The build root for the given origin, where build products will + // be placed. Any directories inside SourceFile.RelativePath will be + // appended here. + buildRoot *paths.Path } func (f *SourceFile) Equals(g *SourceFile) bool { - return f.Origin == g.Origin && - f.RelativePath.EqualsTo(g.RelativePath) + return f.relativePath.EqualsTo(g.relativePath) && + f.buildRoot.EqualsTo(g.buildRoot) && + f.sourceRoot.EqualsTo(g.sourceRoot) } // Create a SourceFile containing the given source file path within the // given origin. The given path can be absolute, or relative within the // origin's root source folder func MakeSourceFile(ctx *Context, origin interface{}, path *paths.Path) (*SourceFile, error) { - if path.IsAbs() { - var err error - path, err = sourceRoot(ctx, origin).RelTo(path) - if err != nil { - return nil, err - } - } - return &SourceFile{Origin: origin, RelativePath: path}, nil -} + res := &SourceFile{} -// Return the build root for the given origin, where build products will -// be placed. Any directories inside SourceFile.RelativePath will be -// appended here. -func buildRoot(ctx *Context, origin interface{}) *paths.Path { switch o := origin.(type) { case *sketch.Sketch: - return ctx.SketchBuildPath + res.buildRoot = ctx.SketchBuildPath + res.sourceRoot = ctx.SketchBuildPath case *libraries.Library: - return ctx.LibrariesBuildPath.Join(o.DirName) + res.buildRoot = ctx.LibrariesBuildPath.Join(o.DirName) + res.sourceRoot = o.SourceDir + res.extraIncludePath = o.UtilityDir default: panic("Unexpected origin for SourceFile: " + fmt.Sprint(origin)) } -} -// Return the source root for the given origin, where its source files -// can be found. Prepending this to SourceFile.RelativePath will give -// the full path to that source file. -func sourceRoot(ctx *Context, origin interface{}) *paths.Path { - switch o := origin.(type) { - case *sketch.Sketch: - return ctx.SketchBuildPath - case *libraries.Library: - return o.SourceDir - default: - panic("Unexpected origin for SourceFile: " + fmt.Sprint(origin)) + if path.IsAbs() { + var err error + path, err = res.sourceRoot.RelTo(path) + if err != nil { + return nil, err + } } + res.relativePath = path + return res, nil +} + +func (f *SourceFile) ExtraIncludePath() *paths.Path { + return f.extraIncludePath } -func (f *SourceFile) SourcePath(ctx *Context) *paths.Path { - return sourceRoot(ctx, f.Origin).JoinPath(f.RelativePath) +func (f *SourceFile) SourcePath() *paths.Path { + return f.sourceRoot.JoinPath(f.relativePath) } -func (f *SourceFile) ObjectPath(ctx *Context) *paths.Path { - return buildRoot(ctx, f.Origin).Join(f.RelativePath.String() + ".o") +func (f *SourceFile) ObjectPath() *paths.Path { + return f.buildRoot.Join(f.relativePath.String() + ".o") } -func (f *SourceFile) DepfilePath(ctx *Context) *paths.Path { - return buildRoot(ctx, f.Origin).Join(f.RelativePath.String() + ".d") +func (f *SourceFile) DepfilePath() *paths.Path { + return f.buildRoot.Join(f.relativePath.String() + ".d") } type LibraryResolutionResult struct {