Skip to content

Ensure that no spurious file is added to AdditionalFiles #299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Ensure that no spurious file is added to AdditionalFiles #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90b8a31
Ensure that no spurious file is added to AdditionalFiles
facchinm Oct 5, 2018
61d05d2
Always add additional files if buildpath is empty
facchinm Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions sketch_loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (s *SketchLoader) Run(ctx *types.Context) error {
return i18n.ErrorfWithLogger(logger, constants.MSG_CANT_FIND_SKETCH_IN_PATH, sketchLocation, filepath.Dir(sketchLocation))
}

sketch, err := makeSketch(sketchLocation, allSketchFilePaths, logger)
sketch, err := makeSketch(sketchLocation, allSketchFilePaths, ctx.BuildPath, logger)
if err != nil {
return i18n.WrapError(err)
}
Expand All @@ -100,7 +100,7 @@ func collectAllSketchFiles(from string) ([]string, error) {
return filePaths, i18n.WrapError(err)
}

func makeSketch(sketchLocation string, allSketchFilePaths []string, logger i18n.Logger) (*types.Sketch, error) {
func makeSketch(sketchLocation string, allSketchFilePaths []string, buildLocation string, logger i18n.Logger) (*types.Sketch, error) {
sketchFilesMap := make(map[string]types.SketchFile)
for _, sketchFilePath := range allSketchFilePaths {
source, err := ioutil.ReadFile(sketchFilePath)
Expand All @@ -123,7 +123,9 @@ func makeSketch(sketchLocation string, allSketchFilePaths []string, logger i18n.
otherSketchFiles = append(otherSketchFiles, sketchFile)
}
} else if ADDITIONAL_FILE_VALID_EXTENSIONS[ext] {
additionalFiles = append(additionalFiles, sketchFile)
if !strings.Contains(filepath.Dir(sketchFile.Name), buildLocation) || buildLocation == "" {
additionalFiles = append(additionalFiles, sketchFile)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is not necessary, as recursive compilation is already limited to the src/ subdirectory. Have you confirmed that this check ever returns false with a build dir inside the sketch dir?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked and if I only remove the other line all temporary cpp files get added top the list (even if not in src)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also a test that should not pass if files outside src were not added to the list (https://github.com/arduino/arduino-builder/blob/master/test/sketch_loader_test.go#L129) . Is it intended @cmaglie ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test is there it probably means something. you should go back to the commit that added those lines and check why they have been added...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps there's a difference between the files that end up in the "additional files" list and the files that end up being compiled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the commit that added the "extra" files to the test: 9d7d1d7

} else {
return nil, i18n.ErrorfWithLogger(logger, constants.MSG_UNKNOWN_SKETCH_EXT, sketchFile.Name)
}
Expand Down
5 changes: 0 additions & 5 deletions wipeout_build_path_if_build_options_changed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"encoding/json"
"os"
"path/filepath"
"strings"

"github.com/arduino/arduino-builder/builder_utils"
"github.com/arduino/arduino-builder/constants"
Expand Down Expand Up @@ -86,10 +85,6 @@ func (s *WipeoutBuildPathIfBuildOptionsChanged) Run(ctx *types.Context) error {
if err != nil {
return i18n.WrapError(err)
}
// if build path is inside the sketch folder, also wipe ctx.AdditionalFiles
if strings.Contains(ctx.BuildPath, filepath.Dir(ctx.SketchLocation)) {
ctx.Sketch.AdditionalFiles = ctx.Sketch.AdditionalFiles[:0]
}
for _, file := range files {
os.RemoveAll(filepath.Join(buildPath, file.Name()))
}
Expand Down