Load contents of source files only when needed (#559)

* [skip changelog] Pass path and contents separately to SketchSaveItemCpp

Previously, these two arguments were wrapped together in a sketch.Item,
but since all callers build such an item during the call, there is no
compelling reason to do it like this. This commit splits the Item
parameter into a separate path and contents, which prepares for removing
the file contents from sketch.Item later.

* [skip changelog] Let SketchMergeSources return error

This adds an error return value, which is currently always nil. This
prepares for making changes that require returning errors.

* [skip changelog] Let sketch.Item.GetSourceStr return error

This adds an error return value to this method, which is currently
always nil. This prepares for actually returning errors later.

* Load sketch file contents only when needed

Previously, the full contents of *all* sketch files would be loaded into
memory. This includes all source and header files inside the sketch
directory, even when they will not even be compiled (e.g.
subdirectories other than src). In practice, only the .ino file contents
will actually be used, so these are now read on demand.

Note that when copying the sketch into the build directory, the contents
of all these sketch files *is* used, but that code (`writeIfDifferent()`
in `arduino/builder/sketch.go`) already did not use the preloaded data
but read the file contents when copying.

For small sketches, this does not make much of a difference, but bigger
sketches, especially when they include libraries, core definitions,
tools, examples, documentation, etc. the memory usage can quite explode,
for no good reason.

Author: matthijskooijman

Diff for: arduino/builder/sketch.go

@@ -47,17 +47,17 @@ func QuoteCppString(str string) string {
 }
 
 // SketchSaveItemCpp saves a preprocessed .cpp sketch file on disk
-func SketchSaveItemCpp(item *sketch.Item, destPath string) error {
+func SketchSaveItemCpp(path string, contents []byte, destPath string) error {
 
-	sketchName := filepath.Base(item.Path)
+	sketchName := filepath.Base(path)
 
 	if err := os.MkdirAll(destPath, os.FileMode(0755)); err != nil {
 		return errors.Wrap(err, "unable to create a folder to save the sketch")
 	}
 
 	destFile := filepath.Join(destPath, sketchName+".cpp")
 
-	if err := ioutil.WriteFile(destFile, item.Source, os.FileMode(0644)); err != nil {
+	if err := ioutil.WriteFile(destFile, contents, os.FileMode(0644)); err != nil {
 		return errors.Wrap(err, "unable to save the sketch on disk")
 	}
 
@@ -216,26 +216,34 @@ func SketchLoad(sketchPath, buildPath string) (*sketch.Sketch, error) {
 }
 
 // SketchMergeSources merges all the source files included in a sketch
-func SketchMergeSources(sketch *sketch.Sketch) (int, string) {
+func SketchMergeSources(sketch *sketch.Sketch) (int, string, error) {
 	lineOffset := 0
 	mergedSource := ""
 
 	// add Arduino.h inclusion directive if missing
-	if !includesArduinoH.MatchString(sketch.MainFile.GetSourceStr()) {
+	mainSrc, err := sketch.MainFile.GetSourceStr()
+	if err != nil {
+		return 0, "", err
+	}
+	if !includesArduinoH.MatchString(mainSrc) {
 		mergedSource += "#include <Arduino.h>\n"
 		lineOffset++
 	}
 
 	mergedSource += "#line 1 " + QuoteCppString(sketch.MainFile.Path) + "\n"
-	mergedSource += sketch.MainFile.GetSourceStr() + "\n"
+	mergedSource += mainSrc + "\n"
 	lineOffset++
 
 	for _, item := range sketch.OtherSketchFiles {
+		src, err := item.GetSourceStr()
+		if err != nil {
+			return 0, "", err
+		}
 		mergedSource += "#line 1 " + QuoteCppString(item.Path) + "\n"
-		mergedSource += item.GetSourceStr() + "\n"
+		mergedSource += src + "\n"
 	}
 
-	return lineOffset, mergedSource
+	return lineOffset, mergedSource, nil
 }
 
 // SketchCopyAdditionalFiles copies the additional files for a sketch to the

Diff for: arduino/builder/sketch_test.go

@@ -25,7 +25,6 @@ import (
 	"testing"
 
 	"github.com/arduino/arduino-cli/arduino/builder"
-	"github.com/arduino/arduino-cli/arduino/sketch"
 	"github.com/stretchr/testify/require"
 )
 
@@ -40,7 +39,7 @@ func TestSaveSketch(t *testing.T) {
 		t.Fatalf("unable to read golden file %s: %v", sketchFile, err)
 	}
 
-	builder.SketchSaveItemCpp(&sketch.Item{Path: sketchName, Source: source}, tmp)
+	builder.SketchSaveItemCpp(sketchName, source, tmp)
 
 	out, err := ioutil.ReadFile(filepath.Join(tmp, outName))
 	if err != nil {
@@ -181,7 +180,8 @@ func TestMergeSketchSources(t *testing.T) {
 		t.Fatalf("unable to read golden file %s: %v", mergedPath, err)
 	}
 
-	offset, source := builder.SketchMergeSources(s)
+	offset, source, err := builder.SketchMergeSources(s)
+	require.Nil(t, err)
 	require.Equal(t, 2, offset)
 	require.Equal(t, string(mergedBytes), source)
 }
@@ -192,7 +192,8 @@ func TestMergeSketchSourcesArduinoIncluded(t *testing.T) {
 	require.NotNil(t, s)
 
 	// ensure not to include Arduino.h when it's already there
-	_, source := builder.SketchMergeSources(s)
+	_, source, err := builder.SketchMergeSources(s)
+	require.Nil(t, err)
 	require.Equal(t, 1, strings.Count(source, "<Arduino.h>"))
 }
 

Diff for: arduino/sketch/sketch.go

@@ -27,25 +27,32 @@ import (
 
 // Item holds the source and the path for a single sketch file
 type Item struct {
-	Path   string
-	Source []byte
+	Path string
 }
 
 // NewItem reads the source code for a sketch item and returns an
 // Item instance
-func NewItem(itemPath string) (*Item, error) {
+func NewItem(itemPath string) *Item {
+	return &Item{itemPath}
+}
+
+// GetSourceBytes reads the item file contents and returns it as bytes
+func (i *Item) GetSourceBytes() ([]byte, error) {
 	// read the file
-	source, err := ioutil.ReadFile(itemPath)
+	source, err := ioutil.ReadFile(i.Path)
 	if err != nil {
 		return nil, errors.Wrap(err, "error reading source file")
 	}
-
-	return &Item{itemPath, source}, nil
+	return source, nil
 }
 
-// GetSourceStr returns the Source contents in string format
-func (i *Item) GetSourceStr() string {
-	return string(i.Source)
+// GetSourceStr reads the item file contents and returns it as a string
+func (i *Item) GetSourceStr() (string, error) {
+	source, err := i.GetSourceBytes()
+	if err != nil {
+		return "", err
+	}
+	return string(source), nil
 }
 
 // ItemByPath implements sort.Interface for []Item based on
@@ -73,10 +80,7 @@ func New(sketchFolderPath, mainFilePath, buildPath string, allFilesPaths []strin
 	pathToItem := make(map[string]*Item)
 	for _, p := range allFilesPaths {
 		// create an Item
-		item, err := NewItem(p)
-		if err != nil {
-			return nil, errors.Wrap(err, "error creating the sketch")
-		}
+		item := NewItem(p)
 
 		if p == mainFilePath {
 			// store the main sketch file

Diff for: arduino/sketch/sketch_test.go

@@ -26,22 +26,26 @@ import (
 
 func TestNewItem(t *testing.T) {
 	sketchItem := filepath.Join("testdata", t.Name()+".ino")
-	item, err := sketch.NewItem(sketchItem)
-	assert.Nil(t, err)
+	item := sketch.NewItem(sketchItem)
 	assert.Equal(t, sketchItem, item.Path)
-	assert.Equal(t, []byte(`#include <testlib.h>`), item.Source)
-	assert.Equal(t, "#include <testlib.h>", item.GetSourceStr())
+	sourceBytes, err := item.GetSourceBytes()
+	assert.Nil(t, err)
+	assert.Equal(t, []byte(`#include <testlib.h>`), sourceBytes)
+	sourceStr, err := item.GetSourceStr()
+	assert.Nil(t, err)
+	assert.Equal(t, "#include <testlib.h>", sourceStr)
 
-	item, err = sketch.NewItem("doesnt/exist")
-	assert.Nil(t, item)
+	item = sketch.NewItem("doesnt/exist")
+	sourceBytes, err = item.GetSourceBytes()
+	assert.Nil(t, sourceBytes)
 	assert.NotNil(t, err)
 }
 
 func TestSort(t *testing.T) {
 	items := []*sketch.Item{
-		&sketch.Item{"foo", nil},
-		&sketch.Item{"baz", nil},
-		&sketch.Item{"bar", nil},
+		&sketch.Item{"foo"},
+		&sketch.Item{"baz"},
+		&sketch.Item{"bar"},
 	}
 
 	sort.Sort(sketch.ItemByPath(items))

Diff for: legacy/builder/container_add_prototypes.go

@@ -17,7 +17,6 @@ package builder
 
 import (
 	bldr "github.com/arduino/arduino-cli/arduino/builder"
-	"github.com/arduino/arduino-cli/arduino/sketch"
 	"github.com/arduino/arduino-cli/legacy/builder/constants"
 	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
@@ -54,7 +53,7 @@ func (s *ContainerAddPrototypes) Run(ctx *types.Context) error {
 		}
 	}
 
-	if err := bldr.SketchSaveItemCpp(&sketch.Item{ctx.Sketch.MainFile.Name.String(), []byte(ctx.Source)}, ctx.SketchBuildPath.String()); err != nil {
+	if err := bldr.SketchSaveItemCpp(ctx.Sketch.MainFile.Name.String(), []byte(ctx.Source), ctx.SketchBuildPath.String()); err != nil {
 		return i18n.WrapError(err)
 	}
 

Diff for: legacy/builder/container_merge_copy_sketch_files.go

@@ -17,7 +17,6 @@ package builder
 
 import (
 	bldr "github.com/arduino/arduino-cli/arduino/builder"
-	"github.com/arduino/arduino-cli/arduino/sketch"
 	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
 	"github.com/go-errors/errors"
@@ -30,11 +29,14 @@ func (s *ContainerMergeCopySketchFiles) Run(ctx *types.Context) error {
 	if sk == nil {
 		return i18n.WrapError(errors.New("unable to convert legacy sketch to the new type"))
 	}
-	offset, source := bldr.SketchMergeSources(sk)
+	offset, source, err := bldr.SketchMergeSources(sk)
+	if err != nil {
+		return err
+	}
 	ctx.LineOffset = offset
 	ctx.Source = source
 
-	if err := bldr.SketchSaveItemCpp(&sketch.Item{ctx.Sketch.MainFile.Name.String(), []byte(ctx.Source)}, ctx.SketchBuildPath.String()); err != nil {
+	if err := bldr.SketchSaveItemCpp(ctx.Sketch.MainFile.Name.String(), []byte(ctx.Source), ctx.SketchBuildPath.String()); err != nil {
 		return i18n.WrapError(err)
 	}
 

Diff for: legacy/builder/preprocess_sketch.go

@@ -24,7 +24,6 @@ import (
 	"strings"
 
 	bldr "github.com/arduino/arduino-cli/arduino/builder"
-	"github.com/arduino/arduino-cli/arduino/sketch"
 	"github.com/arduino/arduino-cli/legacy/builder/constants"
 	"github.com/arduino/arduino-cli/legacy/builder/i18n"
 	"github.com/arduino/arduino-cli/legacy/builder/types"
@@ -68,7 +67,7 @@ func (s *PreprocessSketchArduino) Run(ctx *types.Context) error {
 	if ctx.CodeCompleteAt != "" {
 		err = new(OutputCodeCompletions).Run(ctx)
 	} else {
-		err = bldr.SketchSaveItemCpp(&sketch.Item{ctx.Sketch.MainFile.Name.String(), []byte(ctx.Source)}, ctx.SketchBuildPath.String())
+		err = bldr.SketchSaveItemCpp(ctx.Sketch.MainFile.Name.String(), []byte(ctx.Source), ctx.SketchBuildPath.String())
 	}
 
 	return err

Diff for: legacy/builder/sketch_loader.go

@@ -88,11 +88,7 @@ func collectAllSketchFiles(from *paths.Path) (paths.PathList, error) {
 func makeSketch(sketchLocation *paths.Path, allSketchFilePaths paths.PathList, buildLocation *paths.Path, logger i18n.Logger) (*types.Sketch, error) {
 	sketchFilesMap := make(map[string]types.SketchFile)
 	for _, sketchFilePath := range allSketchFilePaths {
-		source, err := sketchFilePath.ReadFile()
-		if err != nil {
-			return nil, i18n.WrapError(err)
-		}
-		sketchFilesMap[sketchFilePath.String()] = types.SketchFile{Name: sketchFilePath, Source: string(source)}
+		sketchFilesMap[sketchFilePath.String()] = types.SketchFile{Name: sketchFilePath}
 	}
 
 	mainFile := sketchFilesMap[sketchLocation.String()]

Diff for: legacy/builder/types/types.go

@@ -86,8 +86,7 @@ func (f *SourceFile) DepfilePath(ctx *Context) *paths.Path {
 }
 
 type SketchFile struct {
-	Name   *paths.Path
-	Source string
+	Name *paths.Path
 }
 
 type SketchFileSortByName []SketchFile
@@ -114,20 +113,17 @@ func SketchToLegacy(sketch *sketch.Sketch) *Sketch {
 	s := &Sketch{}
 	s.MainFile = SketchFile{
 		paths.New(sketch.MainFile.Path),
-		string(sketch.MainFile.Source),
 	}
 
 	for _, item := range sketch.OtherSketchFiles {
 		s.OtherSketchFiles = append(s.OtherSketchFiles, SketchFile{
 			paths.New(item.Path),
-			string(item.Source),
 		})
 	}
 
 	for _, item := range sketch.AdditionalFiles {
 		s.AdditionalFiles = append(s.AdditionalFiles, SketchFile{
 			paths.New(item.Path),
-			string(item.Source),
 		})
 	}
 
@@ -137,22 +133,19 @@ func SketchToLegacy(sketch *sketch.Sketch) *Sketch {
 func SketchFromLegacy(s *Sketch) *sketch.Sketch {
 	others := []*sketch.Item{}
 	for _, f := range s.OtherSketchFiles {
-		if i, err := sketch.NewItem(f.Name.String()); err == nil {
-			others = append(others, i)
-		}
+		i := sketch.NewItem(f.Name.String())
+		others = append(others, i)
 	}
 
 	additional := []*sketch.Item{}
 	for _, f := range s.AdditionalFiles {
-		if i, err := sketch.NewItem(f.Name.String()); err == nil {
-			additional = append(additional, i)
-		}
+		i := sketch.NewItem(f.Name.String())
+		additional = append(additional, i)
 	}
 
 	return &sketch.Sketch{
 		MainFile: &sketch.Item{
-			Path:   s.MainFile.Name.String(),
-			Source: []byte(s.MainFile.Source),
+			Path: s.MainFile.Name.String(),
 		},
 		LocationPath:     s.MainFile.Name.Parent().String(),
 		OtherSketchFiles: others,