[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.

Author: cmaglie

Diff for: 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 for: 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 for: legacy/builder/fail_if_imported_library_is_wrong.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 {