From 1499123700f25d315b46c33946f21f241bf0ca87 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 16 Jan 2020 21:19:25 +0100 Subject: [PATCH 1/4] [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. --- arduino/builder/sketch.go | 6 +++--- arduino/builder/sketch_test.go | 3 +-- legacy/builder/container_add_prototypes.go | 3 +-- legacy/builder/container_merge_copy_sketch_files.go | 3 +-- legacy/builder/preprocess_sketch.go | 3 +-- 5 files changed, 7 insertions(+), 11 deletions(-) diff --git a/arduino/builder/sketch.go b/arduino/builder/sketch.go index 67ebfd93d48..45cbd494b0a 100644 --- a/arduino/builder/sketch.go +++ b/arduino/builder/sketch.go @@ -47,9 +47,9 @@ 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") @@ -57,7 +57,7 @@ func SketchSaveItemCpp(item *sketch.Item, destPath string) error { 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") } diff --git a/arduino/builder/sketch_test.go b/arduino/builder/sketch_test.go index 781acc7bb98..186de6f172e 100644 --- a/arduino/builder/sketch_test.go +++ b/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 { diff --git a/legacy/builder/container_add_prototypes.go b/legacy/builder/container_add_prototypes.go index 89494dff927..ddb1889367f 100644 --- a/legacy/builder/container_add_prototypes.go +++ b/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 --git a/legacy/builder/container_merge_copy_sketch_files.go b/legacy/builder/container_merge_copy_sketch_files.go index be7a953bc87..e5cc73b8ac0 100644 --- a/legacy/builder/container_merge_copy_sketch_files.go +++ b/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" @@ -34,7 +33,7 @@ func (s *ContainerMergeCopySketchFiles) Run(ctx *types.Context) error { 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 --git a/legacy/builder/preprocess_sketch.go b/legacy/builder/preprocess_sketch.go index 6a179eb7184..93e4fb97f70 100644 --- a/legacy/builder/preprocess_sketch.go +++ b/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 From 7b31ac3fca049eeefc581b9fd92eb7b1a2f049a4 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 16 Jan 2020 21:26:00 +0100 Subject: [PATCH 2/4] [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. --- arduino/builder/sketch.go | 4 ++-- arduino/builder/sketch_test.go | 6 ++++-- legacy/builder/container_merge_copy_sketch_files.go | 5 ++++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/arduino/builder/sketch.go b/arduino/builder/sketch.go index 45cbd494b0a..d7bfb02724a 100644 --- a/arduino/builder/sketch.go +++ b/arduino/builder/sketch.go @@ -216,7 +216,7 @@ 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 := "" @@ -235,7 +235,7 @@ func SketchMergeSources(sketch *sketch.Sketch) (int, string) { mergedSource += item.GetSourceStr() + "\n" } - return lineOffset, mergedSource + return lineOffset, mergedSource, nil } // SketchCopyAdditionalFiles copies the additional files for a sketch to the diff --git a/arduino/builder/sketch_test.go b/arduino/builder/sketch_test.go index 186de6f172e..1c186067a3a 100644 --- a/arduino/builder/sketch_test.go +++ b/arduino/builder/sketch_test.go @@ -180,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) } @@ -191,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, "")) } diff --git a/legacy/builder/container_merge_copy_sketch_files.go b/legacy/builder/container_merge_copy_sketch_files.go index e5cc73b8ac0..3f88c215434 100644 --- a/legacy/builder/container_merge_copy_sketch_files.go +++ b/legacy/builder/container_merge_copy_sketch_files.go @@ -29,7 +29,10 @@ 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 From 4d48dc54351980e20de1a758900d1605b4786e75 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 16 Jan 2020 21:28:38 +0100 Subject: [PATCH 3/4] [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. --- arduino/builder/sketch.go | 14 +++++++++++--- arduino/sketch/sketch.go | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/arduino/builder/sketch.go b/arduino/builder/sketch.go index d7bfb02724a..b540d9d943a 100644 --- a/arduino/builder/sketch.go +++ b/arduino/builder/sketch.go @@ -221,18 +221,26 @@ func SketchMergeSources(sketch *sketch.Sketch) (int, string, error) { 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 \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, nil diff --git a/arduino/sketch/sketch.go b/arduino/sketch/sketch.go index d0dbd6d407d..4a5c69c950d 100644 --- a/arduino/sketch/sketch.go +++ b/arduino/sketch/sketch.go @@ -44,8 +44,8 @@ func NewItem(itemPath string) (*Item, error) { } // GetSourceStr returns the Source contents in string format -func (i *Item) GetSourceStr() string { - return string(i.Source) +func (i *Item) GetSourceStr() (string, error) { + return string(i.Source), nil } // ItemByPath implements sort.Interface for []Item based on From 903247d0e15e83ec491a54b095683e8eaf6d3fd0 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 16 Jan 2020 21:33:04 +0100 Subject: [PATCH 4/4] 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. --- arduino/sketch/sketch.go | 28 ++++++++++++++++------------ arduino/sketch/sketch_test.go | 22 +++++++++++++--------- legacy/builder/sketch_loader.go | 6 +----- legacy/builder/types/types.go | 19 ++++++------------- 4 files changed, 36 insertions(+), 39 deletions(-) diff --git a/arduino/sketch/sketch.go b/arduino/sketch/sketch.go index 4a5c69c950d..7069beca882 100644 --- a/arduino/sketch/sketch.go +++ b/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 +// GetSourceStr reads the item file contents and returns it as a string func (i *Item) GetSourceStr() (string, error) { - return string(i.Source), nil + 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 --git a/arduino/sketch/sketch_test.go b/arduino/sketch/sketch_test.go index 8c37d3ae7d7..3e213fecba8 100644 --- a/arduino/sketch/sketch_test.go +++ b/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 `), item.Source) - assert.Equal(t, "#include ", item.GetSourceStr()) + sourceBytes, err := item.GetSourceBytes() + assert.Nil(t, err) + assert.Equal(t, []byte(`#include `), sourceBytes) + sourceStr, err := item.GetSourceStr() + assert.Nil(t, err) + assert.Equal(t, "#include ", 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 --git a/legacy/builder/sketch_loader.go b/legacy/builder/sketch_loader.go index 58d9268838d..c6cec1d8008 100644 --- a/legacy/builder/sketch_loader.go +++ b/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 --git a/legacy/builder/types/types.go b/legacy/builder/types/types.go index b8834f0c587..82a8122b13f 100644 --- a/legacy/builder/types/types.go +++ b/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,