Skip to content
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

Don't scan hidden directories in arduino-builder (.git, etc.) #366

Merged
merged 3 commits into from Sep 5, 2019

Conversation

earlephilhower
Copy link
Contributor

Port of arduino/arduino-builder#328

When checking for updated core files, don't scan inside UNIX hidden
directories like .git. Scanning inside a GIT tree for a large project
with many branches could take many minutes of runtime and multiple GB of
RAM. This was seen in the Arduino core for the ESP8266, but is probably
also an issue for other core development teams.

Fixes arduino/arduino-builder#327

Signed-off-by: Earle F. Philhower, III earlephilhower@yahoo.com

@CLAassistant
Copy link

CLAassistant commented Aug 28, 2019

CLA assistant check
All committers have signed the CLA.

@earlephilhower
Copy link
Contributor Author

I see this change is already in the new -cli. Thanks! Closing.

@d-a-v
Copy link
Contributor

d-a-v commented Aug 28, 2019

Sadly,
using today's arduino-nightly IDE, arduino-builder is still scanning my .git directory located in my hardware repository (under hardware/ from arduino IDE root dir).
@gvarisco @matthijskooijman @facchinm (from arduino/arduino-builder#328)

@earlephilhower
Copy link
Contributor Author

Reopening as a WIP. I'll need to find the core scanner as well and apply the same sort of logic that's being done on the sketch directory.

@earlephilhower earlephilhower changed the title Don't scan hidden directories in arduino-builder (.git, etc.) WIP - Don't scan hidden directories in arduino-builder (.git, etc.) Aug 29, 2019
@masci
Copy link
Contributor

masci commented Aug 29, 2019

Can you rebase on master so the tests can run? Sorry for the trouble!

@earlephilhower
Copy link
Contributor Author

Sure thing. I'll do the rebase by EOD my time.

I also noted in the Sketch file-filtering routine that cvs and rcs dirs are also skipped, so I think a refactor of both the core and sketch file filtering to use a single common "IgnoreDir()" function will be in order. Don't want to duplicate code, incompletely, through the codebase, so this will need a little more work.

@masci masci added the status: in progress Work is in progress on this label Aug 30, 2019
Arduino-builder scans the core to determine if core config files have
changed between invocations.  Unfortunately, it goes into SCCS dirs
such as .git (which can have 100,000+ files).  This is wasted effort
and can cause massive amounts of runtime and memory use when core
developers are building in their git clones.

Use a new function already added by the builder/utils.go to determine
if a file or folder is SCCS, and if so skip it in the rebuild-required
check.

Fixes arduino/arduino-builder#327

Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com>
@earlephilhower earlephilhower changed the title WIP - Don't scan hidden directories in arduino-builder (.git, etc.) Don't scan hidden directories in arduino-builder (.git, etc.) Aug 30, 2019
@earlephilhower
Copy link
Contributor Author

@masci, thanks for updating the tag. I had to get a new Go build environment before I could rebase/fix the PR. I can confirm @d-a-v's observation that git head of both repos does result in the 4.5GB RAM and 3:00++ min runtime, and that this PR as-is fixes the issue in my testing.

The new PR simply hooks the scans that arduino-builder already applies when looking at source folders to be scanned when checking that the Arduino *.txt (platform.txt, boards.txt, mainly) files are not updated since last build.

@earlephilhower
Copy link
Contributor Author

CI failure looks like a Codecov token issue, not related the the PR (I hope!).

@d-a-v
Copy link
Contributor

d-a-v commented Sep 2, 2019

@masci Is it possible re-trigger CI for this PR ?

@masci
Copy link
Contributor

masci commented Sep 2, 2019

git seems to be in an inconsistent state, checkout is failing with this message: https://github.com/arduino/arduino-cli/pull/366/checks?check_run_id=209807044#step:3:346

This seems to happen to others as well, see actions/checkout#23 but I'm afraid there's nothing I can do to fix this. @earlephilhower can you try adding another commit to your branch, or pick the commit into a brand new branch? (no idea if this will change anything, just speculation)

@earlephilhower
Copy link
Contributor Author

Thanks, @masci. I've pushed in a no-op change and am seeing the GH actions working now. We'll see in a few minutes if we're all good.

@masci
Copy link
Contributor

masci commented Sep 2, 2019

@earlephilhower we're good now.

The remaining failure is bad copypasta (fat fingers are mine), this line https://github.com/arduino/arduino-cli/blob/master/.github/workflows/test.yaml#L80 should be added here as well https://github.com/arduino/arduino-cli/blob/master/.github/workflows/test.yaml#L66

@earlephilhower
Copy link
Contributor Author

Is the copypasta fix something you want in this PR? Seems like it's better to have it in a separate push for sanity's sake, then I would just merge upstream/master and the commit logs would be comprehensive.

@masci
Copy link
Contributor

masci commented Sep 2, 2019

@earlephilhower you can rebase on master, I've merged the copypasta fix and that should give us a green build

@earlephilhower
Copy link
Contributor Author

Just a ping to say the WIP label can be removed at your convenience. With your fix it's building and running cleanly now on my system and in CI.

@masci masci removed the status: in progress Work is in progress on this label Sep 4, 2019
@rsora rsora merged commit 84172d8 into arduino:master Sep 5, 2019
rsora pushed a commit that referenced this pull request Sep 5, 2019
* Don't scan hidden directories in arduino-builder

Arduino-builder scans the core to determine if core config files have
changed between invocations.  Unfortunately, it goes into SCCS dirs
such as .git (which can have 100,000+ files).  This is wasted effort
and can cause massive amounts of runtime and memory use when core
developers are building in their git clones.

Use a new function already added by the builder/utils.go to determine
if a file or folder is SCCS, and if so skip it in the rebuild-required
check.

Fixes arduino/arduino-builder#327

Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com>

* Add comment to force rebuild
@earlephilhower earlephilhower deleted the skiphidden branch September 5, 2019 14:10
@d-a-v
Copy link
Contributor

d-a-v commented Sep 7, 2019

Many thanks for the fix, and for merging it quickly. Performance increase is just tremendous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please prevent from scanning hidden directories
6 participants