Fix macos bug deleting too many files #10699
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you, I didn't notice it since I'm not using in-place build at all. |
nobu
reviewed
May 4, 2024
template/Makefile.in
Outdated
Comment on lines
496
to
497
| $(Q)find . -depth \( -name '*.bc' -o -name '*.[is]' \) -delete 2> /dev/null || true | ||
| -$(Q)$(RMALL) *.dSYM |
There was a problem hiding this comment.
Unfortunately, -delete is not standardized in POSIX find.
Suggested change
| $(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.
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. 👍🏼
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.
eileencodes
force-pushed
the
partially-revert-10209
branch
from
d7fa792
to
b3ff841
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since #10209 we've been noticing that on macos after running
make cleanthecoroutine/arm64/Context.Sfile is missing, causing subsequent make calls to fail becauseContext.Sis needed to buildContext.o.The reason this is happening is because macos is case-insensitive so the
.slooks forcoroutine/arm64/Context.sand findscoroutine/arm64/Context.Sand deletes it. This does not happen on linux because the filesystem is case sensitive.I attempted to use
findbecause it is case sensitive regardless of filesystem, but it was a lot slower thanrmsince 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