-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Fix macos bug deleting too many files #10699
Conversation
Thank you, I didn't notice it since I'm not using in-place build at all. |
template/Makefile.in
Outdated
$(Q)find . -depth \( -name '*.bc' -o -name '*.[is]' \) -delete 2> /dev/null || true | ||
-$(Q)$(RMALL) *.dSYM |
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.
Unfortunately, -delete
is not standardized in POSIX find.
$(Q)find . -depth \( -name '*.bc' -o -name '*.[is]' \) -delete 2> /dev/null || true | |
-$(Q)$(RMALL) *.dSYM | |
$(Q)find . ! -type d \( -name '*.bc' -o -name '*.[is]' \) -exec rm -f {} + || true |
Since we don't expect to delete directories here, -depth
is not needed.
Does find
print any error or warning messages?
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 made this change and tested it on macos and linux. I didn't see any errors or warnings. It also deleted the correct files. 👍🏼
d7fa792
to
b3ff841
Compare
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.
Thanks, LGTM.
I sometimes also use git clean -xfd
.
Since ruby#10209 we've been noticing that on macos after running `make clean` the `coroutine/arm64/Context.S` file is missing, causing subsequent make calls to fail because `Context.S` is needed to build `Context.o`. The reason this is happening is because macos is case-insensitive so the `.s` looks for `coroutine/arm64/Context.s` and finds `coroutine/arm64/Context.s`. This does not happen on linux because the filesystem is case sensitive. I attempted to use `find` because it is case sensitive regardless of filesystem, but it was a lot slower than `rm` since we can't pass multiple file names the same way to `find`. Reverting this small part of ruby#10209 fixes the issue for macos and it wasn't clear that those changes were strictly necessary for the rest of the PR. We changed the original code to use `rm` instead of `delete` because it is not standarized on POSIX.
b3ff841
to
1e8db06
Compare
Since #10209 we've been noticing that on macos after running
make clean
thecoroutine/arm64/Context.S
file is missing, causing subsequent make calls to fail becauseContext.S
is needed to buildContext.o
.The reason this is happening is because macos is case-insensitive so the
.s
looks forcoroutine/arm64/Context.s
and findscoroutine/arm64/Context.S
and deletes it. This does not happen on linux because the filesystem is case sensitive.I attempted to use
find
because it is case sensitive regardless of filesystem, but it was a lot slower thanrm
since we can't pass multiple file names the same way tofind
.Reverting this small part of #10209 fixes the issue for macos and it wasn't clear that those changes were strictly necessary for the rest of the PR.
cc/ @nobu as the author of #10209