libcnb: Remove use of the stacker crate
#517
Conversation
Since: - The stdlib `remove_dir_all` recursive implementation (which this function is replacing) doesn't have stack overflow protection - Testing has proven it's unnecessary (see #516) In addition: - The function has been refactored to avoid recursing for every file (now only directories), which reduces depth of recursion by one, and saves an additional stat on every file, since the file type is already retrieved by `read_dir` so it's redundant to call the more expensive `symlink_metadata()` again on the next recursion. - `remove_recursively` has been renamed to `remove_dir_recursively` to more clearly reflect it's intended to operate on directories (and for closer parity to the name of `remove_dir_all`) GUS-W-11917787.
edmorley
force-pushed
the
edmorley/rm-stacker
branch
from
935e9ae to
15296ae
Compare
There was a problem hiding this comment.
Generally speaking, this PR makes a trade-off I wouldn't usually make (no surprise as the changed code was originally written by me 😄). When dealing with recursion I'd rather have a larger(-ish) binary (and the additional compile time for that code) and be sure that we won't smash the stack. I find debugging stack overflows rather hard, when the stack size varies across operating systems, configurations, optimisation level, etc. Especially when the code relies on external state (the FS in this case) that might be required to reproduce the issue.
In practice, removing the code that prevents stack overflows is probably fine in this context, as demonstrated by your testing. I therefore won't block this from merging.
Since:
remove_dir_allrecursive implementation (which this function is replacing) doesn't have stack overflow protection.stackerinremove_recursively()#516).In addition:
read_dirso it's redundant to call the more expensivesymlink_metadata()again on the next recursion.remove_recursivelyhas been renamed toremove_dir_recursivelyto more clearly reflect it's intended to operate on directories (and for closer parity to the name ofremove_dir_all).Closes #516.
GUS-W-11917787.