-
-
Notifications
You must be signed in to change notification settings - Fork 114
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.
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
Ensure that no spurious file is added to AdditionalFiles #299
Conversation
If the build is executed "in place" (inside the sketch folder) intermediate cpp files were added to the AdditionaFiles list. This was solved (badly) in arduino@849faa1#diff-b54df391d234f64238253f1d968e2e39R89 . This patch fixes the issue by not including these files in the list in the first place, so we can avoid removing them later.
additionalFiles = append(additionalFiles, sketchFile) | ||
if !strings.Contains(filepath.Dir(sketchFile.Name), buildLocation) { | ||
additionalFiles = append(additionalFiles, sketchFile) | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
Merged manually |
If the build is executed "in place" (inside the sketch folder) intermediate cpp files were added to the AdditionaFiles list.
This was solved (badly) in 849faa1#diff-b54df391d234f64238253f1d968e2e39R89 .
This patch fixes the issue by not including these files in the list in the first place, so we can avoid removing them later.